fix(subscriptions): emit converter switch arms most-specific-first - #592
Conversation
) ConsumeContextConverterGenerator emitted one type-pattern switch arm per discovered message type in discovery order. When one type was a supertype of another (a base [EventType] record, or an interface), its arm could precede and subsume the more specific one, failing the build with CS8510. Where it did compile, a derived message was wrapped in the base type's context, unlike the reflection fallback which uses the exact runtime type. Rank each candidate by its number of supertypes (base classes plus all interfaces), which is strictly greater for a subtype than for any of its supertypes, and emit arms by that rank descending, then by name for deterministic output. Base-class depth alone is not enough: all interfaces have depth zero, so a base interface could still precede a derived one. Add generator tests covering class and interface inheritance. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR Summary by QodoEmit converter switch arms most-specific-first
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
![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: bb0c4e7355
ℹ️ 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".
| static int GetSpecificity(ITypeSymbol symbol) { | ||
| var count = symbol.AllInterfaces.Length; | ||
|
|
||
| for (var baseType = symbol.BaseType; baseType != null; baseType = baseType.BaseType) count++; |
There was a problem hiding this comment.
When candidates are related through generic variance—for example, IEvent<out T> used as both IMessageConsumeContext<IEvent<Base>> and IMessageConsumeContext<IEvent<Derived>>—the two constructed interfaces have identical base/interface counts. The name tiebreak can therefore emit IEvent<Base> first, which subsumes the IEvent<Derived> pattern and still produces CS8510. Determine ordering from actual implicit reference conversions/subtyping rather than relying solely on the supertype count.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified with a generator test: IEnvelope<Zebra> and IEnvelope<Animal> (with IEnvelope<out T>) get the same supertype count, and the name tie-break emitted the broader arm first, which fails with CS8510. The name tie-break could also break a project that compiled before this PR, when arms simply followed declaration order.
Fixed in a965b3e: the name tie-break is gone and the sort is stable, so equally specific types keep their discovery order. That is still deterministic, and anything that compiled before still compiles. Added Should_keep_discovery_order_for_types_of_equal_specificity as a regression test, and reworded the comment that claimed the rank alone guarantees no arm is subsumed.
Not doing the conversion-based topological ordering here. Variance-related message types declared broader-first failed before this PR too, so it is not a regression. Ordering them correctly needs implicit-conversion checks across all candidate pairs, on every generator run, over every [EventType] type in all referenced assemblies, and nobody has reported hitting this. It can be revisited if it comes up.
…er arms Types related only through generic variance (IEnvelope<Zebra> and IEnvelope<Animal> with IEnvelope<out T>) have the same number of supertypes, so the specificity rank cannot order them. Breaking such ties by name could emit the broader arm first and fail with CS8510, including in projects that compiled before, when arms followed declaration order. Drop the name tie-break and rely on the stable sort, so equally specific types keep their discovery order. That is still deterministic, and anything that compiled before the arms were ranked still compiles. Reword the comments that claimed the rank alone guarantees no arm is subsumed, and add a regression test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
Fixes #589
Problem
Since 0.16.4 (#542),
ConsumeContextConverterGeneratordiscovers all[EventType]types and emits one type-pattern switch arm per type, in discovery order. When one message type is a supertype of another, its arm can come first and subsume the more specific arm, so the consuming project fails to build withCS8510: The pattern is unreachable. Namespace traversal goes outer to inner, so a base record inFooalways lands before a derived record inFoo.Bar.It is not limited to
[EventType]discovery: any scanned usage that brings in both a base and a derived type (for exampleOn<Base>next toOn<Derived>) takes the same path.It is also a correctness problem. The reflection fallback in
MessageConsumeContextConverterconverts using the message's exact runtime type, so the generated converter only agrees with it when the most specific arm wins.Fix
Each candidate now carries a specificity rank: the number of its base classes plus all of its interfaces. A subtype always has strictly more supertypes than any of its supertypes, so emitting arms by rank descending keeps the arm of a type ahead of the arms of its supertypes.
Types of equal rank keep their discovery order (the sort is stable), which is still deterministic. This matters for types related only through generic variance, such as
IEnvelope<Zebra>andIEnvelope<Animal>withIEnvelope<out T>: they have the same rank, so the rank cannot order them. Keeping discovery order means anything that compiled before this change still compiles. Variance-related message types declared broader-first still fail with CS8510, as they did before; ordering those correctly needs implicit-conversion checks across all candidate pairs on every generator run, which is not worth it until someone actually hits it.The issue suggested sorting by base-class depth. That covers classes but not interfaces: every interface has depth zero, so
IAnyEventcould still precedeIBookingEvent : IAnyEventand hit the same error. Counting interfaces as well covers both.Tests
The generator had no tests.
ConsumeContextConverterGeneratorTestsinEventuous.Tests.Subscriptionsruns the generator throughCSharpGeneratorDriverand asserts that the output compiles without errors and that arms are ordered most-specific-first, for:All three failed with CS8510 before the corresponding fix. The test project now references the generator project and the CodeAnalysis packages.
Verified locally on net10.0 only (137 tests in the project pass); net8.0 and net9.0 runtimes are not installed on my machine, so those rely on CI.
Notes
No public API change, so no docs or plugin updates are needed.
Not addressed here:
GetTypeSyntaxprefixes keyword types withglobal::, soIMessageConsumeContext<string>generatesglobal::string(CS1041). It predates this change and deserves its own issue.🤖 Generated with Claude Code