Skip to content

Commit

Permalink
Add FilterTree/FilterSpan API on SymbolStartAnalysisContext and Symbo…
Browse files Browse the repository at this point in the history
…lAnalysisContext

Work towards dotnet#68033

- Add FilterTree/FilterSpan API on SymbolStartAnalysisContext and SymbolAnalysisContext
- Move IDE code style SymbolStart analyzers to respect the FilterTree/FilterSpan API
  • Loading branch information
mavasani committed May 2, 2023
1 parent 91fc1ac commit eb371ec
Show file tree
Hide file tree
Showing 25 changed files with 551 additions and 152 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,22 @@ public static void CreateAndRegisterActions(CompilationStartAnalysisContext cont
if (namedTypeSymbol.TypeKind != TypeKind.Struct)
return;
//We check if struct contains any 'readonly' fields
// We check if struct contains any 'readonly' fields
if (!HasReadonlyField(namedTypeSymbol))
return;
// Check if diagnostic location is within the analysis span
if (!context.ShouldAnalyzeLocation(GetDiagnosticLocation(namedTypeSymbol)))
return;
var symbolAnalyzer = new SymbolAnalyzer(namedTypeSymbol);
symbolAnalyzer.RegisterActions(context);
}, SymbolKind.NamedType);
}

private static Location GetDiagnosticLocation(INamedTypeSymbol namedTypeSymbol)
=> namedTypeSymbol.Locations[0];

private static bool HasReadonlyField(INamedTypeSymbol namedTypeSymbol)
{
return namedTypeSymbol
Expand Down Expand Up @@ -105,7 +112,7 @@ private void SymbolEndAction(SymbolAnalysisContext context)
{
var diagnostic = Diagnostic.Create(
s_diagnosticDescriptor,
_namedTypeSymbol.Locations[0]);
GetDiagnosticLocation(_namedTypeSymbol));
context.ReportDiagnostic(diagnostic);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using System.Threading;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
Expand Down Expand Up @@ -38,6 +39,15 @@ protected override void InitializeWorker(AnalysisContext context)
return;
context.RegisterSymbolStartAction(context =>
{
if (!ShouldAnalyze(context, out var option))
return;
context.RegisterOperationBlockAction(
context => AnalyzeBlock(context, option.Notification.Severity));
}, SymbolKind.NamedType);
static bool ShouldAnalyze(SymbolStartAnalysisContext context, [NotNullWhen(true)] out CodeStyleOption2<bool>? option)
{
// Only run on non-readonly structs. If the struct is already readonly, no need to make the members readonly.
if (context.Symbol is not INamedTypeSymbol
Expand All @@ -47,19 +57,38 @@ protected override void InitializeWorker(AnalysisContext context)
DeclaringSyntaxReferences: [var reference, ..],
} structType)
{
return;
option = null;
return false;
}
var cancellationToken = context.CancellationToken;
var declaration = reference.GetSyntax(cancellationToken);
var options = context.GetCSharpAnalyzerOptions(declaration.SyntaxTree);
var option = options.PreferReadOnlyStructMember;
option = options.PreferReadOnlyStructMember;
if (!option.Value)
return;
return false;
context.RegisterOperationBlockAction(
context => AnalyzeBlock(context, option.Notification.Severity));
}, SymbolKind.NamedType);
// Skip analysis if the analysis filter span does not contain the primary location where we would report a diagnostic.
if (context.FilterSpan.HasValue)
{
Contract.ThrowIfNull(context.FilterTree);
var shouldAnalyze = false;
foreach (var member in structType.GetMembers())
{
var location = GetDiagnosticLocation(member, cancellationToken, out _);
if (location != null && context.ShouldAnalyzeLocation(location))
{
shouldAnalyze = true;
break;
}
}
if (!shouldAnalyze)
return false;
}
return true;
}
});

private void AnalyzeBlock(
Expand All @@ -68,18 +97,9 @@ private void AnalyzeBlock(
{
var cancellationToken = context.CancellationToken;

// if it's not a method, or it's already readonly, nothing to do.
if (context.OwningSymbol is not IMethodSymbol
{
MethodKind: MethodKind.Ordinary or MethodKind.ExplicitInterfaceImplementation or MethodKind.PropertyGet or MethodKind.PropertySet,
IsReadOnly: false,
IsStatic: false,
IsImplicitlyDeclared: false,
DeclaringSyntaxReferences: [var methodReference, ..],
} owningMethod)
{
var location = GetDiagnosticLocation(context.OwningSymbol, cancellationToken, out var additionalLocation);
if (location == null)
return;
}

foreach (var blockOperation in context.OperationBlocks)
{
Expand All @@ -89,10 +109,38 @@ private void AnalyzeBlock(
if (blockOperation is IBlockOperation { Operations: [IThrowOperation or IExpressionStatementOperation { Operation: IThrowOperation }] })
return;

if (BlockOperationPotentiallyMutatesThis(owningMethod, blockOperation, cancellationToken))
if (BlockOperationPotentiallyMutatesThis((IMethodSymbol)context.OwningSymbol, blockOperation, cancellationToken))
return;
}

context.ReportDiagnostic(DiagnosticHelper.Create(
Descriptor,
location,
severity,
additionalLocations: ImmutableArray.Create(additionalLocation),
properties: null));
}

private static Location? GetDiagnosticLocation(
ISymbol symbol,
CancellationToken cancellationToken,
[NotNullWhen(true)] out Location? additionalLocation)
{
additionalLocation = null;

// if it's not a method, or it's already readonly, nothing to do.
if (symbol is not IMethodSymbol
{
MethodKind: MethodKind.Ordinary or MethodKind.ExplicitInterfaceImplementation or MethodKind.PropertyGet or MethodKind.PropertySet,
IsReadOnly: false,
IsStatic: false,
IsImplicitlyDeclared: false,
DeclaringSyntaxReferences: [var methodReference, ..],
} owningMethod)
{
return null;
}

var declaration = methodReference.GetSyntax(cancellationToken);

var nameToken = declaration switch
Expand All @@ -109,14 +157,10 @@ private void AnalyzeBlock(
declaration = declaration.GetRequiredParent();

if (nameToken is null)
return;
return null;

context.ReportDiagnostic(DiagnosticHelper.Create(
Descriptor,
nameToken.Value.GetLocation(),
severity,
additionalLocations: ImmutableArray.Create(declaration.GetLocation()),
properties: null));
additionalLocation = declaration.GetLocation();
return nameToken.Value.GetLocation();
}

private static bool BlockOperationPotentiallyMutatesThis(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ protected override void InitializeWorker(AnalysisContext context)
context.RegisterSymbolStartAction(context =>
{
// First, see if this at least strongly looks like a struct that could be converted.
if (!IsCandidate(context, out var typeDeclaration, out var option))
if (!IsCandidate(context, out var location, out var additionalLocation, out var option))
return;
// Looks good. However, we have to make sure that the struct has no code which actually overwrites 'this'
Expand All @@ -60,23 +60,25 @@ protected override void InitializeWorker(AnalysisContext context)
context.ReportDiagnostic(DiagnosticHelper.Create(
Descriptor,
typeDeclaration.Identifier.GetLocation(),
location,
option.Notification.Severity,
additionalLocations: ImmutableArray.Create(typeDeclaration.GetLocation()),
additionalLocations: ImmutableArray.Create(additionalLocation),
properties: null));
});
}, SymbolKind.NamedType);
});

private static bool IsCandidate(
SymbolStartAnalysisContext context,
[NotNullWhen(true)] out TypeDeclarationSyntax? typeDeclaration,
[NotNullWhen(true)] out Location? primaryLocation,
[NotNullWhen(true)] out Location? additionalLocation,
[NotNullWhen(true)] out CodeStyleOption2<bool>? option)
{
var typeSymbol = (INamedTypeSymbol)context.Symbol;
var cancellationToken = context.CancellationToken;

typeDeclaration = null;
primaryLocation = null;
additionalLocation = null;
option = null;
if (typeSymbol.TypeKind is not TypeKind.Struct)
return false;
Expand All @@ -87,7 +89,7 @@ private static bool IsCandidate(
if (typeSymbol.DeclaringSyntaxReferences.Length == 0)
return false;

typeDeclaration = typeSymbol.DeclaringSyntaxReferences[0].GetSyntax(cancellationToken) as TypeDeclarationSyntax;
var typeDeclaration = typeSymbol.DeclaringSyntaxReferences[0].GetSyntax(cancellationToken) as TypeDeclarationSyntax;
if (typeDeclaration is null)
return false;

Expand All @@ -111,6 +113,12 @@ private static bool IsCandidate(
if (!hasField)
return false;

// Check if the primary location for the diagnostic is part of the analysis span.
primaryLocation = typeDeclaration.Identifier.GetLocation();
if (!context.ShouldAnalyzeLocation(primaryLocation))
return false;

additionalLocation = typeDeclaration.GetLocation();
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.Extensions;
using Microsoft.CodeAnalysis.CSharp.LanguageService;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.LanguageService;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.CodeAnalysis.RemoveUnusedParametersAndValues;

Expand All @@ -22,6 +24,8 @@ public CSharpRemoveUnusedParametersAndValuesDiagnosticAnalyzer()
{
}

protected override ISyntaxFacts SyntaxFacts => CSharpSyntaxFacts.Instance;

protected override bool SupportsDiscard(SyntaxTree tree)
=> tree.Options.LanguageVersion() >= LanguageVersion.CSharp7;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ protected override void InitializeWorker(AnalysisContext context)
context.RegisterSymbolStartAction(context =>
{
if (!ShouldAnalyze(context))
return;
context.RegisterOperationAction(AnalyzeOperation, OperationKind.FieldReference);
// Can't allow changing the fields to readonly if the struct overwrites itself. e.g. `this = default;`
Expand All @@ -86,7 +89,11 @@ protected override void InitializeWorker(AnalysisContext context)
// Local functions.
void AnalyzeFieldSymbol(SymbolAnalysisContext symbolContext)
{
_ = TryGetOrInitializeFieldState((IFieldSymbol)symbolContext.Symbol, symbolContext.Options, symbolContext.CancellationToken);
var field = (IFieldSymbol)symbolContext.Symbol;
if (!symbolContext.ShouldAnalyzeLocation(GetDiagnosticLocation(field)))
return;
_ = TryGetOrInitializeFieldState(field, symbolContext.Options, symbolContext.CancellationToken);
}
void AnalyzeOperation(OperationAnalysisContext operationContext)
Expand Down Expand Up @@ -119,7 +126,7 @@ void OnSymbolEnd(SymbolAnalysisContext symbolEndContext)
var option = GetCodeStyleOption(field, symbolEndContext.Options);
var diagnostic = DiagnosticHelper.Create(
Descriptor,
field.Locations[0],
GetDiagnosticLocation(field),
option.Notification.Severity,
additionalLocations: null,
properties: null);
Expand All @@ -129,6 +136,22 @@ void OnSymbolEnd(SymbolAnalysisContext symbolEndContext)
}
}
bool ShouldAnalyze(SymbolStartAnalysisContext context)
{
var members = ((INamedTypeSymbol)context.Symbol).GetMembers();
foreach (var member in members)
{
if (member is IFieldSymbol field
&& IsCandidateField(field, threadStaticAttribute, dataContractAttribute, dataMemberAttribute)
&& context.ShouldAnalyzeLocation(GetDiagnosticLocation(field)))
{
return true;
}
}
return false;
}
static bool IsCandidateField(IFieldSymbol symbol, INamedTypeSymbol? threadStaticAttribute, INamedTypeSymbol? dataContractAttribute, INamedTypeSymbol? dataMemberAttribute)
=> symbol is
{
Expand Down Expand Up @@ -202,6 +225,9 @@ void UpdateFieldStateOnWrite(IFieldSymbol field)
});
}

private static Location GetDiagnosticLocation(IFieldSymbol field)
=> field.Locations[0];

private static bool IsFieldWrite(IFieldReferenceOperation fieldReference, ISymbol owningSymbol)
{
// Check if the underlying member is being written or a writable reference to the member is taken.
Expand Down Expand Up @@ -259,6 +285,6 @@ private static bool IsFieldWrite(IFieldReferenceOperation fieldReference, ISymbo
}

private static CodeStyleOption2<bool> GetCodeStyleOption(IFieldSymbol field, AnalyzerOptions options)
=> options.GetAnalyzerOptions(field.Locations[0].SourceTree!).PreferReadonly;
=> options.GetAnalyzerOptions(GetDiagnosticLocation(field).SourceTree!).PreferReadonly;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ private CompilationAnalyzer(
_attributeSetForMethodsToIgnore = ImmutableHashSet.CreateRange(GetAttributesForMethodsToIgnore(compilation));
}

private static Location GetDiagnosticLocation(ISymbol symbol)
=> symbol.Locations[0];

private static IEnumerable<INamedTypeSymbol> GetAttributesForMethodsToIgnore(Compilation compilation)
{
// Ignore methods with special serialization attributes, which are invoked by the runtime
Expand Down Expand Up @@ -181,6 +184,9 @@ private void RegisterActions(CompilationStartAnalysisContext compilationStartCon
Action<ISymbol, ValueUsageInfo> onSymbolUsageFound = OnSymbolUsage;
compilationStartContext.RegisterSymbolStartAction(symbolStartContext =>
{
if (!ShouldAnalyze(symbolStartContext))
return;
var hasUnsupportedOperation = false;
symbolStartContext.RegisterOperationAction(AnalyzeMemberReferenceOperation, OperationKind.FieldReference, OperationKind.MethodReference, OperationKind.PropertyReference, OperationKind.EventReference);
symbolStartContext.RegisterOperationAction(AnalyzeFieldInitializer, OperationKind.FieldInitializer);
Expand All @@ -202,12 +208,28 @@ private void RegisterActions(CompilationStartAnalysisContext compilationStartCon
// Register custom language-specific actions, if any.
_analyzer.HandleNamedTypeSymbolStart(symbolStartContext, onSymbolUsageFound);
}, SymbolKind.NamedType);

bool ShouldAnalyze(SymbolStartAnalysisContext context)
{
var members = ((INamedTypeSymbol)context.Symbol).GetMembers();
foreach (var member in members)
{
if (IsCandidateSymbol(member.OriginalDefinition)
&& context.ShouldAnalyzeLocation(GetDiagnosticLocation(member)))
{
return true;
}
}

return false;
}
}

private void AnalyzeSymbolDeclaration(SymbolAnalysisContext symbolContext)
{
var symbol = symbolContext.Symbol.OriginalDefinition;
if (IsCandidateSymbol(symbol))
if (IsCandidateSymbol(symbol)
&& symbolContext.ShouldAnalyzeLocation(GetDiagnosticLocation(symbol)))
{
lock (_gate)
{
Expand Down Expand Up @@ -474,7 +496,7 @@ member is IPropertySymbol property &&
// We report the diagnostic on the first location of the member.
var diagnostic = DiagnosticHelper.CreateWithMessage(
rule,
member.Locations[0],
GetDiagnosticLocation(member),
rule.GetEffectiveSeverity(symbolEndContext.Compilation.Options),
additionalLocations: null,
properties: null,
Expand Down
Loading

0 comments on commit eb371ec

Please sign in to comment.