fix(analyzers): EVTC001 false positive on object payloads, harden type lookups - #581
Conversation
…e lookups Fixes three defects in the EVTC001 event usage analyzer: - System.Object is no longer treated as a concrete event type, so runtime-resolved payloads (StreamEvent.Payload, IMessageConsumeContext.Message) passed to State<T>.When() no longer trigger a false positive (#535) - The functional service builder type lookups used metadata names without the arity suffix (CommandHandlerBuilder vs CommandHandlerBuilder`2), so they never resolved and symbol comparison silently fell back to string matching; call sites also see constructed generics, so the comparison now uses OriginalDefinition - Act/ActAsync lambda arguments surface as IDelegateCreationOperation, which the analyzer did not match, so handler bodies were never analyzed and unannotated events created in ActAsync handlers were missed All string-name fallbacks are deleted. Metadata names now live in WellKnownTypeNames as the single source of truth, resolved through GetTypesByMetadataName to tolerate ambiguous references, and pinned by tests that resolve every name against the real Eventuous assemblies so a type rename fails CI in the same change. Closes #535 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR Summary by QodoFix EVTC001 false positives on object payloads; harden analyzer type resolution
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1d11f2545
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| IDelegateCreationOperation { Target: IAnonymousFunctionOperation anon } => anon, | ||
| IConversionOperation { Operand: IAnonymousFunctionOperation anon } => anon, | ||
| IConversionOperation { Operand: IDelegateCreationOperation { Target: IAnonymousFunctionOperation anon } } => anon, |
There was a problem hiding this comment.
When an Act/ActAsync lambda builds its result with an intermediate allocation, such as var events = new List<object>();, this newly enabled traversal sends the entire lambda body to a scanner that treats every class or struct construction except object as a domain event. It therefore emits EVTC001 for List<object> and other helper objects that are never events; trace only values returned or added to NewEvents instead of scanning every creation in the body.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 88c22b8. IsConcreteEvent now excludes the entire System namespace, so List<object> and other BCL allocations inside handlers are no longer flagged (pinned by Should_not_warn_for_helper_allocations_in_act_handlers). User-defined non-event types constructed inside a handler (e.g. value objects) can still be flagged — full emitted-value tracing would trade that for false negatives on indirect flows like var e = new MyEvent(); return [e];, which this analyzer intentionally keeps catching.
| _ => null | ||
| }; | ||
|
|
||
| if (lambda != null) AnalyzeDelegateBodyForEventCreations(ctx, lambda.Body, knownTypes); |
There was a problem hiding this comment.
For a synchronous handler such as Act(_ => new object[] { new UnannotatedEvent() }), this traversal reports the event construction, and the separately registered OperationKind.ObjectCreation callback analyzes the same node again. Because the lambda symbol returns NewEvents as IEnumerable<object>, ReturnsNewEvents accepts it and emits an identical diagnostic at the same location; reserve that fallback for method groups or otherwise deduplicate these reports.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 88c22b8. The object-creation safety net now skips creations located inside a lambda passed to a functional-service Act/ActAsync invocation — the invocation traversal owns those, so the safety net remains for method groups only. Pinned by Should_report_sync_act_event_exactly_once, which failed with 2 identical diagnostics before the change.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| 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; | ||
| } |
There was a problem hiding this comment.
Keeping the explicit loop: the logic is 'return null when a second accessible candidate appears', and moving the filter into .Where(...) still leaves the two-match early exit in the loop body, so it doesn't simplify anything — it just splits one condition across two places.
| foreach (var payload in payloads) { | ||
| if (payload != null) state = state.When(payload); | ||
| } |
There was a problem hiding this comment.
Keeping as is: this file is analyzed fixture source, and the loop deliberately mirrors the exact reproduction from #535 (if (evt.Payload != null) state = state.When(evt.Payload)). Rewriting it with LINQ would change the operation shapes the analyzer sees and diverge from the reported pattern the test exists to pin.
Code Review by Qodo
1. Registered events may warn
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| 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; | ||
|
|
There was a problem hiding this comment.
1. Registered events may warn 🐞 Bug ≡ Correctness
If TypeMapper cannot be uniquely resolved (e.g., multiple accessible candidates), KnownTypeSymbols.TypeMapper becomes null and GetExplicitRegistrations returns an empty set, so IsExplicitlyRegistered will never suppress EVTC001 for TypeMap.Instance.AddType registrations. This can reintroduce false positives for explicitly-registered events in projects with ambiguous references.
Agent Prompt
## Issue description
`GetBestTypeByMetadataName` intentionally returns `null` when more than one accessible candidate exists. For `TypeMapper`, that cascades into `GetExplicitRegistrations` returning an empty set, which disables the explicit-registration suppression and can produce EVTC001 for events that are actually registered via `TypeMap.Instance.AddType(...)`.
## Issue Context
This PR removed all string-name fallbacks and made explicit-registration detection depend on successfully resolving `Eventuous.TypeMapper`. In ambiguous-reference scenarios (or other resolution failures), suppression now breaks.
## Fix Focus Areas
- src/Core/gen/Eventuous.Shared.Generators/EventUsageAnalyzer.cs[61-107]
## Suggested fix approach
- Keep the symbol-first approach, but add a narrowly-scoped fallback for explicit registration detection only:
- If `knownTypes.TypeMapper == null`, treat invocations as registrations when:
- method name is `AddType`, and
- containing type metadata name matches `WellKnownTypeNames.TypeMapper` (or containing type name/namespace match), and
- the invocation target is reachable via `TypeMap.Instance` (optional).
- Alternatively: change `GetBestTypeByMetadataName` for `TypeMapper` to select a deterministic candidate instead of returning null when multiple accessible candidates exist (e.g., prefer the one whose containing assembly name matches the Eventuous assembly reference set, or prefer the one that also contains `EventTypeAttribute`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Addressed in f28b6bf, with a stronger invariant than a fallback: if either EventTypeAttribute or TypeMapper fails 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 accessible Eventuous.TypeMapper definitions) means the user's own TypeMap.Instance.AddType calls fail with CS0433, so their build is broken regardless; the ILMerge-style case (one public + internalized copies) already resolves to the public candidate. Pinned by Should_stay_silent_when_event_type_attribute_cannot_be_resolved.
Test Results 44 files + 21 44 suites +21 13m 27s ⏱️ -15s Results for commit f28b6bf. ± Comparison against base commit a178a37. This pull request removes 26 and adds 16 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…handlers Addresses review findings on the Act/ActAsync analysis: - Types in the System namespace (List<object>, DateTime, ...) are never domain events, so creations of those inside handlers are not flagged - Creations inside lambdas passed to Act/ActAsync are reported only by the invocation traversal; the object-creation safety net skips them, so a sync handler no longer produces two identical diagnostics for the same creation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nnot resolve Without these two symbols neither annotations nor explicit registrations can be verified, so any EVTC001 diagnostic would be an unverifiable false positive. Skip registering the analysis actions entirely in that case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Closes #535
Problem
The EVTC001 analyzer flagged
objectitself as an unannotated domain event when a runtime-resolved payload (e.g.IEventReader.ReadEvents()→StreamEvent.Payload) was passed toState<T>.When(), breaking builds on the documented replay pattern.Reproducing it in tests surfaced two more latent defects, both previously hidden by the string-name fallbacks:
CommandHandlerBuilder,IDefineExecution,ICommandHandlerBuilder,IDefineStoreOrExecution) used metadata names without the arity suffix, soGetTypeByMetadataNamealways returned null and only the string fallback matched. The symbol comparison also compared constructed generics against unbound definitions.Act/ActAsynclambda arguments surface asIDelegateCreationOperation, which the analyzer didn't match — handler bodies were never traversed, so unannotated events created inActAsynchandlers produced no diagnostic at all.Changes
IsConcreteEventexcludesSystem.Object: an object-typed value carries a runtime-resolved event type, so there is nothing to annotate at the call siteIsFunctionalServiceActcomparesContainingType.OriginalDefinitionIDelegateCreationOperation(plain and conversion-wrapped)WellKnownTypeNames, resolved viaGetTypesByMetadataNameto handle the ambiguous-reference case whereGetTypeByMetadataNamereturns nullIsAggregate/IsState/IsEventHandlercollapsed into a sharedDerivesFromhelperTests
State<T>.When()must produce no diagnostic forobjectActAsynchandler must produce a diagnostic (pins the builder lookups behaviorally)TypeMap.Instance.AddType<T>()must be suppressed (pins theTypeMapperlookup)WellKnownTypeNames.Allmust resolve against the real Eventuous assemblies, so a type rename that misses the analyzer fails CI in the same PRFull solution builds with zero EVTC001 warnings; analyzer suite 5/5, core tests 29/29.
🤖 Generated with Claude Code