WIP Base Analyzer #45595
WIP Base Analyzer #45595
Conversation
| @@ -34,15 +35,15 @@ public CSharpConvertNameOfDiagnosticAnalyzer() | |||
|
|
|||
| protected override void InitializeWorker(AnalysisContext context) | |||
| { | |||
| context.RegisterSyntaxNodeAction(AnalyzeSyntaxAction, SyntaxKind.TypeOfExpression); | |||
| context.RegisterOperationAction(AnalyzeTypeOfAction, OperationKind.TypeOf); | |||
There was a problem hiding this comment.
Nice! I was going to suggest switching to IOperation.
| } | ||
|
|
||
| private void AnalyzeSyntaxAction(SyntaxNodeAnalysisContext syntaxContext) | ||
| private void AnalyzeTypeOfAction(OperationAnalysisContext typeofOp) |
There was a problem hiding this comment.
Nit: We avoid abbreviated names for locals.
| private void AnalyzeTypeOfAction(OperationAnalysisContext typeofOp) | |
| private void AnalyzeTypeOfAction(OperationAnalysisContext context) |
| // We know that it is a typeof() instance, but we only want to offer the fix if it is a .Name access | ||
| var parent = node.Parent; | ||
| if (!(node.Parent.IsKind(SyntaxKind.SimpleMemberAccessExpression) && parent.IsNameMemberAccess())) |
There was a problem hiding this comment.
Can we avoid walking the syntax parent and instead look at IOperation.Parent of the operation node? That should be an IPropertyReferenceOperation with it's property symbol named FullName for us to continue analysis. This will help you avoid calling IsNameMemberAccess as well and should be faster.
| return false; | ||
| } | ||
|
|
||
| private static bool ArgumentIsPrimitive(IOperation op) |
There was a problem hiding this comment.
A better way to check for primitive type is something like: https://github.com/dotnet/roslyn-analyzers/blob/f15404b312295e7cee16fb40b4c8d3f91f10f087/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DoNotLockOnObjectsWithWeakIdentity.cs#L124-L144
Maybe you can invoke the below method on ITypeSymbol.SpecialType and check if the returned predefined type is not None?
|
|
||
| // Current case can be effectively changed to a nameof instance so report a diagnostic | ||
| var location = Location.Create(syntaxTree, parent.Span); | ||
| var additionalLocations = ImmutableArray.Create(node.GetLocation()); | ||
|
|
||
| syntaxContext.ReportDiagnostic(DiagnosticHelper.Create( | ||
| typeofOp.ReportDiagnostic(DiagnosticHelper.Create( | ||
| Descriptor, | ||
| location, | ||
| ReportDiagnostic.Hidden, |
There was a problem hiding this comment.
Unless you plan to add a code style option for this analyzer, you can directly invoke Diagnostic.Create(Descriptor, location) here.
| if (ArgumentIsGeneric(typeofOp.Operation) || ArgumentIsPrimitive(typeofOp.Operation)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Current case can be effectively changed to a nameof instance so report a diagnostic | ||
| var location = Location.Create(syntaxTree, parent.Span); |
There was a problem hiding this comment.
| var location = Location.Create(syntaxTree, parent.Span); | |
| var location = parent.GetLocation(); |
| if (ArgumentIsGeneric(typeofOp.Operation) || ArgumentIsPrimitive(typeofOp.Operation)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Current case can be effectively changed to a nameof instance so report a diagnostic | ||
| var location = Location.Create(syntaxTree, parent.Span); | ||
| var additionalLocations = ImmutableArray.Create(node.GetLocation()); |
There was a problem hiding this comment.
I think you can avoid adding any additionalLocations here.
| var additionalLocations = ImmutableArray.Create(node.GetLocation()); |
This analyzer generally covers all cases as discussed in the specification. It needs to be tested on a more robust set of test cases in order to address additional bugs.