-
-
Notifications
You must be signed in to change notification settings - Fork 99
fix(analyzers): EVTC001 false positive on object payloads, harden type lookups #581
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
Changes from all commits
d1d11f2
88c22b8
f28b6bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,6 @@ | |
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.CodeAnalysis.Operations; | ||
| using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
| using static Eventuous.Shared.Generators.Constants; | ||
|
|
||
| // ReSharper disable CognitiveComplexity | ||
|
|
||
|
|
@@ -37,48 +36,66 @@ public override void Initialize(AnalysisContext context) { | |
| var compilation = compilationContext.Compilation; | ||
| var knownTypes = new KnownTypeSymbols(compilation); | ||
|
|
||
| // Without these two, annotations and explicit registrations cannot be checked, | ||
| // so any diagnostic would be an unverifiable false positive — stay silent instead | ||
| if (knownTypes.EventTypeAttribute == null || knownTypes.TypeMapper == null) return; | ||
|
|
||
| compilationContext.RegisterOperationAction(ctx => AnalyzeInvocation(ctx, knownTypes), OperationKind.Invocation); | ||
| compilationContext.RegisterOperationAction(ctx => AnalyzeObjectCreation(ctx, knownTypes), OperationKind.ObjectCreation); | ||
| }); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Cache of well-known type symbols resolved from the compilation. | ||
| /// This makes the analyzer refactoring-safe by using symbol comparison instead of string matching. | ||
| /// Symbol comparison against these is the only matching mechanism; if a symbol doesn't resolve, | ||
| /// the corresponding check simply doesn't apply. The metadata names in <see cref="WellKnownTypeNames"/> | ||
| /// are pinned by tests against the real Eventuous assemblies. | ||
| /// </summary> | ||
| sealed class KnownTypeSymbols(Compilation compilation) { | ||
| public INamedTypeSymbol? EventTypeAttribute { get; } = compilation.GetTypeByMetadataName(EventTypeAttrFqcn); | ||
| public INamedTypeSymbol? TypeMapper { get; } = compilation.GetTypeByMetadataName($"{BaseNamespace}.TypeMapper"); | ||
| public INamedTypeSymbol? Aggregate { get; } = compilation.GetTypeByMetadataName($"{BaseNamespace}.Aggregate`1"); | ||
| public INamedTypeSymbol? State { get; } = compilation.GetTypeByMetadataName($"{BaseNamespace}.State`1"); | ||
| public INamedTypeSymbol? CommandHandlerBuilder { get; } = compilation.GetTypeByMetadataName($"{BaseNamespace}.CommandHandlerBuilder"); | ||
| public INamedTypeSymbol? IDefineExecution { get; } = compilation.GetTypeByMetadataName($"{BaseNamespace}.IDefineExecution"); | ||
| public INamedTypeSymbol? ICommandHandlerBuilder { get; } = compilation.GetTypeByMetadataName($"{BaseNamespace}.ICommandHandlerBuilder"); | ||
| public INamedTypeSymbol? IDefineStoreOrExecution { get; } = compilation.GetTypeByMetadataName($"{BaseNamespace}.IDefineStoreOrExecution"); | ||
| public INamedTypeSymbol? BaseEventHandler { get; } = compilation.GetTypeByMetadataName("Eventuous.Subscriptions.BaseEventHandler"); | ||
| public INamedTypeSymbol? EventTypeAttribute { get; } = GetBestTypeByMetadataName(compilation, WellKnownTypeNames.EventTypeAttribute); | ||
| public INamedTypeSymbol? TypeMapper { get; } = GetBestTypeByMetadataName(compilation, WellKnownTypeNames.TypeMapper); | ||
| public INamedTypeSymbol? Aggregate { get; } = GetBestTypeByMetadataName(compilation, WellKnownTypeNames.Aggregate); | ||
| public INamedTypeSymbol? State { get; } = GetBestTypeByMetadataName(compilation, WellKnownTypeNames.State); | ||
| public INamedTypeSymbol? CommandHandlerBuilder { get; } = GetBestTypeByMetadataName(compilation, WellKnownTypeNames.CommandHandlerBuilder); | ||
| public INamedTypeSymbol? IDefineExecution { get; } = GetBestTypeByMetadataName(compilation, WellKnownTypeNames.IDefineExecution); | ||
| public INamedTypeSymbol? ICommandHandlerBuilder { get; } = GetBestTypeByMetadataName(compilation, WellKnownTypeNames.ICommandHandlerBuilder); | ||
| public INamedTypeSymbol? IDefineStoreOrExecution { get; } = GetBestTypeByMetadataName(compilation, WellKnownTypeNames.IDefineStoreOrExecution); | ||
| public INamedTypeSymbol? BaseEventHandler { get; } = GetBestTypeByMetadataName(compilation, WellKnownTypeNames.BaseEventHandler); | ||
|
|
||
| // GetTypeByMetadataName returns null not only when the type is missing but also when more than | ||
| // one referenced assembly defines it; in the ambiguous case pick the single accessible candidate | ||
| static INamedTypeSymbol? GetBestTypeByMetadataName(Compilation compilation, string metadataName) { | ||
| var type = compilation.GetTypeByMetadataName(metadataName); | ||
|
|
||
| if (type != null) return type; | ||
|
|
||
| INamedTypeSymbol? best = null; | ||
|
|
||
| foreach (var candidate in compilation.GetTypesByMetadataName(metadataName)) { | ||
| if (candidate.DeclaredAccessibility != Accessibility.Public | ||
| && !SymbolEqualityComparer.Default.Equals(candidate.ContainingAssembly, compilation.Assembly)) continue; | ||
|
|
||
| if (best != null) return null; | ||
|
|
||
| best = candidate; | ||
| } | ||
|
Comment on lines
+74
to
+81
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping the explicit loop: the logic is 'return null when a second accessible candidate appears', and moving the filter into |
||
|
|
||
| return best; | ||
| } | ||
| } | ||
|
|
||
| static ImmutableHashSet<ITypeSymbol> GetExplicitRegistrations(OperationAnalysisContext ctx, KnownTypeSymbols knownTypes) { | ||
| var model = ctx.Operation.SemanticModel; | ||
| if (model == null) return ImmutableHashSet<ITypeSymbol>.Empty; | ||
| if (model == null || knownTypes.TypeMapper == null) return ImmutableHashSet<ITypeSymbol>.Empty; | ||
| var root = ctx.Operation.Syntax.SyntaxTree.GetRoot(); | ||
| var set = ImmutableHashSet.CreateBuilder<ITypeSymbol>(SymbolEqualityComparer.Default); | ||
|
|
||
| foreach (var invSyntax in root.DescendantNodes().OfType<InvocationExpressionSyntax>()) { | ||
| if (model.GetOperation(invSyntax) is not IInvocationOperation op) continue; | ||
| var m = op.TargetMethod; | ||
|
|
||
| // Use symbol comparison when available, fall back to string comparison | ||
| if (m.Name != "AddType") continue; | ||
| var ct = m.ContainingType; | ||
| if (ct == null) continue; | ||
|
|
||
| // Prefer symbol comparison (refactoring-safe) | ||
| var isTypeMapper = knownTypes.TypeMapper != null | ||
| ? SymbolEqualityComparer.Default.Equals(ct, knownTypes.TypeMapper) | ||
| : ct.Name == "TypeMapper" && ct.ContainingNamespace?.ToDisplayString() == BaseNamespace; | ||
|
|
||
| if (!isTypeMapper) continue; | ||
| if (!SymbolEqualityComparer.Default.Equals(m.ContainingType, knownTypes.TypeMapper)) continue; | ||
|
|
||
| if (m.TypeArguments.Length == 1) { | ||
| set.Add(m.TypeArguments[0]); | ||
|
|
@@ -159,20 +176,18 @@ static void AnalyzeInvocation(OperationAnalysisContext ctx, KnownTypeSymbols kno | |
| // Heuristic: only consider the overloads that accept a delegate and are defined in CommandHandlerBuilder interfaces/classes | ||
| if (!IsFunctionalServiceAct(method, knownTypes)) return; | ||
|
|
||
| // If the argument is a lambda, analyze its body for created event instances. | ||
| // Lambdas passed as delegate arguments surface as IDelegateCreationOperation, possibly wrapped in a conversion. | ||
| foreach (var value in inv.Arguments.Select(arg => arg.Value)) { | ||
| switch (value) { | ||
| case null: | ||
| continue; | ||
| // If the argument is a lambda, analyze its body for created event instances | ||
| case IAnonymousFunctionOperation lambda: | ||
| AnalyzeDelegateBodyForEventCreations(ctx, lambda.Body, knownTypes); | ||
|
|
||
| break; | ||
| case IConversionOperation { Operand: IAnonymousFunctionOperation lambdaConv }: | ||
| AnalyzeDelegateBodyForEventCreations(ctx, lambdaConv.Body, knownTypes); | ||
|
|
||
| break; | ||
| } | ||
| var lambda = value switch { | ||
| IAnonymousFunctionOperation anon => anon, | ||
| IDelegateCreationOperation { Target: IAnonymousFunctionOperation anon } => anon, | ||
| IConversionOperation { Operand: IAnonymousFunctionOperation anon } => anon, | ||
| IConversionOperation { Operand: IDelegateCreationOperation { Target: IAnonymousFunctionOperation anon } } => anon, | ||
|
Comment on lines
+184
to
+186
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 88c22b8. |
||
| _ => null | ||
| }; | ||
|
|
||
| if (lambda != null) AnalyzeDelegateBodyForEventCreations(ctx, lambda.Body, knownTypes); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For a synchronous handler such as Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 88c22b8. The object-creation safety net now skips creations located inside a lambda passed to a functional-service |
||
| } | ||
| } | ||
| } | ||
|
|
@@ -204,13 +219,22 @@ static void AnalyzeObjectCreation(OperationAnalysisContext ctx, KnownTypeSymbols | |
|
|
||
| if (method == null) return; | ||
|
|
||
| if (ReturnsNewEvents(method)) { | ||
| // Creations inside lambdas passed to Act/ActAsync are reported by the invocation traversal; skip them here | ||
| if (ReturnsNewEvents(method) && !IsWithinFunctionalActInvocation(create, knownTypes)) { | ||
| if (!HasEventTypeAttribute(created, knownTypes) && !IsExplicitlyRegistered(created, ctx, knownTypes)) { | ||
| ctx.ReportDiagnostic(Diagnostic.Create(MissingEventTypeAttribute, create.Syntax.GetLocation(), created.ToDisplayString())); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| static bool IsWithinFunctionalActInvocation(IOperation op, KnownTypeSymbols knownTypes) { | ||
| for (var p = op.Parent; p != null; p = p.Parent) { | ||
| if (p is IInvocationOperation inv && IsFunctionalServiceAct(inv.TargetMethod, knownTypes)) return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| static IMethodSymbol? GetEnclosingMethod(IOperation op) { | ||
| for (var p = op.Parent; p != null; p = p.Parent) { | ||
| switch (p) { | ||
|
|
@@ -248,105 +272,57 @@ static bool IsIEnumerableOfObject(INamedTypeSymbol type) { | |
| return false; | ||
| } | ||
|
|
||
| static bool IsAggregate(INamedTypeSymbol? type, KnownTypeSymbols knownTypes) { | ||
| if (type == null) return false; | ||
|
|
||
| // Walk base types to check if it derives from Eventuous.Aggregate<> | ||
| for (var t = type; t != null; t = t.BaseType) { | ||
| // Prefer symbol comparison (refactoring-safe) | ||
| if (knownTypes.Aggregate != null) { | ||
| if (SymbolEqualityComparer.Default.Equals(t.OriginalDefinition, knownTypes.Aggregate)) { | ||
| return true; | ||
| } | ||
| } | ||
| else { | ||
| // Fallback to string comparison | ||
| if (t is { Name: "Aggregate", Arity: 1 } && t.ContainingNamespace.ToDisplayString() == BaseNamespace) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| static bool IsState(INamedTypeSymbol? type, KnownTypeSymbols knownTypes) { | ||
| if (type == null) return false; | ||
| // Walk base types to check if the type derives from Eventuous.Aggregate<> | ||
| static bool IsAggregate(INamedTypeSymbol? type, KnownTypeSymbols knownTypes) => DerivesFrom(type, knownTypes.Aggregate); | ||
|
|
||
| // Walk base types to check if it derives from Eventuous.State<> | ||
| for (var t = type; t != null; t = t.BaseType) { | ||
| // Prefer symbol comparison (refactoring-safe) | ||
| if (knownTypes.State != null) { | ||
| if (SymbolEqualityComparer.Default.Equals(t.OriginalDefinition, knownTypes.State)) { | ||
| return true; | ||
| } | ||
| } | ||
| else { | ||
| // Fallback to string comparison | ||
| if (t is { Name: "State", Arity: 1 } && t.ContainingNamespace.ToDisplayString() == BaseNamespace) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| // Walk base types to check if the type derives from Eventuous.State<> | ||
| static bool IsState(INamedTypeSymbol? type, KnownTypeSymbols knownTypes) => DerivesFrom(type, knownTypes.State); | ||
|
|
||
| return false; | ||
| } | ||
| static bool IsEventHandler(INamedTypeSymbol? type, KnownTypeSymbols knownTypes) => DerivesFrom(type, knownTypes.BaseEventHandler); | ||
|
|
||
| static bool IsEventHandler(INamedTypeSymbol? type, KnownTypeSymbols knownTypes) { | ||
| if (type == null) return false; | ||
| static bool DerivesFrom(INamedTypeSymbol? type, INamedTypeSymbol? baseDefinition) { | ||
| if (baseDefinition == null) return false; | ||
|
|
||
| for (var t = type; t != null; t = t.BaseType) { | ||
| if (knownTypes.BaseEventHandler != null) { | ||
| if (SymbolEqualityComparer.Default.Equals(t.OriginalDefinition, knownTypes.BaseEventHandler)) { | ||
| return true; | ||
| } | ||
| } | ||
| else { | ||
| if (t is { Name: "BaseEventHandler", Arity: 0 } && t.ContainingNamespace?.ToDisplayString() == "Eventuous.Subscriptions") { | ||
| return true; | ||
| } | ||
| } | ||
| if (SymbolEqualityComparer.Default.Equals(t.OriginalDefinition, baseDefinition)) return true; | ||
| } | ||
|
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
|
||
|
|
||
| return false; | ||
| } | ||
|
|
||
| static bool IsFunctionalServiceAct(IMethodSymbol method, KnownTypeSymbols knownTypes) { | ||
| // We only care about the Act methods from CommandHandlerBuilder and the related interfaces in Eventuous namespace | ||
| // We only care about the Act methods from CommandHandlerBuilder and the related interfaces in Eventuous namespace. | ||
| // The containing type at the call site is a constructed generic, so compare its original definition. | ||
| if (method.Name is not ("Act" or "ActAsync")) return false; | ||
|
|
||
| var containing = method.ContainingType; | ||
| var definition = method.ContainingType?.OriginalDefinition; | ||
|
|
||
| if (containing == null) return false; | ||
| if (definition == null) return false; | ||
|
|
||
| // Prefer symbol comparison (refactoring-safe) | ||
| if (knownTypes.CommandHandlerBuilder != null || knownTypes.IDefineExecution != null || | ||
| knownTypes.ICommandHandlerBuilder != null || knownTypes.IDefineStoreOrExecution != null) { | ||
| return SymbolEqualityComparer.Default.Equals(containing, knownTypes.CommandHandlerBuilder) || | ||
| SymbolEqualityComparer.Default.Equals(containing, knownTypes.IDefineExecution) || | ||
| SymbolEqualityComparer.Default.Equals(containing, knownTypes.ICommandHandlerBuilder) || | ||
| SymbolEqualityComparer.Default.Equals(containing, knownTypes.IDefineStoreOrExecution); | ||
| } | ||
|
|
||
| // Fallback to string comparison | ||
| var ns = containing.ContainingNamespace?.ToDisplayString(); | ||
| if (ns != BaseNamespace) return false; | ||
|
|
||
| return containing.Name is "CommandHandlerBuilder" or "IDefineExecution" or "ICommandHandlerBuilder" or "IDefineStoreOrExecution"; | ||
| return SymbolEqualityComparer.Default.Equals(definition, knownTypes.CommandHandlerBuilder) | ||
| || SymbolEqualityComparer.Default.Equals(definition, knownTypes.IDefineExecution) | ||
| || SymbolEqualityComparer.Default.Equals(definition, knownTypes.ICommandHandlerBuilder) | ||
| || SymbolEqualityComparer.Default.Equals(definition, knownTypes.IDefineStoreOrExecution); | ||
| } | ||
|
|
||
| static bool IsConcreteEvent(ITypeSymbol type) => type.TypeKind is TypeKind.Class or TypeKind.Struct; | ||
|
|
||
| static bool HasEventTypeAttribute(ITypeSymbol type, KnownTypeSymbols knownTypes) { | ||
| // Prefer symbol comparison (refactoring-safe) | ||
| if (knownTypes.EventTypeAttribute != null) { | ||
| return type.GetAttributes().Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, knownTypes.EventTypeAttribute)); | ||
| // System.Object is excluded: an object-typed value (e.g. StreamEvent.Payload or IMessageConsumeContext.Message) | ||
| // carries a runtime-resolved event type, so there is nothing to annotate at the call site. | ||
| // The System namespace is excluded as a whole: framework types constructed inside handlers | ||
| // (List<object>, DateTime, ...) are never domain events. | ||
| static bool IsConcreteEvent(ITypeSymbol type) | ||
| => type.SpecialType is not SpecialType.System_Object | ||
| && type.TypeKind is TypeKind.Class or TypeKind.Struct | ||
| && !IsInSystemNamespace(type); | ||
|
|
||
| static bool IsInSystemNamespace(ITypeSymbol type) { | ||
| for (var ns = type.ContainingNamespace; ns is { IsGlobalNamespace: false }; ns = ns.ContainingNamespace) { | ||
| if (ns.ContainingNamespace is { IsGlobalNamespace: true }) return ns.Name == "System"; | ||
| } | ||
|
|
||
| // Fallback to string comparison | ||
| return (from attrClass in type.GetAttributes().Select(a => a.AttributeClass).OfType<INamedTypeSymbol>() | ||
| let name = attrClass.ToDisplayString() | ||
| where name == EventTypeAttrFqcn || attrClass.Name is EventTypeAttribute | ||
| select attrClass).Any(); | ||
| return false; | ||
| } | ||
|
|
||
| static bool HasEventTypeAttribute(ITypeSymbol type, KnownTypeSymbols knownTypes) | ||
| => knownTypes.EventTypeAttribute != null | ||
| && type.GetAttributes().Any(a => SymbolEqualityComparer.Default.Equals(a.AttributeClass, knownTypes.EventTypeAttribute)); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| // Copyright (C) Eventuous HQ OÜ. All rights reserved | ||
| // Licensed under the Apache License, Version 2.0. | ||
|
|
||
| using System.Collections.Immutable; | ||
|
|
||
| namespace Eventuous.Shared.Generators; | ||
|
|
||
| /// <summary> | ||
| /// Fully qualified metadata names of the Eventuous types the analyzers resolve via | ||
| /// <see cref="Microsoft.CodeAnalysis.Compilation.GetTypeByMetadataName(string)"/>. | ||
| /// The analyzer cannot reference the runtime assemblies (it targets netstandard2.0, they don't), | ||
| /// so these strings are the single source of truth. Every name is pinned by a test that resolves | ||
| /// it against the current Eventuous assemblies, so renaming a type fails CI in the same change. | ||
| /// </summary> | ||
| public static class WellKnownTypeNames { | ||
| public const string EventTypeAttribute = Constants.EventTypeAttrFqcn; | ||
| public const string TypeMapper = $"{Constants.BaseNamespace}.TypeMapper"; | ||
| public const string Aggregate = $"{Constants.BaseNamespace}.Aggregate`1"; | ||
| public const string State = $"{Constants.BaseNamespace}.State`1"; | ||
| public const string CommandHandlerBuilder = $"{Constants.BaseNamespace}.CommandHandlerBuilder`2"; | ||
| public const string IDefineExecution = $"{Constants.BaseNamespace}.IDefineExecution`2"; | ||
| public const string ICommandHandlerBuilder = $"{Constants.BaseNamespace}.ICommandHandlerBuilder`2"; | ||
| public const string IDefineStoreOrExecution = $"{Constants.BaseNamespace}.IDefineStoreOrExecution`2"; | ||
| public const string BaseEventHandler = "Eventuous.Subscriptions.BaseEventHandler"; | ||
|
|
||
| public static readonly ImmutableArray<string> All = [ | ||
| EventTypeAttribute, | ||
| TypeMapper, | ||
| Aggregate, | ||
| State, | ||
| CommandHandlerBuilder, | ||
| IDefineExecution, | ||
| ICommandHandlerBuilder, | ||
| IDefineStoreOrExecution, | ||
| BaseEventHandler | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. Registered events may warn
🐞 Bug≡ CorrectnessAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation toolsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f28b6bf, with a stronger invariant than a fallback: if either
EventTypeAttributeorTypeMapperfails to resolve, the analyzer registers no actions at all — no diagnostic can be verified without them, so silence beats unverifiable warnings. Note the ambiguity scenario itself (two accessibleEventuous.TypeMapperdefinitions) means the user's ownTypeMap.Instance.AddTypecalls fail with CS0433, so their build is broken regardless; the ILMerge-style case (one public + internalized copies) already resolves to the public candidate. Pinned byShould_stay_silent_when_event_type_attribute_cannot_be_resolved.