dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.55k stars 10.05k forks source link

Warning AD0001 : Analyzer 'Microsoft.AspNetCore.Analyzers.RouteHandlers.RouteHandlerAnalyzer' NullReferenceException due to generics in minimal API #56831

Open madskonradsen opened 4 months ago

madskonradsen commented 4 months ago

Is there an existing issue for this?

Describe the bug

AspNetCore Analyzer throws warning due to usage of generics in minimal API:

1>CSC: Warning AD0001 : Analyzer 'Microsoft.AspNetCore.Analyzers.RouteHandlers.Route HandlerAnalyzer' threw an exception of type 'System.NullReferenceException' with message 'Object reference not set to an instance of an object.'.

Expected Behavior

This all works fine when running it, the analyzer is just not pleased with the usage of generics. Switching TEndpointInput to a concrete type such as DummyEndpointInput of course also make the warning disappear.

Steps To Reproduce

https://github.com/madskonradsen/minimal-reproducibles/tree/main/dotnet-analyzer-warning-minimalapi-generics/MinimalAPIGenericsWarning

Or simply:

var builder = WebApplication.CreateBuilder(args);

var app = builder.Build();

static void UseEndpoint<TEndpointInput>(WebApplication app) where TEndpointInput : class
{
    app.MapPost("/test", (TEndpointInput data) => Results.Ok(data));
}

UseEndpoint<DummyEndpointInput>(app);

app.Run();

public class DummyEndpointInput
{
}

Exceptions (if any)

No response

.NET Version

8.0.303

Anything else?

No response

captainsafia commented 3 months ago

@madskonradsen Thanks for reporting this issue! I resolved the stack trace for this and it looks like the DisallowNonParsableComplexTypesOnParameters analyzer is throwing this particular error.

at Microsoft.AspNetCore.Analyzers.RouteHandlers.RouteHandlerAnalyzer.<DisallowNonParsableComplexTypesOnParameters>g__ResovleParameterTypeSymbol|8_2(IParameterSymbol parameterSymbol)

I suspect the issue is in this chunk of code, which doesn't handle the fact that the type associated with TEndpointInput might not have passed the INamedTypeSymbol check.

https://github.com/dotnet/aspnetcore/blob/e31445e897e416b743f436b27cb81f3c87c56d27/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/DisallowNonParsableComplexTypesOnParameters.cs#L125-L131

Would you be interested in opening a PR with a fix for this? I'm happy to provide guidance on how to test/fix this.

madskonradsen commented 3 months ago

Sure! Seems like a nice small task :)

captainsafia commented 3 months ago

@madskonradsen Great! You can check out the build from source document for instructions on how to set up the repo for local development. You might run into issues here since our repo does unique things (relies on a nightly build of the .NET SDK, builds IIS if you're on Windows, etc) but I can hopefully get it working with any issues that you run into here.

After that, you can copy the code from your repro into a new test case in this file and that should get you setup to be able to debug and apply a fix.

madskonradsen commented 3 months ago

@captainsafia Sooo getting the environment up and running was pretty breezy. Roslyn analyzers on the other hand. But they are still fun to work with :) I feel like I've gone in the wrong direction: https://github.com/dotnet/aspnetcore/compare/main...madskonradsen:aspnetcore:mads/56831

I would have expected methodSymbol.TypeParameters to not be empty, but seems like I'm missing something. Any pointers? :)

captainsafia commented 3 months ago

@madskonradsen Yes, I believe the issue here is the fact that methodSymbol actually refers to the lambda expression based as an argument to the MapPost call ((TEndpointInput data) => Results.Ok(data)) which has no type parameters itself but has a parameter that is a type parameter.

I believe what you might actually want to do here is iterate through methodSymbol.Parameters and check if the SymbolKind associated with each parameters is a TypeParameter.

KennethHoff commented 3 months ago

This has annoyed me more-or-less ever since .Net 8 launched, but never understood what causes it to happen nor found any issues on it, so I just ignored the warning and went on with my life. Good to see it's not just me experiencing this. Generic handlers would be a logical denominator. Quite niche after all.

madskonradsen commented 3 months ago

Take 2: https://github.com/dotnet/aspnetcore/compare/main...madskonradsen:aspnetcore:mads/56831-2

This is not finished, but it just feels like the wrong direction to go in, but based on the current API, I'm having a hard time seeing other ways around it, and how I should be able to achieve it by just looking at the Parameters. The code above passes 2/3 tests so far. But I just can't shake the feeling that there must be a better way.

@captainsafia

jofford commented 1 week ago

Any progress on this? The warning still exists in dotnet cli 9.0.100. It doesn't seem to appear when building in Visual Studio 17.12 though.

madskonradsen commented 1 week ago

I haven't had much luck with the patch apart from the code above, and haven't heard any feedback from the dotnet team.