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.

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

Redo pubternalyzer with Roslyn Operations #21996

Merged
merged 13 commits into from
Aug 16, 2020
213 changes: 151 additions & 62 deletions src/EFCore.Analyzers/InternalUsageDiagnosticAnalyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,19 @@
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;

namespace Microsoft.EntityFrameworkCore
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
public class InternalUsageDiagnosticAnalyzer : DiagnosticAnalyzer
{
public const string Id = "EF1001";

public const string MessageFormat
= "{0} is an internal API that supports the Entity Framework Core infrastructure and " +
"not subject to the same compatibility standards as public APIs. " +
"It may be changed or removed without notice in any release.";
= "{0} is an internal API that supports the Entity Framework Core infrastructure and "
+ "not subject to the same compatibility standards as public APIs. "
+ "It may be changed or removed without notice in any release.";

protected const string DefaultTitle = "Internal EF Core API usage.";
protected const string Category = "Usage";
Expand All @@ -40,84 +41,172 @@ private static readonly DiagnosticDescriptor _descriptor
public override void Initialize(AnalysisContext context)
{
context.EnableConcurrentExecution();
context.RegisterSyntaxNodeAction(
AnalyzeNode,
SyntaxKind.SimpleMemberAccessExpression,
SyntaxKind.ObjectCreationExpression,
SyntaxKind.ClassDeclaration);
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);

context.RegisterOperationAction(AnalyzeNode,
OperationKind.FieldReference,
OperationKind.PropertyReference,
OperationKind.MethodReference,
OperationKind.EventReference,
OperationKind.Invocation,
OperationKind.ObjectCreation,
OperationKind.VariableDeclaration,
OperationKind.TypeOf);

context.RegisterSyntaxNodeAction(AnalyzeClassDeclaration, SyntaxKind.ClassDeclaration);
}

private void AnalyzeNode(SyntaxNodeAnalysisContext context)
private static void AnalyzeNode(OperationAnalysisContext context)
{
switch (context.Node)
switch (context.Operation.Kind)
{
case MemberAccessExpressionSyntax memberAccessSyntax:
{
if (context.SemanticModel.GetSymbolInfo(context.Node, context.CancellationToken).Symbol is ISymbol symbol
&& !Equals(symbol.ContainingAssembly, context.Compilation.Assembly))
case OperationKind.FieldReference:
AnalyzeMember(context, ((IFieldReferenceOperation)context.Operation).Field);
break;
case OperationKind.PropertyReference:
AnalyzeMember(context, ((IPropertyReferenceOperation)context.Operation).Property);
break;
case OperationKind.EventReference:
AnalyzeMember(context, ((IEventReferenceOperation)context.Operation).Event);
break;
case OperationKind.MethodReference:
AnalyzeMember(context, ((IMethodReferenceOperation)context.Operation).Method);
break;
case OperationKind.ObjectCreation:
AnalyzeMember(context, ((IObjectCreationOperation)context.Operation).Constructor);
break;

roji marked this conversation as resolved.
Show resolved Hide resolved
case OperationKind.Invocation:
AnalyzeInvocation(context, (IInvocationOperation)context.Operation);
break;

case OperationKind.VariableDeclaration:
AnalyzeVariableDeclaration(context, ((IVariableDeclarationOperation)context.Operation));
break;

case OperationKind.TypeOf:
AnalyzeTypeof(context, ((ITypeOfOperation)context.Operation));
break;

default:
throw new ArgumentException($"Unexpected {nameof(OperationKind)}: {context.Operation.Kind}");
}

}

private static void AnalyzeMember(OperationAnalysisContext context, ISymbol symbol)
{
if (Equals(symbol.ContainingAssembly, context.Compilation.Assembly))
{
// Skip all methods inside the same assembly - internal access is fine
return;
}

var containingType = symbol.ContainingType;

switch (symbol)
{
case IMethodSymbol _:
case IFieldSymbol _:
case IPropertySymbol _:
case IEventSymbol _:
roji marked this conversation as resolved.
Show resolved Hide resolved
if (HasInternalAttribute(symbol))
{
var containingType = symbol.ContainingType;

if (HasInternalAttribute(symbol))
{
context.ReportDiagnostic(
Diagnostic.Create(_descriptor, memberAccessSyntax.Name.GetLocation(), $"{containingType}.{symbol.Name}"));
return;
}

if (IsInInternalNamespace(containingType)
|| HasInternalAttribute(containingType))
{
context.ReportDiagnostic(Diagnostic.Create(_descriptor, memberAccessSyntax.Name.GetLocation(), containingType));
return;
}
ReportDiagnostic(symbol.Name == ".ctor" ? (object)containingType : $"{containingType}.{symbol.Name}");
return;
}
break;
}

return;
}
if (IsTypeInternal(context, containingType))
{
ReportDiagnostic(containingType);
}

case ObjectCreationExpressionSyntax creationSyntax:
void ReportDiagnostic(object messageArg)
{
// For C# member access expressions, report a narrowed-down diagnostic, otherwise take the whole invocation.
var syntax = context.Operation.Syntax switch
{
if (context.SemanticModel.GetSymbolInfo(context.Node, context.CancellationToken).Symbol is ISymbol symbol
&& !Equals(symbol.ContainingAssembly, context.Compilation.Assembly))
{
var containingType = symbol.ContainingType;

if (HasInternalAttribute(symbol))
{
context.ReportDiagnostic(Diagnostic.Create(_descriptor, creationSyntax.GetLocation(), containingType));
return;
}

if (IsInInternalNamespace(containingType)
|| HasInternalAttribute(containingType))
{
context.ReportDiagnostic(Diagnostic.Create(_descriptor, creationSyntax.Type.GetLocation(), containingType));
return;
}
}
InvocationExpressionSyntax invocationSyntax
when invocationSyntax.Expression is MemberAccessExpressionSyntax memberAccessSyntax
=> memberAccessSyntax.Name,
MemberAccessExpressionSyntax memberAccessSyntax
=> memberAccessSyntax.Name,
ObjectCreationExpressionSyntax objectCreationSyntax
=> objectCreationSyntax.Type,
_
=> context.Operation.Syntax
};

context.ReportDiagnostic(Diagnostic.Create(_descriptor, syntax.GetLocation(), messageArg));
}
}

return;
private static void AnalyzeInvocation(OperationAnalysisContext context, IInvocationOperation invocation)
{
// First check for any internal type parameters
foreach (var a in invocation.TargetMethod.TypeArguments)

Choose a reason for hiding this comment

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

Do we do similar analysis for generic type arguments of named types?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not at the moment. Do you have guidance on how to get the generic type arguments of an ITypeSymbol?

Copy link
Member Author

Choose a reason for hiding this comment

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

Scoping out to #22086, any guidance on this would be appreciated.

{
if (IsTypeInternal(context, a))
{
context.ReportDiagnostic(Diagnostic.Create(_descriptor, context.Operation.Syntax.GetLocation(), a));
}
}

case ClassDeclarationSyntax declarationSyntax:
{
if (context.SemanticModel.GetDeclaredSymbol(declarationSyntax)?.BaseType is ISymbol symbol
&& !Equals(symbol.ContainingAssembly, context.Compilation.Assembly)
&& (IsInInternalNamespace(symbol) || HasInternalAttribute(symbol))
&& declarationSyntax.BaseList?.Types.Count > 0)
{
context.ReportDiagnostic(Diagnostic.Create(_descriptor, declarationSyntax.BaseList.Types[0].GetLocation(), symbol));
}
// Then check the method being invoked
AnalyzeMember(context, invocation.TargetMethod);
}

private static void AnalyzeVariableDeclaration(OperationAnalysisContext context, IVariableDeclarationOperation variableDeclaration)
{
foreach (var declarator in variableDeclaration.Declarators)
{
if (IsTypeInternal(context, declarator.Symbol.Type))
{
context.ReportDiagnostic(
Diagnostic.Create(
_descriptor,
((VariableDeclarationSyntax)context.Operation.Syntax).Type.GetLocation(),
declarator.Symbol.Type));
return;
}
}
}

private static void AnalyzeTypeof(OperationAnalysisContext context, ITypeOfOperation typeOf)
{
if (IsTypeInternal(context, typeOf.TypeOperand))
{
context.ReportDiagnostic(
Diagnostic.Create(
_descriptor,
((TypeOfExpressionSyntax)context.Operation.Syntax).Type.GetLocation(),
typeOf.TypeOperand));
}
}

private static void AnalyzeClassDeclaration(SyntaxNodeAnalysisContext context)
{
var declarationSyntax = (ClassDeclarationSyntax)context.Node;

if (context.SemanticModel.GetDeclaredSymbol(declarationSyntax)?.BaseType is ISymbol symbol
&& !Equals(symbol.ContainingAssembly, context.Compilation.Assembly)
&& (IsInInternalNamespace(symbol) || HasInternalAttribute(symbol))
&& declarationSyntax.BaseList?.Types.Count > 0)
{
context.ReportDiagnostic(Diagnostic.Create(_descriptor, declarationSyntax.BaseList.Types[0].GetLocation(), symbol));
}
}

private static bool IsTypeInternal(OperationAnalysisContext context, ISymbol symbol)
=> !Equals(symbol.ContainingAssembly, context.Compilation.Assembly)
&& (IsInInternalNamespace(symbol) || HasInternalAttribute(symbol));

private static bool HasInternalAttribute(ISymbol symbol)
=> symbol != null && symbol.GetAttributes().Any(a => a.AttributeClass.Name == "EntityFrameworkInternalAttribute");
=> symbol != null
&& symbol.GetAttributes().Any(a =>
a.AttributeClass.ToDisplayString() == "Microsoft.EntityFrameworkCore.Infrastructure.EntityFrameworkInternalAttribute");

private static bool IsInInternalNamespace(ISymbol symbol)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -365,9 +365,12 @@ private void AddInclude(
var includeMethod = navigation.IsCollection ? _includeCollectionMethodInfo : _includeReferenceMethodInfo;
var includingClrType = navigation.DeclaringEntityType.ClrType;
var relatedEntityClrType = navigation.TargetEntityType.ClrType;
#pragma warning disable EF1001 // Internal EF Core API usage.
Copy link
Member Author

Choose a reason for hiding this comment

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

I think you already know about these internal usages @AndriySvyryd, but just in case.

var entityEntryVariable = _trackQueryResults
? shaperBlock.Variables.Single(v => v.Type == typeof(InternalEntityEntry))
: (Expression)Expression.Constant(null, typeof(InternalEntityEntry));
#pragma warning restore EF1001 // Internal EF Core API usage.

var concreteEntityTypeVariable = shaperBlock.Variables.Single(v => v.Type == typeof(IEntityType));
var inverseNavigation = navigation.Inverse;
var fixup = GenerateFixup(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,28 +254,28 @@ private bool TryGenerateIdFromKeys(IProperty idProperty, out object value)
{
var entityEntry = Activator.CreateInstance(_readItemExpression.EntityType.ClrType);

#pragma warning disable EF1001
#pragma warning disable EF1001 // Internal EF Core API usage.
var internalEntityEntry = new InternalEntityEntryFactory().Create(
_cosmosQueryContext.Context.GetDependencies().StateManager, _readItemExpression.EntityType, entityEntry);
#pragma warning restore EF1001
#pragma warning restore EF1001 // Internal EF Core API usage.

foreach (var keyProperty in _readItemExpression.EntityType.FindPrimaryKey().Properties)
{
var property = _readItemExpression.EntityType.FindProperty(keyProperty.Name);

if (TryGetParameterValue(property, out var parameterValue))
{
#pragma warning disable EF1001 // Internal EF Core API usage.
internalEntityEntry[property] = parameterValue;
#pragma warning restore EF1001 // Internal EF Core API usage.
}
}

#pragma warning disable EF1001 // Internal EF Core API usage.
internalEntityEntry.SetEntityState(EntityState.Added);
#pragma warning restore EF1001 // Internal EF Core API usage.

value = internalEntityEntry[idProperty];

#pragma warning disable EF1001 // Internal EF Core API usage.
internalEntityEntry.SetEntityState(EntityState.Detached);
#pragma warning restore EF1001 // Internal EF Core API usage.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ public MigrationsModelDiffer(
{
Check.NotNull(typeMappingSource, nameof(typeMappingSource));
Check.NotNull(migrationsAnnotations, nameof(migrationsAnnotations));
#pragma warning disable EF1001 // Internal EF Core API usage.
Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @bricelam

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll defer to @AndriySvyryd on these since they're here for seed data

Copy link
Member

Choose a reason for hiding this comment

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

Filed #22063
Let's just say that the current approach is not as robust as it should be

Check.NotNull(changeDetector, nameof(changeDetector));
#pragma warning restore EF1001 // Internal EF Core API usage.
Check.NotNull(updateAdapterFactory, nameof(updateAdapterFactory));
Check.NotNull(commandBatchPreparerDependencies, nameof(commandBatchPreparerDependencies));

Expand Down
Loading