Skip to content

Improve internal analyzer regarding AppImplementationFactory #1234

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

Merged
martinothamar merged 3 commits into from
Apr 7, 2025
Merged

Improve internal analyzer regarding AppImplementationFactory #1234

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
844adc5
Improve internal analyzer regarding 'AppImplementationFactory'
martinothamar Apr 4, 2025
8ab58b6
Tolerate arrow function properties
martinothamar Apr 7, 2025
9c23379
Merge branch 'main' into chore/more-internal-analysis
martinothamar Apr 7, 2025
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
2 changes: 2 additions & 0 deletions src/Altinn.App.Internal.Analyzers/AnalyzerReleases.Unshipped.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,6 @@ Rule ID | Category | Severity | Notes
--------|----------|----------|-------
ALTINNINT0001 | General | Error | Dangerous constructor injection
ALTINNINT0002 | General | Error | Dangerous 'IServiceProvider' service resolution
ALTINNINT0003 | General | Error | Dangerous 'AppImplementationFactory' service resolution
ALTINNINT0004 | General | Error | Invalid 'AppImplementationFactory' service resolution
ALTINNINT9999 | General | Error | Unknown error
268 changes: 243 additions & 25 deletions src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
{
public static readonly ImmutableArray<DiagnosticDescriptor> All;

static Diagnostics()

Check warning on line 9 in src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs

View workflow job for this annotation

GitHub Actions / Static code analysis 

Initialize all 'static fields' inline and remove the 'static constructor'. (https://rules.sonarsource.com/csharp/RSPEC-3963)
{
var getDiagnostics = static (Type type) =>
type.GetFields(BindingFlags.Public | BindingFlags.Static)
Expand Down Expand Up @@ -40,6 +40,22 @@
+ " App implementable interfaces are only meant to be resolved through 'AppImplementationFactory'."
);

public static readonly DiagnosticDescriptor DangerousAppImplementationFactoryUse = Error(
"ALTINNINT0003",
Category.General,
"Dangerous 'AppImplementationFactory' service resolution",
"App implementable service interface '{0}' is resolved through 'AppImplementationFactory' in a constructor."
+ " App implementable interfaces are only meant to be resolved lazily (as late as possible, right before use) to ensure expected lifetime."
);

public static readonly DiagnosticDescriptor NonAppImplementableServiceThroughAppImplementationFactory = Error(
"ALTINNINT0004",
Category.General,
"Invalid 'AppImplementationFactory' service resolution",
"Non-appimplementable service interface '{0}' is resolved through 'AppImplementationFactory'."
+ " You can use 'IServiceProvider' for these instead."
);

private static DiagnosticDescriptor Error(string id, string category, string title, string messageFormat) =>
Create(id, title, messageFormat, category, DiagnosticSeverity.Error);

Expand All @@ -61,7 +77,7 @@
public sealed class AppImplementationInjectionAnalyzer : DiagnosticAnalyzer
{
private const string MarkerAttributeName = "ImplementableByAppsAttribute";
private static readonly SymbolEqualityComparer _symbolComparer = SymbolEqualityComparer.Default;
private static readonly SymbolEqualityComparer _comparer = SymbolEqualityComparer.Default;

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => Diagnostics.All;

Expand All @@ -72,14 +88,106 @@

context.RegisterSyntaxNodeAction(AnalyzeConstructors, SyntaxKind.ParameterList);
context.RegisterSyntaxNodeAction(AnalyzeDIServiceCalls, SyntaxKind.InvocationExpression);
context.RegisterSyntaxNodeAction(
AnalyzeAppImplementationFactoryConstructorUse,
SyntaxKind.InvocationExpression
);
context.RegisterSyntaxNodeAction(
AnalyzeNonAppimplementableServicesThroughAppImplementationFactory,
SyntaxKind.InvocationExpression
);
}

private static void AnalyzeNonAppimplementableServicesThroughAppImplementationFactory(
SyntaxNodeAnalysisContext context
)
{
var semanticModel = context.SemanticModel;
var invocation = (InvocationExpressionSyntax)context.Node;
var methodSymbol = semanticModel.GetSymbolInfo(invocation, context.CancellationToken).Symbol as IMethodSymbol;
if (methodSymbol is null)
return;

var appImplementationFactoryType = GetAppImplementationFactorySymbol(context);
if (appImplementationFactoryType is null)
return;

if (!_comparer.Equals(methodSymbol.ContainingType, appImplementationFactoryType))
return;

var typeInfo = GetAppImplementationFactoryInvocationTypeArgument(
context,
appImplementationFactoryType,
invocation
);
if (typeInfo is null)
return;

if (IsMarkedWithAttribute(typeInfo))
return;

var diagnostic = Diagnostic.Create(
Diagnostics.NonAppImplementableServiceThroughAppImplementationFactory,
invocation.GetLocation(),
typeInfo.Name
);
context.ReportDiagnostic(diagnostic);
}

private static void AnalyzeAppImplementationFactoryConstructorUse(SyntaxNodeAnalysisContext context)
{
var invocation = (InvocationExpressionSyntax)context.Node;

var methodSymbol =
context.SemanticModel.GetSymbolInfo(invocation, context.CancellationToken).Symbol as IMethodSymbol;
if (methodSymbol is null)
return;

var appImplementationFactoryType = GetAppImplementationFactorySymbol(context);
if (appImplementationFactoryType is null)
return;

if (!_comparer.Equals(methodSymbol.ContainingType, appImplementationFactoryType))
return;

var parent = context.Node.Parent;
while (parent != null)
{
// Checks if the invocation is in a constructor body or field initializer (primary constructor)
if (parent is ConstructorDeclarationSyntax or FieldDeclarationSyntax)
{
var typeInfo = GetAppImplementationFactoryInvocationTypeArgument(
context,
appImplementationFactoryType,
invocation
);
if (typeInfo is null || !IsMarkedWithAttribute(typeInfo))
return;
var diagnostic = Diagnostic.Create(
Diagnostics.DangerousAppImplementationFactoryUse,
invocation.GetLocation(),
typeInfo.Name
);
context.ReportDiagnostic(diagnostic);
break;
}

if (parent is MethodDeclarationSyntax or PropertyDeclarationSyntax or ClassDeclarationSyntax)
{
// We can stop checking
break;
}

parent = parent.Parent;
}
}

private static void AnalyzeDIServiceCalls(SyntaxNodeAnalysisContext context)

Check warning on line 185 in src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs

View workflow job for this annotation

GitHub Actions / Static code analysis 

Refactor this method to reduce its Cognitive Complexity from 30 to the 15 allowed. (https://rules.sonarsource.com/csharp/RSPEC-3776)
{
var invocation = (InvocationExpressionSyntax)context.Node;

// This code checks the following forms of IServiceProvider service resolutions
// sp.GetService<IEFormidlingMetadata>();

Check warning on line 190 in src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs

View workflow job for this annotation

GitHub Actions / Static code analysis 

Remove this commented out code. (https://rules.sonarsource.com/csharp/RSPEC-125)
// sp.GetService(typeof(IEFormidlingMetadata));
// sp.GetRequiredService<IEFormidlingMetadata>();
// sp.GetRequiredService(typeof(IEFormidlingMetadata));
Expand All @@ -91,12 +199,10 @@
if (methodSymbol is null)
return;

var serviceProviderType = context.SemanticModel.Compilation.GetTypeByMetadataName("System.IServiceProvider");
var serviceProviderType = GetIServiceProviderSymbol(context);
if (serviceProviderType is null)
return;
var enumerableType = context
.SemanticModel.Compilation.GetTypeByMetadataName("System.Collections.Generic.IEnumerable`1")
?.ConstructUnboundGenericType();
var enumerableType = GetIEnumerableSymbol(context);
if (enumerableType is null)
return;

Expand All @@ -109,19 +215,17 @@
var firstArgType = context
.SemanticModel.GetTypeInfo(arguments[0].Expression, context.CancellationToken)
.Type;
isLongFormExtMethodCall = _symbolComparer.Equals(firstArgType, serviceProviderType);
isLongFormExtMethodCall = _comparer.Equals(firstArgType, serviceProviderType);
}
if (!_symbolComparer.Equals(methodSymbol.ReceiverType, serviceProviderType) && !isLongFormExtMethodCall)
if (!_comparer.Equals(methodSymbol.ReceiverType, serviceProviderType) && !isLongFormExtMethodCall)
return;
}
else
{
if (!_symbolComparer.Equals(methodSymbol.ContainingType, serviceProviderType))
if (!_comparer.Equals(methodSymbol.ContainingType, serviceProviderType))
return;
}

// System.Diagnostics.Debugger.Launch();

// check the generic form, e.g. GetService<T>()
TypeSyntax? typeSyntax = null;
var typeArgumentList = invocation.DescendantNodes().OfType<TypeArgumentListSyntax>().FirstOrDefault();
Expand All @@ -141,21 +245,21 @@
if (typeSyntax is null or PredefinedTypeSyntax)
return;

var typeInfoSymbol = context.SemanticModel.GetTypeInfo(typeSyntax, context.CancellationToken).Type;
var typeInfoSymbol = ResolveTypeSyntaxToPotentialAppImplementableType(context, typeSyntax);
if (typeInfoSymbol is not INamedTypeSymbol typeInfo)
return;

if (typeInfo.IsGenericType && _symbolComparer.Equals(typeInfo.ConstructUnboundGenericType(), enumerableType))
if (typeInfo.IsGenericType && _comparer.Equals(typeInfo.ConstructUnboundGenericType(), enumerableType))
{
if (typeInfo.TypeArguments.FirstOrDefault() is not INamedTypeSymbol innerType)
var enumerableTypeArgument = typeInfo.TypeArguments.FirstOrDefault();
if (enumerableTypeArgument is ITypeParameterSymbol typeParameterSymbol)
enumerableTypeArgument = GetMostRelevantConstraintType(typeParameterSymbol);
if (enumerableTypeArgument is not INamedTypeSymbol innerType)
return;
typeInfo = innerType;
}

if (
typeInfo.TypeKind == TypeKind.Interface
&& typeInfo.GetAttributes().Any(attr => attr.AttributeClass?.Name == MarkerAttributeName)
)
if (IsMarkedWithAttribute(typeInfo))
{
var diagnostic = Diagnostic.Create(
Diagnostics.DangerousServiceProviderServiceResolution,
Expand All @@ -167,7 +271,7 @@
}
}

private static void AnalyzeConstructors(SyntaxNodeAnalysisContext context)

Check warning on line 274 in src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs

View workflow job for this annotation

GitHub Actions / Static code analysis 

Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed. (https://rules.sonarsource.com/csharp/RSPEC-3776)
{
SyntaxToken? typeIdentifier = context.Node.Parent switch
{
Expand All @@ -181,9 +285,7 @@
if (typeIdentifier is null)
return;

var enumerableType = context
.SemanticModel.Compilation.GetTypeByMetadataName("System.Collections.Generic.IEnumerable`1")
?.ConstructUnboundGenericType();
var enumerableType = GetIEnumerableSymbol(context);
if (enumerableType is null)
return;

Expand All @@ -194,7 +296,7 @@

var constructorComments = node.GetLeadingTrivia().ToFullString();

foreach (var parameter in node.Parameters)

Check warning on line 299 in src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs

View workflow job for this annotation

GitHub Actions / Static code analysis 

Loop should be simplified by calling Select(parameter => parameter.Type)) (https://rules.sonarsource.com/csharp/RSPEC-3267)
{
var identifier = parameter.Type;
if (identifier is null)
Expand All @@ -205,7 +307,7 @@

if (context.Node.Parent is ConstructorDeclarationSyntax constructorDeclaration)
{
if (constructorDeclaration.Body is not null)

Check warning on line 310 in src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs

View workflow job for this annotation

GitHub Actions / Static code analysis 

Merge this if statement with the enclosing one. (https://rules.sonarsource.com/csharp/RSPEC-1066)
{
var identifiers = constructorDeclaration.Body.DescendantNodes().OfType<IdentifierNameSyntax>();
foreach (var identifier in identifiers)
Expand Down Expand Up @@ -234,7 +336,7 @@
context.ReportDiagnostic(diagnostic);
}

static void Process(

Check warning on line 339 in src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs

View workflow job for this annotation

GitHub Actions / Static code analysis 

Refactor this static local function to reduce its Cognitive Complexity from 19 to the 15 allowed. (https://rules.sonarsource.com/csharp/RSPEC-3776)
SyntaxNodeAnalysisContext context,
ExpressionSyntax syntax,
Dictionary<Location, (ITypeSymbol Symbol, CSharpSyntaxNode Syntax)> typesReferenced,
Expand All @@ -248,11 +350,12 @@
context.SemanticModel.GetSymbolInfo(syntax, context.CancellationToken).Symbol as INamedTypeSymbol;
if (typeInfo is null)
return;
if (
typeInfo.IsGenericType && _symbolComparer.Equals(typeInfo.ConstructUnboundGenericType(), enumerableType)
)
if (typeInfo.IsGenericType && _comparer.Equals(typeInfo.ConstructUnboundGenericType(), enumerableType))
{
if (typeInfo.TypeArguments.FirstOrDefault() is not INamedTypeSymbol innerType)
var enumerableTypeArgument = typeInfo.TypeArguments.FirstOrDefault();
if (enumerableTypeArgument is ITypeParameterSymbol typeParameterSymbol)
enumerableTypeArgument = GetMostRelevantConstraintType(typeParameterSymbol);
if (enumerableTypeArgument is not INamedTypeSymbol innerType)
return;
typeInfo = innerType;
}
Expand All @@ -261,10 +364,125 @@
var key = syntax.GetLocation();
if (typesReferenced.ContainsKey(key))
return;
if (!typeInfo.GetAttributes().Any(attr => attr.AttributeClass?.Name == MarkerAttributeName))
if (!IsMarkedWithAttribute(typeInfo))
return;

typesReferenced.Add(key, (typeInfo, syntax));
}
}

private static bool IsMarkedWithAttribute(ITypeSymbol symbol)
{
var attributes = symbol.GetAttributes();
foreach (var attribute in attributes)
{
if (attribute.AttributeClass?.Name == MarkerAttributeName)
return true;
}

return false;
}

private static INamedTypeSymbol? GetAppImplementationFactoryInvocationTypeArgument(

Check warning on line 386 in src/Altinn.App.Internal.Analyzers/AppImplementationInjectionAnalyzer.cs

View workflow job for this annotation

GitHub Actions / Static code analysis 

Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed. (https://rules.sonarsource.com/csharp/RSPEC-3776)
SyntaxNodeAnalysisContext context,
INamedTypeSymbol appImplementationFactoryType,
// Some method invocation syntax on the `AppImplementationFactory` type
// e.g. `GetRequired<T>()`
InvocationExpressionSyntax invocation
)
{
TypeArgumentListSyntax? typeArgumentList = null;
var typeArgumentLists = invocation.DescendantNodes().OfType<TypeArgumentListSyntax>().ToArray();
if (typeArgumentLists.Length == 0)
return null;
if (typeArgumentLists.Length == 1)
{
typeArgumentList = typeArgumentLists[0];
}
else
{
var factoryMethods = appImplementationFactoryType
.GetMembers()
.Where(m =>
m is IMethodSymbol method && !method.IsStatic && !method.IsAbstract && method.IsGenericMethod
)
.Cast<IMethodSymbol>()
.ToArray();
foreach (var typeArgumentListSyntax in typeArgumentLists)
{
if (typeArgumentListSyntax.Parent is not NameSyntax methodName)
continue;
if (context.SemanticModel.GetSymbolInfo(methodName).Symbol is not IMethodSymbol method)
continue;
if (!factoryMethods.Contains(method.OriginalDefinition, _comparer))
continue;

typeArgumentList = typeArgumentListSyntax;
break;
}
}

if (typeArgumentList is null)
return null;
var typeSyntax = typeArgumentList.Arguments.FirstOrDefault();
if (typeSyntax is null)
return null;
return ResolveTypeSyntaxToPotentialAppImplementableType(context, typeSyntax);
}

private static INamedTypeSymbol? ResolveTypeSyntaxToPotentialAppImplementableType(
SyntaxNodeAnalysisContext context,
TypeSyntax typeSyntax
)
{
var typeInfoSymbol = context.SemanticModel.GetTypeInfo(typeSyntax, context.CancellationToken).Type;
if (typeInfoSymbol is ITypeParameterSymbol typeParameterSymbol)
typeInfoSymbol = GetMostRelevantConstraintType(typeParameterSymbol);

if (typeInfoSymbol is not INamedTypeSymbol symbol)
return null;

return symbol;
}

private static ITypeSymbol? GetMostRelevantConstraintType(ITypeParameterSymbol typeParameterSymbol)
{
// If the type is a type parameter (T), we just take the first constraint type
// that is marked by the attribute or fallback to the first constraint type
// if none is marked with the attribute.
if (typeParameterSymbol.ConstraintTypes.Length == 0)
return null;

for (int i = 0; i < typeParameterSymbol.ConstraintTypes.Length; i++)
{
var constraintType = typeParameterSymbol.ConstraintTypes[i];
if (IsMarkedWithAttribute(constraintType))
return constraintType;
}

return typeParameterSymbol.ConstraintTypes[0];
}

private static INamedTypeSymbol? GetIEnumerableSymbol(SyntaxNodeAnalysisContext context)
{
var enumerableType = context
.SemanticModel.Compilation.GetTypeByMetadataName("System.Collections.Generic.IEnumerable`1")
?.ConstructUnboundGenericType();

return enumerableType;
}

private static INamedTypeSymbol? GetIServiceProviderSymbol(SyntaxNodeAnalysisContext context)
{
var serviceProviderType = context.SemanticModel.Compilation.GetTypeByMetadataName("System.IServiceProvider");
return serviceProviderType;
}

private static INamedTypeSymbol? GetAppImplementationFactorySymbol(SyntaxNodeAnalysisContext context)
{
var appImplementationFactoryType = context.SemanticModel.Compilation.GetTypeByMetadataName(
"Altinn.App.Core.Features.AppImplementationFactory"
);
return appImplementationFactoryType;
}
}
Loading