Skip to content

Commit

Permalink
optimize/fix DefaultValueConsistencyAnalyzer (#933)
Browse files Browse the repository at this point in the history
  • Loading branch information
@omsmith
omsmith authored Nov 20, 2023
1 parent 983f9b8 commit 8587e9d
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 111 deletions.
197 changes: 86 additions & 111 deletions src/D2L.CodeStyle.Analyzers/Language/DefaultValueConsistencyAnalyzer.cs
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
#nullable disable
#nullable enable

using System.Collections.Immutable;
using D2L.CodeStyle.Analyzers.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using System;
using System.Collections.Immutable;
using System.Linq;

namespace D2L.CodeStyle.Analyzers.Language {
[DiagnosticAnalyzer( LanguageNames.CSharp )]
Expand All @@ -22,134 +19,98 @@ public override void Initialize( AnalysisContext context ) {
context.EnableConcurrentExecution();
context.ConfigureGeneratedCodeAnalysis( GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics );

// Check for methods with the "override" modifier
context.RegisterSyntaxNodeAction(
AnalyzeOverrideMethodDeclaration,
SyntaxKind.MethodDeclaration
);

// We also need to check interface method impls, and unfortunately
// this is the best way I know how to do it:
context.RegisterSyntaxNodeAction(
AnalyzeBaseList,
SyntaxKind.BaseList
context.RegisterSymbolAction(
static ctx => AnalyzeNamedType( ctx, (INamedTypeSymbol)ctx.Symbol ),
SymbolKind.NamedType
);
}

private static void AnalyzeOverrideMethodDeclaration(
SyntaxNodeAnalysisContext ctx
private static void AnalyzeNamedType(
SymbolAnalysisContext ctx,
INamedTypeSymbol type
) {
var methodDecl = (MethodDeclarationSyntax)ctx.Node;

var hostDecl = methodDecl.Parent as TypeDeclarationSyntax;

// The parser may create MethodDeclaration nodes in weird places
// (e.g. as a member of a NamespaceDeclaration) just to continue
// on parsing and generating diagnostics. The only sensible places
// are TypeDeclarationSyntaxs (class, struct or interface decls)
// so just bail out if we're in a weird situation.
if ( hostDecl == null ) {
return;
}

// As a performance optimization we can bail out early if there
// are no overrides we care about (e.g. our base class is either
// System.Object or System.ValueType and we don't implement any
// interfaces.) This saves querying the semantic model.
if ( hostDecl.BaseList == null ) {
return;
}

var method = ctx.SemanticModel
.GetDeclaredSymbol( methodDecl, ctx.CancellationToken );

if ( method == null || method.Kind == SymbolKind.ErrorType ) {
return;
}

if( !method.IsOverride ) {
return;
}

// There might be other build errors (e.g. from missing partial
// classes from code-gen, or incomplete code when we're running
// in intellisense) preventing the semantic model from locating
// the original method. Ignore these.
if( method.OverriddenMethod == null ) {
return;
}

AnalyzeMethod(
ctx.ReportDiagnostic,
baseMethod: method.OverriddenMethod,
implMethod: method,
ctx.CancellationToken
);
}

private static void AnalyzeBaseList( SyntaxNodeAnalysisContext ctx ) {
var baseList = (BaseListSyntax)ctx.Node;

// ignore enums, their BaseListSyntax isn't relevant
if ( baseList.Parent is EnumDeclarationSyntax ) {
if( type.TypeKind == TypeKind.Interface ) {
return;
}

// Not likely, maybe as the user is typing stuff out.
if ( baseList.Types.Count == 0 ) {
return;
}

var implType = ctx.SemanticModel
.GetDeclaredSymbol( (TypeDeclarationSyntax)baseList.Parent, ctx.CancellationToken );

// The most expensive thing we do:
var interfaceMethodsAndImpls = implType
.AllInterfaces // interfaces can extend interfaces, etc.
.SelectMany( iface => iface.GetMembers() )
.OfType<IMethodSymbol>()
.Select( ifaceMethod =>
(ifaceMethod, implType.FindImplementationForInterfaceMember( ifaceMethod ) as IMethodSymbol )
);
// Check for base class overrides
foreach( ISymbol member in type.GetMembers() ) {
if( member is not IMethodSymbol method ) {
continue;
}

foreach( (var ifaceMethod, var implMethod) in interfaceMethodsAndImpls ) {
if ( implMethod == null ) {
// Maybe they implemented a method with a non-method (which
// would be a different build error) or they haven't
// implemented it yet.
// Not overriding anything on the base type, so ignore
if( method.OverriddenMethod is null ) {
continue;
}

if ( !implMethod.ContainingType.Equals( implType, SymbolEqualityComparer.Default ) ) {
// Our base class could implement the method. We don't want
// to duplicate the work on principle but also when we look
// at DeclaringSyntaxReferences in GetLocation it could
// fail because our base could be in a different assembly.
return;
// Ignore overloads of System.Object methods (such as Equals) that we don't care about
if( SymbolEqualityComparer.Default.Equals( method.OverriddenMethod.ContainingType, ctx.Compilation.ObjectType ) ) {
continue;
}

// This is O(# of explicit implementations for this method), in
// a loop which includes at least that many things, so O(n^2)
// for some n. But n is probably small.
if ( implMethod.ExplicitInterfaceImplementations.Contains( ifaceMethod ) ) {
// Explicit implementations can't change default value
// behaviour, so we don't need to worry about them.
// Ignore overloads of System.ValueType methods (such as Equals) that we don't care about
if( SymbolEqualityComparer.Default.Equals( method.OverriddenMethod.ContainingType, ctx.Compilation.GetSpecialType( SpecialType.System_ValueType ) ) ) {
continue;
}

AnalyzeMethod(
ctx.ReportDiagnostic,
baseMethod: ifaceMethod,
implMethod: implMethod,
baseMethod: method.OverriddenMethod,
implMethod: method,
locationOverride: null,
ctx.CancellationToken
);
}

// Check for interface implementations, which may come from base types
foreach( INamedTypeSymbol @interface in type.AllInterfaces ) {
bool? interfaceIsFromBaseType = null;
foreach( ISymbol member in @interface.GetMembers() ) {
if( member is not IMethodSymbol method ) {
continue;
}

ISymbol? implementingMember = type.FindImplementationForInterfaceMember( method );

// Should always be a method symbol
if( implementingMember is not IMethodSymbol implementingMethod ) {
continue;
}

// Explicit implementations can't change default value
// behaviour, so we don't need to worry about them.
if( !implementingMethod.ExplicitInterfaceImplementations.IsEmpty ) {
continue;
}

bool methodIsImplementedLocally = SymbolEqualityComparer.Default.Equals( type, implementingMethod.ContainingType );
if( !methodIsImplementedLocally ) {

// If the method and interface come from the base type then any diagnostics
// will be raised when analyzing the base type, skip analyzing the method
interfaceIsFromBaseType ??= type.BaseType!.AllInterfaces.Contains( @interface, SymbolEqualityComparer.Default );
if( interfaceIsFromBaseType.Value == true ) {
continue;
}
}

AnalyzeMethod(
ctx.ReportDiagnostic,
baseMethod: method,
implMethod: implementingMethod,
locationOverride: methodIsImplementedLocally ? null : GetTypeNameDiagnosticLocation( ctx, type ),
ctx.CancellationToken
);
}
}
}

private static void AnalyzeMethod(
Action<Diagnostic> reportDiagnostic,
IMethodSymbol baseMethod,
IMethodSymbol implMethod,
Location? locationOverride,
CancellationToken cancellationToken
) {
foreach( var implParameter in implMethod.Parameters ) {
Expand All @@ -175,7 +136,7 @@ CancellationToken cancellationToken
reportDiagnostic(
Diagnostic.Create(
Diagnostics.IncludeDefaultValueInOverrideForReadability,
GetLocation( implParameter, cancellationToken ),
locationOverride ?? GetLocation( implParameter, cancellationToken ),
implParameter.Name,
baseMethod.ContainingType.Name
)
Expand All @@ -190,7 +151,7 @@ CancellationToken cancellationToken
reportDiagnostic(
Diagnostic.Create(
Diagnostics.DontIntroduceNewDefaultValuesInOverrides,
GetLocation( implParameter, cancellationToken ),
locationOverride ?? GetLocation( implParameter, cancellationToken ),
implParameter.Name,
baseMethod.ContainingType.Name
)
Expand All @@ -217,7 +178,7 @@ CancellationToken cancellationToken
reportDiagnostic(
Diagnostic.Create(
Diagnostics.DefaultValuesInOverridesShouldBeConsistent,
GetLocation( implParameter, cancellationToken ),
locationOverride ?? GetLocation( implParameter, cancellationToken ),
implParameter.Name,
FormatDefaultValue( implDefault ),
FormatDefaultValue( baseDefault ),
Expand All @@ -228,7 +189,7 @@ CancellationToken cancellationToken
}
}

private static string FormatDefaultValue( object val ) {
private static string FormatDefaultValue( object? val ) {
if ( val == null ) {
return "null";
}
Expand All @@ -246,5 +207,19 @@ private static Location GetLocation( IParameterSymbol param, CancellationToken c
.GetSyntax( cancellationToken )
.GetLocation();
}

private static Location GetTypeNameDiagnosticLocation( SymbolAnalysisContext ctx, INamedTypeSymbol namedType ) {
var (type, baseType) = namedType.ExpensiveGetSyntaxImplementingType( namedType.BaseType!, ctx.Compilation, ctx.CancellationToken );

if( baseType is not null ) {
return baseType.GetLocation();
}

if( type is not null ) {
return type.Identifier.GetLocation();
}

return Location.None;
}
}
}
20 changes: 20 additions & 0 deletions tests/D2L.CodeStyle.Analyzers.Test/Specs/DefaultValueConsistencyAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,26 @@ public void Hello(
}
}

namespace InterfaceFromBaseClass {
public interface IFoo {
void Fizz( int x = 111 );
void Buzz( int y = 222 );
void FizzBuzz( int s );
}

public class BaseClass {
public void Fizz( int x );
public void Buzz( int y = 333 );
public void FizzBuzz( int s = 444 );
}

public class InterhitsInterfaceImplementation : /*
IncludeDefaultValueInOverrideForReadability(x, IFoo)
| DefaultValuesInOverridesShouldBeConsistent(y, 333, 222, IFoo)
| DontIntroduceNewDefaultValuesInOverrides(s, IFoo)
*/ BaseClass /**/, IFoo { }
}

namespace NewModifier {
interface IBaseInterface {
void GetStuff( int count = 99 );
Expand Down

0 comments on commit 8587e9d

Please sign in to comment.