Emit diagnostics for unsupported RDG scenarios #49417
Conversation
captainsafia
commented
Jul 14, 2023
captainsafia
commented
- Emit warnings for generics captured in parameters or response types (fixes Request Delegate Generator fails to compile code with CS0246 error for endpoint with generic type parameter #49384)
- Emit warnings for private/protected types captured in parameters or response types (fixes RDG does not support private parameter and return types #47339)
- Fix warning for anonymous types in awaitable methods
ghost
added
the
captainsafia
added
feature-rdg
area-minimal
captainsafia
requested review from
Tratcher,
BrennanConroy and
halter73
as code owners
captainsafia
requested review from
mitchdenny and
eerhardt
and removed request for
halter73,
Tratcher,
BrennanConroy and
mitchdenny
src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/DiagnosticEmitter.cs
Show resolved
Hide resolved
| { | ||
| diagnostics.Add(Diagnostic.Create(DiagnosticDescriptors.TypeParametersNotSupported, location)); | ||
| } | ||
| if (response.ResponseType?.DeclaredAccessibility is Accessibility.Private or Accessibility.Protected) |
There was a problem hiding this comment.
| if (response.ResponseType?.DeclaredAccessibility is Accessibility.Private or Accessibility.Protected) | |
| if (response.ResponseType?.DeclaredAccessibility is Accessibility.Private or Accessibility.Protected) |
There was a problem hiding this comment.
Unless these are meant to be else if
There was a problem hiding this comment.
Unless these are meant to be else if
I debated this but I've always like the upfront disclosure you get when all the warnings are emitted at once instead of the progressive disclosure that allows you to keep fixing things until you get it right.
There was a problem hiding this comment.
SemanticModel has an IsAccessible(Location, ISymbol) method. I believe that is the intended way to check for accessibility instead of DeclaredAccessibility In the past, I've used it like this: semanticModel.IsAccessible(0, symbol).
There was a problem hiding this comment.
Have you used this in a generator? It seems like the API works by assessing accessibility within the context of an existing syntax tree, but here we're trying to check accessibility in code we haven't generated yet.
src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/DiagnosticEmitter.cs
Show resolved
Hide resolved
src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Emitters/DiagnosticEmitter.cs
Show resolved
Hide resolved
| var parameter = new EndpointParameter(this, method.Parameters[i], wellKnownTypes); | ||
| var parameterSymbol = method.Parameters[i]; | ||
| parameterSymbol.EmitRequiredDiagnostics(Diagnostics, Operation.Syntax.GetLocation()); | ||
| if (Diagnostics.Count > 0) |
There was a problem hiding this comment.
Seems fragile? What if we emit info diagnostics, or diagnostics from a different part of the generator unrelated to this endpoint?
There was a problem hiding this comment.
What if we emit info diagnostics, or diagnostics from a different part of the generator unrelated to this endpoint?
At the moment, we're strictly using diagnostics as a way to indicate that some aspect of the endpoint is wrong and we can't statically generate it.
If we do decide to start emitting info-level diagnostics (I dunno how meaningful that would be for a source generator given that it's only visible in builds triggered from the IDE), we'd have to consider other aspects of the implementation, such as the way we use the lack of diagnostics to determine if we haven't been able to generate an endpoint.
All in all, I feel safe with the simplicity of .Count > 0 here given the constraints were operating under compared to a more thorough check.
src/Http/Http.Extensions/test/RequestDelegateGenerator/CompileTimeCreationTests.cs
Show resolved
Hide resolved
