Add initial app host analyzers to the Aspire.Hosting.AppHost package

[DamianEdwards]

Description

This adds initial set of analyzers to Aspire.Hosting.AppHost and infrastructure to build more out in the future. Initial analyzers included are:

• ASPIRE006: Application model items must have valid names

Diagnostics are reported as errors as they'll result in runtime exceptions.

Contributes to #1209

The analyzer is based on hosting method parameters being decorated with a new ResourceNameAttribute or EndpointNameAttribute, which both implement the new interface IModelNameParameter. All relevant hosting methods (including in hosting integration packages) have been updated with these attributes as appropriate to enable the analyzer.

If we think requiring the attribute to enable the analyzers for a method is too burdensome, we could update the analyzer to also look for method operations invoked on IDistributedApplicationBuilder for methods named like AddXXX that pass a parameter string name. We could keep the attributes to enable other methods to participate where that pattern doesn't suffice, e.g. IResourceBuilder<PostgresResource>.AddDatabase(string name), as it's likely not correct to always assume methods like that result in resources being added to the application model with the specified name.

The analyzer infrastructure is copied from dotnet/aspnetcore.

Checklist

• Is this feature complete?
  • Yes. Ready to ship.
• Are you including unit tests for the changes and scenario tests if relevant?
  • Yes
• Did you add public API?
  • Yes
    • If yes, did you have an API Review for it?
      • No
    • Did you add <remarks /> and <code /> elements on your triple slash comments?
      • Yes
• Does the change make any security assumptions or guarantees?
  • No
• Does the change require an update in our Aspire docs?
  • Yes
    • Link to aspire-docs issue: [New article]: Docs for Aspire hosting analyzers docs-aspire#1658

Microsoft Reviewers: Open in CodeFlow

[mitchdenny]

The unique name detection is going to need to handle branches:

    [Fact]
    public async Task ResourceNameUniqueCanHandleTernaryBranches()
    {
        var test = AnalyzerTest.Create<AppHostAnalyzer>("""
            using Aspire.Hosting;

            var builder = DistributedApplication.CreateBuilder(args);

            var parameter = builder.ExecutionContext.IsPublishMode
                ? builder.AddParameter("x")
                : builder.AddParameter("x");
            """, []);

        await test.RunAsync();
    }

    [Fact]
    public async Task ResourceNameUniqueCanHandleIfBranches()
    {
        var test = AnalyzerTest.Create<AppHostAnalyzer>("""
            using Aspire.Hosting;

            var builder = DistributedApplication.CreateBuilder(args);

            IResourceBuilder<ParameterResource>? parameter;

            if (builder.ExecutionContext.IsPublishMode)
            {
                parameter = builder.AddParameter("x");
            }
            else
            {
                parameter = builder.AddParameter("x");
            }
            """, []);

        await test.RunAsync();
    }

    [Fact]
    public async Task ResourceNameUniqueCanHandleSwitchBranches()
    {
        var test = AnalyzerTest.Create<AppHostAnalyzer>("""
            using Aspire.Hosting;

            var builder = DistributedApplication.CreateBuilder(args);

            var parameter = builder.ExecutionContext.Operation switch {
                DistributedApplicationOperation.Run => builder.AddParameter("x"),
                _ => builder.AddParameter("x")
            };
            """, []);

        await test.RunAsync();
    }

[JamesNK]

ASPIRE0001: Resource names must be unique

I wrote an analyzer like this for ASP.NET Core routing and reporting duplicate routes. It took forever to get right.

With this kind of analyzer you have to be really conservative about finding duplicates. You can only report a duplicate if they are both in the same context: different methods, if branches, switch statements, ternary expressions, null coalesing, etc, etc.

[JamesNK]

My analyzer: https://github.com/dotnet/aspnetcore/blob/d05f3586adc0439595436ff6b8677548302fd64d/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/DetectAmbiguousRoutes.cs

My analyzer is very complex because it's possible to add extension methods to MapGet and friends, and as soon as you do that, you don't know if it is truely a duplicate or not. But you should be able to copy logic about only analyzing in the same context.

[mitchdenny]

Funny you say that @JamesNK as I was looking at this I was remembering that analyzer thinking what a good job you did with it around handling these corner cases (although some of these will be pretty common patterns in Aspire).

[JamesNK]

I didn't do that good a job. There were a bunch of backports during .NET 8 (9?) to fix issues.

[mitchdenny]

... eventually. :)

[DamianEdwards]

With this kind of analyzer you have to be really conservative about finding duplicates. You can only report a duplicate if they are both in the same context: different methods, if branches, switch statements, ternary expressions, null coalesing, etc, etc.

Hmm OK. I think I'll just remove it for now and we can revisit it later. This is mostly about getting the infra in and the first analyzer to verify the resource name is valid.

[eerhardt]

This is unfortunate that it needs to be copied to everywhere that wants to be an analyzer.

[DamianEdwards]

Yeah. I was talking about another case where we wanted to share sources earlier today and day-dreamed about built-in support for producing source packages directly from the CLI, e.g. dotnet pack --id MySourcePackage --version 0.0.1 --description "My Great source package" --sources ./**/*.cs,./**/*.resx, so we could avoid having to create special projects everywhere we want to share sources.

[eerhardt]

See also https://github.com/dotnet/aspire/tree/main/src/Tools/ConfigurationSchemaGenerator/RuntimeSource where we copied a TON of the Configuration.Binder source generator code. 😞

[eerhardt]

What does this mean and why can we just suppress it?

[DamianEdwards]

Copied from dotnet/aspnetcore, no idea 😄

[eerhardt]

Is 4.11 too new? It was only shipped a month ago.

If someone is using net8 and an older VS, this might break them.

[DamianEdwards]

How do we determine what the correct version to use is?

[DamianEdwards]

ASP.NET Core seems to be using 3.3.1 because it says some analyzers need to be used in VS2019, but perhaps that's out of date? https://github.com/dotnet/aspnetcore/blob/e25539b691b0425f94f66818ea0a0b4820f44804/eng/Versions.props#L247-L253

[DamianEdwards]

@jaredpar @dsplaisted what's the right way to determine which version of Microsoft.CodeAnalysis.CSharp our analyzer project should reference?

[DamianEdwards]

@JamesNK @joperezr @captainsafia I believe you all have experiences authoring/shipping analyzers before? Would be good to get someone with analyzer experience to sign-off on this.