Add DynamicallyAccessedMembers analyzer #2038
Conversation
| internal const string DynamicallyAccessedMembersAttribute = nameof (DynamicallyAccessedMembersAttribute); | ||
|
|
||
| #region SupportedDiagnostics | ||
| public const string IL2067 = "IL2067"; |
There was a problem hiding this comment.
We should give this identifiers real names so we can easily differentiate in code.
| switch ((sourceSymbol, targetSymbol)) { | ||
| case (IParameterSymbol sourceParameter, IParameterSymbol targetParameter): | ||
| context.ReportDiagnostic (Diagnostic.Create ( | ||
| s_IL2067, |
There was a problem hiding this comment.
Is the only difference in many of these diagnostics that the source is one type of symbol and the target is another type of symbol?
This seems like a difficult design. As a user this would mean that if I want to silence all the conversion warnings in a method I would have to individually silence each warning code, but I don't see what value I get from having a specific warning code for each one.
These all feel like instances of the same warning.
There was a problem hiding this comment.
At minimum, we should try to extract some of this code into a helper, but really I think we should try to figure out why we need this many warning codes.
There was a problem hiding this comment.
Ah I'm looking at the messages now -- some of them are significantly different. We could probably share more but I see why it's not obvious how to do so. Probably a good investment though.
There was a problem hiding this comment.
We have a helper in linker to produce the right warning.
The many different warning codes were introduce actually according to your (@agocke) guidance that each unique message should have its own warning code. It's true that silencing them all is tricky, but I see that as a benefit (you should not want to do that manually, have the SuppressTrimAnalysisWarnings for it) - the simplest way to silence them in code is to add RequiresUnreferencedCode onto the method which produces them.
| { | ||
| context.EnableConcurrentExecution (); | ||
| context.ConfigureGeneratedCodeAnalysis (GeneratedCodeAnalysisFlags.ReportDiagnostics); | ||
| context.RegisterSyntaxNodeAction (DynamicallyAccessedMembersAnalyze, |
There was a problem hiding this comment.
I think an IOperation context makes more sense here -- syntax seems hard because you may have a lot of different syntaxes for the same semantic operation.
| } | ||
|
|
||
| // For all parameters in the called method, check if the passed arguments have matching annotations. | ||
| for (int i = 0; i < invokedMethodSymbol.Parameters.Length; i++) { |
There was a problem hiding this comment.
I think what you want to use here is an IInvocationOperation, because then you can loop through the IArgumentOperations. Otherwise you'll be stuck trying to match up arguments with parameters, which is just insanely difficult given things like named arguments, optional parameters, params arguments, etc.
| static readonly DiagnosticDescriptor s_IL2067 = new ( | ||
| IL2067, |
There was a problem hiding this comment.
Rant: This feels like a great opportunity for some kind of source generator - having to write this much code is crazy.
For now - can we refactor this a bit - create a helper which only takes the parts which are different and call that instead of repeating the same parameters over and over.
| switch ((sourceSymbol, targetSymbol)) { | ||
| case (IParameterSymbol sourceParameter, IParameterSymbol targetParameter): | ||
| context.ReportDiagnostic (Diagnostic.Create ( | ||
| s_IL2067, |
There was a problem hiding this comment.
We have a helper in linker to produce the right warning.
The many different warning codes were introduce actually according to your (@agocke) guidance that each unique message should have its own warning code. It's true that silencing them all is tricky, but I see that as a benefit (you should not want to do that manually, have the SuppressTrimAnalysisWarnings for it) - the simplest way to silence them in code is to add RequiresUnreferencedCode onto the method which produces them.
This PR adds basic support for
DynamicallyAccessedMembersin the linker's Roslyn analyzer. It focuses on the diagnostics concerning unmatched member annotations (warnings IL2067 to IL2091). The idea is to have this merged into a feature branch that can be merged with master once the analyzer is in a good shape.UnrecognizedReflectionAccessPatternin the analyzer tests infra.