feat(utilities): TypeDeclarationInfo snapshot + tree-bound location round-trip - #150
Conversation
…on round-trip TypeDeclarationInfo / ContainingTypeInfo: cacheable, value-equatable snapshot of a type declaration for incremental generators — namespace, declaration keyword, generic clause, partial-ness, and the containing-type chain captured as per-level declarations (keyword + name + generic clause + partial-ness), so emission into targets nested in generic or struct containers compiles. BeginDeclaration(IndentedStringBuilder) emits the nested partial wrapper as a disposable scope; IsFullyPartial gates emission for diagnostics. LocationInfo.ToLocation(SyntaxTree?) / DiagnosticInfo.ToDiagnostic(SyntaxTree?): bind recreated Locations to a live syntax tree for IDE navigation/squiggles, with a span-bounds guard falling back to the path-based location. Also fixes GetGenericParameterClause: it keyed on IsGenericType, which Roslyn defines as true when any containing type has type parameters, producing a bogus "<>" clause for non-generic nested types. Now keyed on TypeParameters.Length. Surfaced by the new nested-chain tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting Summary by CodeRabbit
WalkthroughThis PR adds tree-bound location resolution for ChangesTree-bound locations and type declaration snapshot
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DiagnosticInfo
participant LocationInfo
Caller->>DiagnosticInfo: ToDiagnostic(syntaxTree)
DiagnosticInfo->>LocationInfo: ToLocation(syntaxTree)
alt tree non-null and span fits
LocationInfo-->>DiagnosticInfo: tree-bound Location
else fallback
LocationInfo-->>DiagnosticInfo: path-based Location
end
DiagnosticInfo-->>Caller: Diagnostic
Possibly related PRs
Comment |
![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: 259b24179c
ℹ️ 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".
| blockCount++; | ||
| } | ||
|
|
||
| builder.AppendLine($"partial {Keyword} {DisplayName}"); |
There was a problem hiding this comment.
When a generator uses this helper for a public partial target (or any target/container with explicit accessibility), this emits a partial declaration with no accessibility modifier. C# partial type parts must use the same accessibility, so the generated file conflicts with the user's declaration (CS0262) before any generated members can compile; capture and replay the accessibility for the target and containing wrappers.
Useful? React with 👍 / 👎.
| type.ContainingNamespace is { IsGlobalNamespace: false } ns ? ns.ToDisplayString() : null, | ||
| GetKeyword(type), | ||
| type.Name, | ||
| type.GetGenericParameterClause(), |
There was a problem hiding this comment.
For a partial interface such as partial interface I<out T> (or a target nested in one), this captures the generic clause via GetGenericParameterClause(), which records only T; BeginDeclaration later emits partial interface I<T>. Partial declarations must use the same variance modifiers, so consumers hit CS1067 for variant interfaces; include the in/out modifier in the stored clause for interface type parameters.
Useful? React with 👍 / 👎.
| return syntaxTree is not null && Span.End <= syntaxTree.Length | ||
| ? Location.Create(syntaxTree, Span) |
There was a problem hiding this comment.
When LocationInfo was created from Location.None (for example, DiagnosticInfo.Create(descriptor, symbol) falls back for symbols without source locations), its span is the default [0,0], so this condition succeeds for any supplied tree and turns a no-location diagnostic into a squiggle at the start of that file. This affects callers that pass a tree to rebind a mixed set of diagnostics; fall back unless the stored location actually identifies the supplied source tree.
Useful? React with 👍 / 👎.
| return new TypeDeclarationInfo( | ||
| type.ContainingNamespace is { IsGlobalNamespace: false } ns ? ns.ToDisplayString() : null, | ||
| GetKeyword(type), | ||
| type.Name, |
There was a problem hiding this comment.
For a legal declaration such as partial class @class { } (or an escaped containing type), INamedTypeSymbol.Name is class, so the snapshot is later emitted as partial class class. That generated source does not parse, which breaks generators using this helper on escaped keyword identifiers; capture the syntax token text or format identifiers with C# keyword escaping before emission.
Useful? React with 👍 / 👎.
| { | ||
| { IsRecord: true, TypeKind: TypeKind.Struct } => "record struct", | ||
| { IsRecord: true } => "record", | ||
| { TypeKind: TypeKind.Struct } => "struct", |
There was a problem hiding this comment.
When the target or a containing type is declared as partial ref struct, this maps the symbol to plain struct, so BeginDeclaration emits partial struct S. Ref structs are a distinct partial type kind, and the generated part will not merge with the user's ref struct declaration (CS0261); include IsRefLikeType when capturing the keyword.
Useful? React with 👍 / 👎.
| GetKeyword(type), | ||
| type.Name, | ||
| type.GetGenericParameterClause(), | ||
| IsDeclaredPartial(type), |
There was a problem hiding this comment.
For a file partial class C, this stores only that the declaration is partial, so IsFullyPartial can pass and BeginDeclaration emits a normal partial class C into the generated file. File-local types are scoped to their declaring source file, so the generated declaration is a different non-file type rather than an augmentation of the target; treat file-local targets as unsupported instead of reporting them as fully partial.
Useful? React with 👍 / 👎.
1 participant
Summary
Two additions inspired by the
Microsoft.Agents.AI.Workflows.Generatorsmodel layer — plus one pre-existing bug the new tests surfaced.1.
TypeDeclarationInfo+ContainingTypeInfo+TypeDeclarationScope(Models/)Cacheable, value-equatable snapshot of a type declaration for incremental generators: namespace, declaration keyword (
class/struct/record/record struct/interface), generic parameter clause,partial-ness, and the containing-type chain.The chain is stored as per-level declarations (keyword + name + generic clause + partial-ness), not a flat name string. This deliberately fixes the flaw in the Microsoft version, whose
ContainingTypeChainstring loses container type kind and generics — its emittedpartial class Outer {wrapper fails to compile when the container is astructor generic.From(INamedTypeSymbol)— snapshot in the transform stage (cache-safe, no symbol retention)BeginDeclaration(IndentedStringBuilder)— emits namespace + full nestedpartialwrapper as a disposable scope, house-styleIsFullyPartial— the report-a-diagnostic-instead-of-emitting gate2. Tree-bound location round-trip
LocationInfo.ToLocation(SyntaxTree?)andDiagnosticInfo.ToDiagnostic(SyntaxTree?)bind the recreatedLocationto a live syntax tree when available (IDE navigation + live squiggles;IsInSource == true), falling back to the existing path-based location. A span-bounds guard makes a changed/wrong tree degrade to the fallback instead of throwing.3. Bug fix:
GetGenericParameterClauseon nested typesThe helper keyed on
INamedTypeSymbol.IsGenericType, which Roslyn defines as “this type or some containing type has type parameters” — so a non-generic type nested inOuter<T>produced a bogus"<>"clause. Now keyed onTypeParameters.Length. Found by the new per-level chain tests.Verification
CI=true dotnet build ANcpLua.Roslyn.Utilities.slnx -c Release— 0 errorsdotnet test— 208 passed, 0 failed, including:Deep.Outer<T>.Middle.Inner<U>(class→struct→record struct)Status: complete-and-verified. Touches
src/**,tests/**, andREADME.md→ publish gate opens; merge auto-bumps to 2.2.30 via trusted publishing.🤖 Generated with Claude Code