Warn that UseStartup/Configure/ConfigureWebHost not supported on WebApplicationBuilder #35884

martincostello · 2021-08-28T11:19:34Z

PR Title

Add WebApplicationBuilder analyzers for unsupported methods

PR Description

Add analyzers that warn on incorrect usage of UseStartup(), Configure() and ConfigureWebHost() when using WebApplicationBuilder.Host and WebApplicationBuilder.WebHost.

TODO

Review the exact diagnostic IDs to use
Review the exact diagnostic messages to use
~~Add tests for existing startup patterns prior to .NET 6 to verify they do not generate false positives~~
~~Refactor analyzer to reduce duplication in detecting similar usage patterns~~

Fixes #35814

pranavkm

This looks great!

src/Framework/Analyzer/src/WebApplicationBuilder/DiagnosticDescriptors.cs

pranavkm · 2021-08-28T17:53:13Z

src/Framework/Analyzer/src/WebApplicationBuilder/DiagnosticDescriptors.cs

+    internal static readonly DiagnosticDescriptor DoNotUseConfigureWebHostWithConfigureHostBuilder = new(
+        "ASP0005",
+        "Do not use ConfigureWebHost with WebApplicationBuilder.Host",
+        "ConfigureWebHost cannot be used with WebApplicationBuilder.Host",


Should we say more about what the right thing to do would be? `Consider blah instead..."? @davidfowl what's the alternative?

Yeah I wasn't sure on the wording that would be wanted, so just went with something basic as a starting point.

pranavkm · 2021-08-28T17:54:37Z

...Framework/Analyzer/src/WebApplicationBuilder/DisallowConfigureHostBuilderConfigureWebHost.cs

+
+public partial class WebApplicationBuilderAnalyzer : DiagnosticAnalyzer
+{
+    private static void DisallowConfigureHostBuilderConfigureWebHost(


Btw, I'm super ok putting this in the same file as the anlayzer. The only reason I thought partials made sense for the minimal action one was because it had a bunch of different code to run per analysis.

pranavkm · 2021-08-28T17:55:45Z

src/Framework/Analyzer/src/WebApplicationBuilder/WebApplicationBuilderAnalyzer.cs

+        IMethodSymbol methodSymbol,
+        INamedTypeSymbol disallowedReceiverType,
+        string disallowedMethodName,
+        params INamedTypeSymbol[] disallowedMethodTypes)


Might want to avoid the params since it allocates per call. We could statically cache the array

src/Framework/Analyzer/src/WebApplicationBuilder/WebApplicationBuilderAnalyzer.cs

pranavkm · 2021-08-28T18:02:31Z

src/Framework/Analyzer/src/WebApplicationBuilder/WebApplicationBuilderAnalyzer.cs

+                return false;
+            }
+
+            return disallowedMethodTypes.Any(type => SymbolEqualityComparer.Default.Equals(type, methodSymbol.ContainingType));


Could we use a regular for loop? We should treat all of the analyzers as if they were running in the hot path all the time.

src/Framework/Analyzer/src/WebApplicationBuilder/WebApplicationBuilderAnalyzer.cs

pranavkm · 2021-08-28T18:10:33Z

...ramework/Analyzer/test/WebApplicationBuilder/DisallowConfigureWebHostBuilderConfigureTest.cs

+    public async Task WebApplicationBuilder_WebHostWithConfigureWithContext_ProducesDiagnostics()
+    {
+        // Arrange
+        var source = TestSource.Read(@"


I know it doesn't matter to the implementation, but could we add one test with producing diagnostics in a non-top level Program?

Sure - didn't think of that.

davidfowl · 2021-08-28T18:33:58Z

I want to feature creep this PR as I realize there are more APIs I want to block but this is awesome

martincostello · 2021-08-28T18:35:56Z

I want to feature creep this PR as I realize there are more APIs

😄

I'd be happy to add some more similar patterns as a follow-up post-merge if you've got a laundry list.

src/Framework/Analyzer/src/DiagnosticDescriptors.cs

martincostello · 2021-08-28T19:06:19Z

...work/Analyzer/test/WebApplicationBuilder/DisallowConfigureHostBuilderConfigureWebHostTest.cs

+    public static void Main(string[] args)
+    {
+        var builder = WebApplication.CreateBuilder(args);
+        /*MM*/builder.Host.ConfigureWebHost(webHostBuilder => { });


Is the marker here supposed to be on the line start instead of at the problematic invocation?

The pattern I've been following is to emit the diagnostic at the point where the problematic invocation exists so if the user clicks on the diagnostic's location in the build output the cursor will be directed to the place where they need to make the fix.

It also makes it easier to write codefixes associated with the analyzer because you can get the syntax at the problematic location right away.

Yeah I figured that would be the case. I originally put it before ConfigureWebHost, but then the tests failed even though I was just using the syntax given to the analyzer to get the location for the invocation. I'll dig around more and see if I can work out why there's a mismatch when I do that so the warning is in the best place.

...ramework/Analyzer/test/WebApplicationBuilder/DisallowConfigureWebHostBuilderConfigureTest.cs

...amework/Analyzer/test/WebApplicationBuilder/DisallowConfigureWebHostBuilderUseStartupTest.cs

martincostello · 2021-08-28T19:09:03Z

src/Framework/Analyzer/src/WebApplicationBuilder/WebApplicationBuilderAnalyzer.cs

+                return;
+            }
+
+            INamedTypeSymbol[] configureTypes = { wellKnownTypes.WebHostBuilderExtensions };


Couldn't statically cache as the compilation is needed to get the well-known types, but I guess the lifetime of the class is "long enough" that these locals being reused in the lambda are OK?

davidfowl · 2021-09-02T05:04:25Z

@pranavkm you are the gate here

davidfowl · 2021-09-02T05:04:41Z

@captainsafia you as well.

martincostello · 2021-09-04T11:31:02Z

src/Framework/Analyzer/src/WebApplicationBuilder/WebApplicationBuilderAnalyzer.cs

+                    var methodName = operation.Syntax
+                        .DescendantNodes()
+                        .OfType<SimpleNameSyntax>()
+                        .Where(node => node is IdentifierNameSyntax || node is GenericNameSyntax)
+                        .Where(node => string.Equals(node.Identifier.Value as string, operation.TargetMethod.Name, StringComparison.Ordinal))
+                        .FirstOrDefault();


Happy to refactor this if there's a better way I didn't work out for myself.

I assumed that using LINQ here was OK as we're already on the path where a diagnostic is going to be fired.

martincostello · 2021-09-04T11:34:16Z

...work/Analyzer/test/WebApplicationBuilder/DisallowConfigureHostBuilderConfigureWebHostTest.cs

+using Microsoft.Extensions.Hosting;
+var builder = WebApplication.CreateBuilder();
+builder.Host
+    .ConfigureServices(services => { }) // Because ConfigureServices() returns IHostBuilder, the type gets erased


Happy to remove this test if it doesn't add any value. It started as my check the analyzer did work, but then realising that as the receiver type on ConfigureWebHost() became IHostBuilder it could no longer be detected.

Alternatively I could try and make it actually fire, but that would mean walking up the invocation chain and trying to find the WebApplicationBuilder.Host at the root, but I'm not sure that's worth adding that additional complexity to try and cover so many possible usage patterns.

src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs

...CoreAnalyzers/test/WebApplicationBuilder/DisallowConfigureHostBuilderConfigureWebHostTest.cs

captainsafia · 2021-09-21T16:10:38Z

...ork/AspNetCoreAnalyzers/src/Analyzers/WebApplicationBuilder/WebApplicationBuilderAnalyzer.cs

+        }
+    }
+
+    // GetReceiverType() adapted from IOperationExtensions.GetReceiverType in dotnet/roslyn-analyzers.


Nit: We can move this to src/Shared/Roslyn.

Add analyzers that warn on incorrect usage of UseStartup(), Configure() and ConfigureWebHost() when using WebApplicationBuilder. Resolves dotnet#35814.

pranavkm · 2022-01-12T01:19:38Z

/azp run

azure-pipelines · 2022-01-12T01:20:02Z

Azure Pipelines successfully started running 3 pipeline(s).

ghost added area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework community-contribution Indicates that the PR has been added by a community member labels Aug 28, 2021

martincostello mentioned this pull request Aug 28, 2021

Analyzer: Warn that UseStartup/Configure and ConfigureWebHost isn't supported on WebApplicationBuilder #35814

Closed

martincostello marked this pull request as ready for review August 28, 2021 14:03

martincostello requested a review from Pilchie as a code owner August 28, 2021 14:04

BrennanConroy requested review from davidfowl and pranavkm August 28, 2021 15:05

pranavkm reviewed Aug 28, 2021

View reviewed changes

martincostello commented Aug 28, 2021

View reviewed changes

davidfowl requested a review from captainsafia September 2, 2021 05:04

martincostello force-pushed the WebApplicationBuilder-Configure-Analyzers-35814 branch from 36a5bae to dedc5d7 Compare September 4, 2021 11:29

martincostello commented Sep 4, 2021

View reviewed changes

martincostello requested a review from pranavkm September 10, 2021 09:34

martincostello force-pushed the WebApplicationBuilder-Configure-Analyzers-35814 branch from dedc5d7 to 358431d Compare September 21, 2021 10:56

captainsafia reviewed Sep 21, 2021

View reviewed changes

src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs Show resolved Hide resolved

captainsafia reviewed Sep 21, 2021

View reviewed changes

...CoreAnalyzers/test/WebApplicationBuilder/DisallowConfigureHostBuilderConfigureWebHostTest.cs Show resolved Hide resolved

captainsafia reviewed Sep 21, 2021

View reviewed changes

mkArtakMSFT added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework labels Oct 6, 2021

mkArtakMSFT assigned rafikiassumani-msft Oct 6, 2021

rafikiassumani-msft assigned captainsafia and unassigned rafikiassumani-msft Nov 4, 2021

Implement analyzers for WebApplicationBuilder usage

79f7251

Add analyzers that warn on incorrect usage of UseStartup(), Configure() and ConfigureWebHost() when using WebApplicationBuilder. Resolves dotnet#35814.

martincostello force-pushed the WebApplicationBuilder-Configure-Analyzers-35814 branch from c2b5322 to 79f7251 Compare November 6, 2021 10:01

rafikiassumani-msft assigned pranavkm and unassigned captainsafia Jan 11, 2022

captainsafia approved these changes Jan 12, 2022

View reviewed changes

captainsafia merged commit bc064f5 into dotnet:main Jan 12, 2022

ghost added this to the 7.0-preview1 milestone Jan 12, 2022

martincostello deleted the WebApplicationBuilder-Configure-Analyzers-35814 branch January 12, 2022 09:10

Warn that UseStartup/Configure/ConfigureWebHost not supported on WebApplicationBuilder #35884

Warn that UseStartup/Configure/ConfigureWebHost not supported on WebApplicationBuilder #35884

Uh oh!

Conversation

martincostello commented Aug 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Aug 28, 2021

Uh oh!

martincostello commented Aug 28, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Sep 2, 2021

Uh oh!

davidfowl commented Sep 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Jan 12, 2022

Uh oh!

azure-pipelines bot commented Jan 12, 2022

Uh oh!

Uh oh!

martincostello commented Aug 28, 2021 •

edited

Loading