Add analyzer for SemanticModel.GetDeclaredSymbol #6779

CollinAlpert · 2023-07-17T20:06:12Z

Adds an analyzer to detect usages of SemanticModel.GetDeclaredSymbol which always returns null.

Fixes #336.

mavasani · 2023-07-18T04:16:17Z

@dotnet/roslyn-compiler for help in reviewing this analyzer. Couple of questions:

Can we come up with an exhaustive list of syntax node types instead of just the three listed in the issue + implemented in the PR?
We should only include the syntax node types where the compiler team has good confidence that the GetDeclaredSymbol API semantics are unlikely to change in future to return a non-null value.

Youssef1313 · 2023-07-25T09:18:07Z

...lyzers/CSharp/MetaAnalyzers/CSharpSemanticModelGetDeclaredSymbolAlwaysReturnsNullAnalyzer.cs

+        {
+            context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+            context.EnableConcurrentExecution();
+            context.RegisterCompilationStartAction(static compilationContext =>


I'd rename compilationContext to context (intentionally shadowing the outer context)

Youssef1313 · 2023-07-25T09:19:46Z

...zers/UnitTests/MetaAnalyzers/SemanticModelGetDeclaredSymbolAlwaysReturnsNullAnalyzerTests.cs

+
+public class Test {{
+    public void M(SemanticModel semanticModel, {type} syntax) {{
+        var x = [|semanticModel.GetDeclaredSymbol(syntax)|];


Consider adding a test with semanticModel.GetDeclaredSymbol() (which will produce a compilation error)

codecov · 2023-07-25T19:33:02Z

Codecov Report

Merging #6779 (da0b5d4) into main (b4ed6a3) will decrease coverage by 0.01%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##             main    #6779      +/-   ##
==========================================
- Coverage   96.43%   96.42%   -0.01%     
==========================================
  Files        1410     1412       +2     
  Lines      336268   336372     +104     
  Branches    11107    11113       +6     
==========================================
+ Hits       324268   324360      +92     
- Misses       8896     8905       +9     
- Partials     3104     3107       +3

Youssef1313 · 2023-07-28T14:32:10Z

...lyzers/CSharp/MetaAnalyzers/CSharpSemanticModelGetDeclaredSymbolAlwaysReturnsNullAnalyzer.cs

+            if (SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, getDeclaredSymbolMethod))
+            {
+                var syntaxNodeType = invocation.Arguments[1].Value.WalkDownConversion().Type;
+                if (syntaxNodeType is not null && syntaxNodeType.GetBaseTypesAndThis().ToSet().Overlaps(new[] { globalStatementSymbol, incompleteMemberSymbol, baseFieldDeclarationSymbol }))


This allocates a new array for every invocation of GetDeclaredSymbol. I think this is not necessary.

Also note there is already a DerivesFrom extension method.

src/Microsoft.CodeAnalysis.Analyzers/Core/CodeAnalysisDiagnosticsResources.resx

333fred · 2023-08-21T19:01:28Z

src/Microsoft.CodeAnalysis.Analyzers/Core/CodeAnalysisDiagnosticsResources.resx

@@ -571,4 +571,13 @@
  <data name="DoNotRegisterCompilerTypesWithBadAssemblyReferenceRuleTitle" xml:space="preserve">
    <value>Compiler extensions should be implemented in assemblies with compiler-provided references</value>
  </data>
+  <data name="SemanticModelGetDeclaredSymbolAlwaysReturnsNullDescription" xml:space="preserve">
+    <value>Calling 'SemanticModel.GetDeclaredSymbol' with an argument of type 'GlobalStatementSyntax', 'IncompleteMemberSyntax' or a type inheriting from 'BaseFieldDeclarationSyntax' will always return 'null'.</value>


It feels like fields should get a specific error message that instructs the user to extract the variable declarators.

Seems like this description needs adjustment as fields now have a separate descriptor/description. Additionally, not sure if we should limit the description to just 'GlobalStatementSyntax' and 'IncompleteMemberSyntax' as the code handles all sub-types of SyntaxNode that do not have an overload. It's fine to mentioned these two as examples, but not as the only node types being handled.

333fred · 2023-08-21T19:07:19Z

...lyzers/CSharp/MetaAnalyzers/CSharpSemanticModelGetDeclaredSymbolAlwaysReturnsNullAnalyzer.cs

+                    || (getDeclaredSymbolMethod = modelExtensions.GetMembers(nameof(ModelExtensions.GetDeclaredSymbol)).FirstOrDefault() as IMethodSymbol) is null
+                    || !typeProvider.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.MicrosoftCodeAnalysisCSharpSyntaxGlobalStatementSyntax, out var globalStatementSymbol)
+                    || !typeProvider.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.MicrosoftCodeAnalysisCSharpSyntaxIncompleteMemberSyntax, out var incompleteMemberSymbol)
+                    || !typeProvider.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.MicrosoftCodeAnalysisCSharpSyntaxBaseFieldDeclarationSyntax, out var baseFieldDeclarationSymbol))


I wonder if we could instead look for all the extension GetDeclaredSymbol methods, and use that a list of types that will return positive results? With maybe a special case for the field decl scenario, since that's somewhat misleading?

If we don't want to try doing that, I think we should at least include VariableDeclarationSyntax, as that is also a common error.

Thanks for the suggestion, I have implemented it.

333fred

LGTM. @mavasani, can you take a look?

mavasani · 2023-10-25T04:31:20Z

...lyzers/CSharp/MetaAnalyzers/CSharpSemanticModelGetDeclaredSymbolAlwaysReturnsNullAnalyzer.cs

+            var invocation = (IInvocationOperation)context.Operation;
+            if (SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, getDeclaredSymbolMethod))
+            {
+                var syntaxNodeDerivingType = invocation.Arguments[1].Value.WalkDownConversion().Type;


Where do we ensure that getDeclaredSymbolMethod has at least 2 parameters?

This would also not work as expected if named arguments are used and arguments are out of order. You probably want to use one of the below helper methods:

roslyn-analyzers/src/Utilities/Compiler/Extensions/IOperationExtensions.cs

Line 850 in b4ed6a3

public static bool TryGetArgumentForParameterAtIndex(

roslyn-analyzers/src/Utilities/Compiler/Extensions/IOperationExtensions.cs

Line 871 in b4ed6a3

public static IArgumentOperation GetArgumentForParameterAtIndex(

mavasani · 2023-10-25T04:38:55Z

...lyzers/CSharp/MetaAnalyzers/CSharpSemanticModelGetDeclaredSymbolAlwaysReturnsNullAnalyzer.cs

+                Diagnostic? diagnostic = null;
+                if (syntaxNodeDerivingType.DerivesFrom(baseFieldDeclarationType))
+                {
+                    diagnostic = invocation.CreateDiagnostic(FieldDiagnosticDescriptor, syntaxNodeDerivingType.Name);
+                }
+                else if (allowedTypes.All(type => !syntaxNodeDerivingType.DerivesFrom(type, baseTypesOnly: true, checkTypeParameterConstraints: false)))
+                {
+                    diagnostic = invocation.CreateDiagnostic(DiagnosticDescriptor, syntaxNodeDerivingType.Name);
+                }
+
+                if (diagnostic is not null)
+                {
+                    context.ReportDiagnostic(diagnostic);
+                }


Nit:

Suggested change

Diagnostic? diagnostic = null;

if (syntaxNodeDerivingType.DerivesFrom(baseFieldDeclarationType))

{

diagnostic = invocation.CreateDiagnostic(FieldDiagnosticDescriptor, syntaxNodeDerivingType.Name);

}

else if (allowedTypes.All(type => !syntaxNodeDerivingType.DerivesFrom(type, baseTypesOnly: true, checkTypeParameterConstraints: false)))

{

diagnostic = invocation.CreateDiagnostic(DiagnosticDescriptor, syntaxNodeDerivingType.Name);

}

if (diagnostic is not null)

{

context.ReportDiagnostic(diagnostic);

}

DiagnosticDescriptor descriptor;

if (syntaxNodeDerivingType.DerivesFrom(baseFieldDeclarationType))

{

descriptor = FieldDiagnosticDescriptor;

}

else if (allowedTypes.All(type => !syntaxNodeDerivingType.DerivesFrom(type, baseTypesOnly: true, checkTypeParameterConstraints: false)))

{

descriptor = DiagnosticDescriptor;

}

else

{

return;

}

context.ReportDiagnostic(invocation.CreateDiagnostic(descriptor, syntaxNodeDerivingType.Name));

mavasani · 2023-10-25T04:40:17Z

...lyzers/CSharp/MetaAnalyzers/CSharpSemanticModelGetDeclaredSymbolAlwaysReturnsNullAnalyzer.cs

+            }
+        }
+
+        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(DiagnosticDescriptor, FieldDiagnosticDescriptor);


Move this to the top of the file above Initialize method

mavasani

Overall LGTM. Have a few minor suggestions, I can merge once these are addressed. Thanks for the PR @CollinAlpert!

CollinAlpert · 2023-10-25T15:52:43Z

Done!

Add analyzer for SemanticModel.GetDeclaredSymbol

3fe1d7c

CollinAlpert requested a review from a team as a code owner July 17, 2023 20:06

github-actions bot added the Community label Jul 17, 2023

Youssef1313 reviewed Jul 25, 2023

View reviewed changes

CollinAlpert added 3 commits July 25, 2023 18:48

PR comments.

ae44cc9

Merge branch 'master' into issue_336

20fa602

Update RulesMissingDocumentation.md

de68e84

CollinAlpert added 2 commits July 28, 2023 16:20

Merge branch 'master' into issue_336

efe5f87

Merge branch 'master' into issue_336

46b50dd

Youssef1313 reviewed Jul 31, 2023

View reviewed changes

CollinAlpert added 2 commits August 1, 2023 10:41

PR comments

40e64c0

Merge branch 'master' into issue_336

5354e2b

333fred reviewed Aug 21, 2023

View reviewed changes

CollinAlpert added 6 commits August 22, 2023 19:57

Merge branch 'master' into issue_336

03afce0

Merge branch 'master' into issue_336

3180f1d

Source types from GetDeclaredSymbol extension methods

f34878e

Merge branch 'master' into issue_336

2c13b86

Apply PR comments.

ea2bf51

Add newline

5b1a52b

333fred approved these changes Oct 23, 2023

View reviewed changes

333fred requested a review from mavasani October 23, 2023 23:59

mavasani reviewed Oct 25, 2023

View reviewed changes

mavasani approved these changes Oct 25, 2023

View reviewed changes

CollinAlpert added 2 commits October 25, 2023 17:43

Merge branch 'master' into issue_336

7d39a9b

Apply PR comments

71f0540

Fix analyzer description.

da0b5d4

mavasani merged commit b40db00 into dotnet:main Oct 30, 2023
11 checks passed

CollinAlpert deleted the issue_336 branch October 30, 2023 08:53

333fred mentioned this pull request Nov 30, 2023

Make GetDeclaredSymbol more strongly typed for LocalFunctionDeclarationSyntax dotnet/roslyn#71034

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add analyzer for SemanticModel.GetDeclaredSymbol #6779

Add analyzer for SemanticModel.GetDeclaredSymbol #6779

CollinAlpert commented Jul 17, 2023

mavasani commented Jul 18, 2023

Youssef1313 Jul 25, 2023

Youssef1313 Jul 25, 2023

codecov bot commented Jul 25, 2023 •

edited

Youssef1313 Jul 28, 2023

333fred Aug 21, 2023

mavasani Oct 25, 2023

333fred Aug 21, 2023

CollinAlpert Oct 16, 2023

333fred left a comment

mavasani Oct 25, 2023

mavasani Oct 25, 2023 •

edited

mavasani Oct 25, 2023

mavasani Oct 25, 2023

mavasani left a comment

CollinAlpert commented Oct 25, 2023

Add analyzer for SemanticModel.GetDeclaredSymbol #6779

Add analyzer for SemanticModel.GetDeclaredSymbol #6779

Conversation

CollinAlpert commented Jul 17, 2023

mavasani commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 25, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani Oct 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

CollinAlpert commented Oct 25, 2023

codecov bot commented Jul 25, 2023 •

edited

mavasani Oct 25, 2023 •

edited