fix(http-client-csharp): preserve union metadata when back compat preserves a property type - #11839
Conversation
…serves a property type Last contract types are Roslyn-backed and carry no TypeSpec union metadata, so preserving one dropped the union item types of the current type. That made the union variant models look unreferenced, so they were removed from the output and their doc references degraded to plain text. Restore the union item types from the current type onto the preserved type, including through list and dictionary element types. The emitted C# is unchanged because a union is always written as BinaryData. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5c237fe4-3c18-45b7-82b3-21b41cb1a714
microsoft-github-policy-service
Bot
added
the
emitter:client:csharp
commit: |
There was a problem hiding this comment.
🟡 Changes recommended
RestoreUnionItemTypes currently fails to propagate restored union metadata through nested collections because it only rebuilds containers when the immediate element becomes a union.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a metadata-loss bug in the http-client-csharp generator’s back-compat path: when a last-contract property type is preserved, union item metadata from the current TypeSpec model is now restored onto the preserved BinaryData (including through list/dictionary element types) so union variant models remain referenced and aren’t pruned/internalized.
Changes:
- Add
CSharpTypeExtensions.RestoreUnionItemTypesand invoke it fromModelProvider.GetPropertyTypeForBackCompatibility. - Add regression coverage for union collection properties retaining union item metadata, plus a negative/control case where the preserved type is non-
BinaryData. - Document the behavior in
generator/docs/backward-compatibility.md.
File summaries
| File | Description |
|---|---|
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/CSharpTypeExtensions.cs | Adds union-metadata restoration helper for preserved last-contract types. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs | Applies union-metadata restoration after back-compat type selection/nullability handling. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs | Adds tests validating union metadata is preserved for list/dictionary properties and dropped for non-BinaryData. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_UnionCollectionPropertiesRetainUnionItemTypes/MockInputModel.cs | Test asset for last-contract compilation with union-collection shapes. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_UnionPropertyReplacedWithNonBinaryDataTypeDropsUnionItemTypes/MockInputModel.cs | Test asset for last-contract compilation where preserved type is object. |
| packages/http-client-csharp/generator/docs/backward-compatibility.md | Documents union metadata preservation behavior and its impact on reachability/doc refs. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
No changes needing a change description found. |
…adata is restored Checking the immediate element for IsUnion dropped restored metadata for nested collections such as IReadOnlyList<IReadOnlyList<BinaryData>>, since the outer element is a collection rather than a union. Use reference identity to detect that anything below the container changed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5c237fe4-3c18-45b7-82b3-21b41cb1a714
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, matches the documented intent, and includes targeted regression coverage (including a negative control) without affecting emitted C# output.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
…cope helper to internal - revert the backward-compatibility.md additions - make RestoreUnionItemTypes internal - compare generated output against TestData expected files with XML docs enabled, which captures the restored union item crefs directly Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5c237fe4-3c18-45b7-82b3-21b41cb1a714
There was a problem hiding this comment.
🟢 Approval recommended
The implementation handles nested collections, preserves reference semantics, and includes focused positive and negative regression coverage.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation preserves emitted types while comprehensive regression tests cover collections, nesting, and incompatible preserved types.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…ailing generic argument Drop the explicit list/dictionary branches and the Arguments[0] index in favor of replacing the trailing generic argument, which is the element for both shapes and matches how CSharpType.ElementType resolves it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5c237fe4-3c18-45b7-82b3-21b41cb1a714
There was a problem hiding this comment.
🟡 Changes recommended
Array-shaped last-contract properties still lose union metadata, and the promised documentation update is absent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:788
- The PR description says this behavior is documented in
generator/docs/backward-compatibility.md, but that file has no union-metadata guidance and is not changed by this PR. Please add the promised Model Properties documentation (or correct the PR description if documentation is intentionally out of scope).
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
…ion restoration Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5c237fe4-3c18-45b7-82b3-21b41cb1a714
There was a problem hiding this comment.
🔵 Needs a closer look
The backward-compatibility documentation promised by the PR description is missing.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:788
- The PR says this behavior is documented, but
generator/docs/backward-compatibility.md:243-297still discusses property preservation and nullable references without mentioning union metadata. Please add the promised union-preservation documentation (or remove that claim from the PR description).
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| /// property is just <see cref="BinaryData"/> in metadata. When a last-contract type is preserved for | ||
| /// back compatibility, the union item types of the current TypeSpec type would otherwise be dropped. | ||
| /// That makes the union variant models appear unreferenced, so they get removed (or internalized) from | ||
| /// the output and their doc references degrade to plain text. Restoring the metadata keeps the emitted |
There was a problem hiding this comment.
But shouldn't these variant models be dropped if they are internalized? I don't think the docs should reference internal types either way.
There was a problem hiding this comment.
Keeping internal union variants looks deliberate rather than incidental. ProviderReferenceMapAnalyzerTests.BinaryDataUnionPropertyDoesNotPublicizeInternalUnionMembers, added in #11288 ("Fix C# reference-map accessibility for hidden union types"), covers exactly this shape — an internal variant reachable only through a public BinaryData union property:
Assert.IsTrue(internalVariant.DeclarationModifiers.HasFlag(TypeSignatureModifiers.Internal));
Assert.IsFalse(internalVariant.DeclarationModifiers.HasFlag(TypeSignatureModifiers.Public));
Assert.IsTrue(ProviderReferenceMapAnalyzer.ShouldWriteProvider(internalVariant));So the analyzer intentionally keeps the variant while refusing to publicize it, and UnionItemTypeReferenceKind exists as the opt-out — the sibling test MetadataOnlyUnionPropertyDoesNotReferenceUnionMember asserts that a MetadataOnly union does not reference its variants and they get dropped. I don't know the original functional motivation, but it was an explicit decision.
Worth noting for the regen that prompted this PR: the AI library was the outlier before, not after. Any library with a public BinaryData union over internal variants already generates them today with no back compat involved. Azure.AI.Projects.Agents only differed because back compat silently stripped the union metadata and sent those models down the unreferenced path. This change makes the back compat path match the mainline path, so back-compat libraries stop diverging from freshly generated ones.
On the docs: agreed, and it is a separate pre-existing gap — PropertyDescriptionBuilder.GetUnionTypesDescriptions emits a cref for every variant with no accessibility filter, independent of back compat. I removed the XML doc from this method since it justified the change partly on that basis. Happy to file a follow-up issue for skipping internal types in union docs (and revisiting whether internal variants should be kept at all) if you think that is worth pursuing.
🤖 Generated by Jorge's Copilot
There was a problem hiding this comment.
We should probably investigate why we have tests to ensure we don't remove internalized union variant types.
There was a problem hiding this comment.
I checked the history. BinaryDataUnionPropertyDoesNotPublicizeInternalUnionMembers was introduced on July 17 in #11288 as part of the provider-reference-map migration. It deliberately asserts both that the variant stays internal and that it is still written, but I could not find a concrete runtime or product scenario requiring an internal model to be emitted when its only reference is erased BinaryData union metadata. It appears to have preserved migration behavior rather than documented a separately established API policy.
I opened #11845 to address the two policy concerns separately: internal variants referenced only through BinaryData union metadata should be removable, and public XML docs should not reference internal or removed variants. That work should split/replace the current test so it independently covers metadata-only removal and retention of an internal variant that has a genuine implementation reference.
I think that policy change is beyond this PR. Rather than removing the existing reference-map test here without changing the behavior it covers, #11839 can stay focused on preserving union metadata through back-compat replacement; the follow-up can then change how the analyzer and documentation consume that metadata.
- by copilot
The remarks justified the change partly by union doc references, which is a separate concern. The inline comments and the call site cover the rationale. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5c237fe4-3c18-45b7-82b3-21b41cb1a714
There was a problem hiding this comment.
🟡 Changes recommended
The tests omit the production reachability-analysis path, and the promised documentation update is absent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
| var file = new TypeProviderWriter(modelProvider).Write(); | ||
| Assert.AreEqual(Helpers.GetExpectedFromFile(), file.Content); |
| // Last-contract types are Roslyn-backed and carry no union metadata. Restore it from the current | ||
| // TypeSpec type so the union variant models stay referenced and are not removed as unused. | ||
| return compatibleType.RestoreUnionItemTypes(currentType); |
Summary
Last contract types are read from Roslyn symbols, which have no notion of a TypeSpec union — a union property is just
BinaryDatain metadata. When API compatibility processing preserved a last contract property type, the union item types from the current TypeSpec type were dropped.That has a real, non-obvious consequence:
ProviderReferenceMapAnalyzertreats union item types as references. Once the union metadata is gone, the union variant models look unreferenced, so they are removed (or internalized) from the output and their doc references degrade from<see cref="..."/>to<c>global::Namespace.Type</c>.This was the mechanism behind the surprise churn in
Azure.AI.Projects.Agentsin Azure/azure-sdk-for-net#62649, where 20+ToolChoice*models reappeared after the nullability fix in #11828 stopped one particular replacement from happening. The underlying metadata loss is still reachable in other shapes — most notably collections of unions, where the last contract and current types differ by more than nullability (e.g.IReadOnlyList<BinaryData>vsIList<BinaryData>).Changes
CSharpTypeExtensions.RestoreUnionItemTypes, which copies the union item types (and their reference kind) from the current TypeSpec type onto the preserved last contract type, recursing through list and dictionary element types.ModelProvider.GetPropertyTypeForBackCompatibility.generator/docs/backward-compatibility.md.The emitted C# is unchanged, since a union is always written as
BinaryData. Only the internal metadata that keeps the variant models reachable is restored. When the preserved type is notBinaryData(e.g. the last contract usedobject), the metadata legitimately cannot be carried over and the variant models are no longer referenced by that property.Tests
BackCompat_UnionCollectionPropertiesRetainUnionItemTypes— list and dictionary union properties keep their union item types when the last contract collection shape is preserved, and the generated C# is unchanged. Verified this fails without the fix.BackCompat_UnionPropertyReplacedWithNonBinaryDataTypeDropsUnionItemTypes— control for the non-BinaryDatapreserved type.Validation
dotnet test Microsoft.TypeSpec.Generator.Tests.csproj— 2194 passed.dotnet test Microsoft.TypeSpec.Generator.ClientModel.Tests.csproj— 1473 passed / 125 failed; the 125 failures are pre-existingSystem.IO.IOExceptiontest-asset failures in this worktree and reproduce identically on a clean baseline.contributes to : #11814