Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

optimize InterfaceBinderAnalyzer #936

Merged
merged 1 commit into from
Nov 20, 2023
Merged

optimize InterfaceBinderAnalyzer #936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 40 additions & 38 deletions src/D2L.CodeStyle.Analyzers/ApiUsage/DataRecordConverters/InterfaceBinderAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
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 Microsoft.CodeAnalysis.Operations;
using static D2L.CodeStyle.Analyzers.Diagnostics;

namespace D2L.CodeStyle.Analyzers.ApiUsage.DataRecordConverters {
Expand All @@ -30,54 +29,57 @@ private void CompilationStart( CompilationStartAnalysisContext context ) {
return;
}

context.RegisterSyntaxNodeAction(
c => AnalyzeGenericName( c, interfaceBinderType, (GenericNameSyntax)c.Node ),
SyntaxKind.GenericName
);
context.RegisterOperationAction(
ctx => AnalyzeMemberReference(
ctx,
interfaceBinderType: interfaceBinderType,
member: ( (IPropertyReferenceOperation)ctx.Operation ).Member
),
OperationKind.PropertyReference
);

context.RegisterOperationAction(
ctx => AnalyzeMemberReference(
ctx,
interfaceBinderType: interfaceBinderType,
member: ( (IInvocationOperation)ctx.Operation ).TargetMethod
),
OperationKind.Invocation
);

context.RegisterOperationAction(
ctx => AnalyzeMemberReference(
ctx,
interfaceBinderType: interfaceBinderType,
member: ( (IMethodReferenceOperation)ctx.Operation ).Member
),
OperationKind.MethodReference
);
}

private static void AnalyzeGenericName(
SyntaxNodeAnalysisContext context,
INamedTypeSymbol interfaceBinderType,
GenericNameSyntax genericName
) {

TypeArgumentListSyntax argumentList = genericName.TypeArgumentList;
if( argumentList.Arguments.Count != 1 ) {
return;
}

ITypeSymbol? genericType = context.SemanticModel
.GetTypeInfo( genericName, context.CancellationToken )
.Type;

if( genericType == null ) {
private static void AnalyzeMemberReference(
OperationAnalysisContext context,
INamedTypeSymbol interfaceBinderType,
ISymbol member
) {
if( member.DeclaredAccessibility != Accessibility.Public ) {
return;
}

if( !SymbolEqualityComparer.Default.Equals( genericType.OriginalDefinition, interfaceBinderType ) ) {
return;
}

TypeSyntax interfaceArgument = argumentList.Arguments[ 0 ];

ITypeSymbol? interfaceType = context.SemanticModel
.GetTypeInfo( interfaceArgument, context.CancellationToken )
.Type;

if( interfaceType == null ) {
if( !SymbolEqualityComparer.Default.Equals( interfaceBinderType, member.ContainingType.OriginalDefinition ) ) {
return;
}

if( interfaceType.TypeKind == TypeKind.Interface ) {
ITypeSymbol boundType = member.ContainingType.TypeArguments[ 0 ];
if( boundType.TypeKind == TypeKind.Interface ) {
return;
}

context.ReportDiagnostic(
InterfaceBinder_InterfacesOnly,
interfaceArgument.GetLocation(),
messageArgs: new[] { interfaceType.Name }
);
InterfaceBinder_InterfacesOnly,
context.Operation.Syntax.GetLocation(),
messageArgs: new[] { boundType.Name }
);
}
}
}
54 changes: 35 additions & 19 deletions tests/D2L.CodeStyle.Analyzers.Test/Specs/InterfaceBinderAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,21 @@

namespace D2L.LP.LayeredArch.Data {

public static class InterfaceBinder<T> { }
public static class InterfaceBinder<T> {

private void PrivateMethod() {
// This would be an error if the analyzer didn't only consider the public API
PrivateMethod2();
}
private void PrivateMethod2() { }

public static object AllFields { get; } = null;
public static object CreateAllFields() => null;

public static object MatchedFields { get; } = null;
public static object CreateMathedFields() => null;

}
}

namespace SpecTests {
Expand All @@ -23,34 +37,36 @@ public interface InterfaceDto {
public class TestCases {

public void UsedWithClass() {
var type = typeof( InterfaceBinder< /* InterfaceBinder_InterfacesOnly(ClassDto) */ ClassDto /**/ > );
// This used to be considered an error; however, the updated version of the analyzer
// only looks at member accesses rather than generic type names to be less expensive
_ = typeof( InterfaceBinder<ClassDto> );

_ = /* InterfaceBinder_InterfacesOnly(ClassDto) */ InterfaceBinder<ClassDto>.AllFields /**/;
_ = /* InterfaceBinder_InterfacesOnly(ClassDto) */ InterfaceBinder<ClassDto>.CreateAllFields() /**/;
_ = /* InterfaceBinder_InterfacesOnly(ClassDto) */ InterfaceBinder<ClassDto>.MatchedFields /**/;
_ = /* InterfaceBinder_InterfacesOnly(ClassDto) */ InterfaceBinder<ClassDto>.CreateMathedFields() /**/;
Comment on lines -26 to +47
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Erroring on a different usage (now just public API usage), but much less work to do so. I don't expect simply referencing the type InterfaceBinder<ClassDto> would really lead to any issues (outside of reflection, but that's a general hole regardless).

}

public void UsedWithInterface() {
var type = typeof( InterfaceBinder<InterfaceDto> );
_ = typeof( InterfaceBinder<InterfaceDto> );

_ = InterfaceBinder<InterfaceDto>.AllFields;
_ = InterfaceBinder<InterfaceDto>.CreateAllFields;
_ = InterfaceBinder<InterfaceDto>.MatchedFields;
_ = InterfaceBinder<InterfaceDto>.CreateMathedFields;
}

public void GenericTypeDefinition() {
var type = typeof( InterfaceBinder<> );
_ = typeof( InterfaceBinder<> );
}

public void UnknownGenericTypeArgument() {
var type = typeof( InterfaceBinder< /* InterfaceBinder_InterfacesOnly(Wacky) */ Wacky /**/ > );
}
}

public class Unrelated {

public void UnknownGenericTypeDefinition() {
var type = typeof( Wacky<int> );
}

public void NotInterfaceBinder() {
var type = typeof( Action<int> );
}
_ = typeof( InterfaceBinder<Wacky> );

public void TooManyGenericTypeArguments() {
var type = typeof( Action<int, int> );
_ = /* InterfaceBinder_InterfacesOnly(Wacky) */ InterfaceBinder<Wacky>.AllFields /**/;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wacky is TypeKind.ErrorType, so still works

_ = /* InterfaceBinder_InterfacesOnly(Wacky) */ InterfaceBinder<Wacky>.CreateAllFields() /**/;
_ = /* InterfaceBinder_InterfacesOnly(Wacky) */ InterfaceBinder<Wacky>.MatchedFields /**/;
_ = /* InterfaceBinder_InterfacesOnly(Wacky) */ InterfaceBinder<Wacky>.CreateMathedFields() /**/;
}
}
}
Loading