[http-client-csharp] Preserve last-contract model base types - #11818
[http-client-csharp] Preserve last-contract model base types#11818Wei Hu (live1206) wants to merge 18 commits into
Conversation
microsoft-github-policy-service
Bot
added
the
emitter:client:csharp
commit: |
|
No changes needing a change description found. |
|
Regen: Azure/azure-sdk-for-net#62636 |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core model inheritance resolution and constructor-chaining constraints in the generator, which is high-impact even with good regression coverage.
Pull request overview
This PR extends the http-client-csharp generator’s last-contract back-compat logic to preserve a model’s previously shipped CLR base type when the current TypeSpec base hierarchy no longer contains it, while guarding against cases that would produce invalid C# (e.g., conflicting partial bases or inaccessible constructor chaining).
Changes:
- Update
ModelProviderbase-type selection to restore last-contract bases when safe, retain promoted bases when already derived, and resolve preserved bases against the current build. - Add constructor-accessibility checks for symbol-backed base types and emit a new diagnostic for incompatible custom-base conflicts.
- Add targeted regression tests and test data for generated, referenced, and framework base-type preservation scenarios.
File summaries
| File | Description |
|---|---|
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_ReferencedLastContractBaseWithInternalParameterlessConstructorIsNotRestored(LastContract)/Models.cs | Adds last-contract baseline source for an external base with an internal parameterless ctor (should not be restored). |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_NonGeneratedLastContractBaseWithoutParameterlessConstructorIsNotRestored(LastContract)/Models.cs | Adds last-contract baseline source for a non-generated base lacking a parameterless ctor (should not be restored). |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_NonGeneratedLastContractBaseWithoutParameterlessConstructorIsNotRestored(Current)/ExternalBase.cs | Adds current-build external base type used to validate constructor-chaining constraints. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_LastContractBaseRestoresInheritedProperties/Models.cs | Adds baseline types to validate inherited properties remain inherited and are not duplicated. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_CustomBaseTakesPrecedenceOverDifferentLastContractBase(LastContract)/Models.cs | Adds last-contract baseline for a previous base used in the custom-base precedence scenario. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_CustomBaseTakesPrecedenceOverDifferentLastContractBase(Current)/Models.cs | Adds current-build custom base partial to validate partial-base conflict handling. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_BaseTypeChangePreservesNonGeneratedLastContractBaseType/DerivedModel.cs | Adds baseline derived model inheriting from a framework type (e.g., System.Exception). |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/BackCompat_BaseTypeChangePreservesLastContractBaseType/Models.cs | Adds baseline types to validate restoring a previously shipped generated base. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs | Adds regression tests covering base restoration, promoted bases, inherited properties, custom-base conflicts, and ctor-accessibility constraints. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/DiagnosticCodes.cs | Introduces incompatible-backcompat-base-type diagnostic code. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/NamedTypeSymbolProvider.cs | Adds HasAccessibleParameterlessConstructor to validate ctor accessibility across assembly boundaries. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs | Implements last-contract base-type preservation logic and resolves preserved bases against the current compilation/providers. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/Emitter.cs | Adds display label for the new back-compat change category. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/EmitterRpc/BackCompatibilityChangeCategory.cs | Adds ModelBaseTypePreserved category for back-compat reporting. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core back-compat inheritance selection logic with broad API-surface implications that warrants final human verification beyond the added regressions.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues in base-type resolution/traversal that can incorrectly block restoring an available last-contract base or mis-detect base hierarchies in nested/generic scenarios.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/ScmModelProvider.cs:95
- IsInBaseTypeHierarchy uses type.FullyQualifiedName for cycle detection, but CSharpType.FullyQualifiedName omits the full declaring-type chain (and generic arity), so unrelated nested/generic types can collide and prematurely stop traversal. Prefer a unique key such as namespace + ClrMetadataName.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:366 - IsInBaseTypeHierarchy uses type.FullyQualifiedName to detect cycles, but CSharpType.FullyQualifiedName only includes the immediate declaring type (and omits generic arity/arguments), so different nested/generic types can collide and prematurely stop traversal. Use a stable unique key such as namespace + ClrMetadataName instead.
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new base-type hierarchy/resolution logic relies on CSharpType.AreNamesEqual in ways that can misidentify multi-level nested types, risking incorrect base restoration decisions unless metadata-identity matching is used consistently.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:435
- Same nested-type resolution issue in the second CSharpTypeMap scan: using AreNamesEqual can select the wrong provider for multi-level nested types. Use metadata identity matching here as well to keep the two scans consistent and correctly disambiguate nested declaring-type chains.
if (provider is not null &&
provider.Type.AreNamesEqual(type) &&
TryUseProviderAsBase(provider, out resolvedProvider))
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:418
- TryResolveTypeInCurrentBuild matches candidate providers using CSharpType.AreNamesEqual. For multi-level nested types, AreNamesEqual can conflate types because FullyQualifiedName drops outer declaring types. This can cause resolving the wrong provider from CSharpTypeMap when nested types share the same immediate declaring type name. Match on metadata identity (outermost namespace + ClrMetadataName) instead.
if (provider is not null &&
provider.Type.AreNamesEqual(type) &&
TryUseProviderAsBase(provider, out resolvedProvider))
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core model base-type resolution/back-compat behavior in the generator with potentially broad API/compat impact, so it warrants final human validation beyond automated review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/ScmModelProvider/ScmModelProviderTests.cs:333
- This Roslyn compilation assertion only checks DiagnosticSeverity.Error. Since generator projects build with TreatWarningsAsErrors=true, consider failing the test on warnings as well so issues like member hiding (CS0108) are caught here before they become build breaks.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs:613 - The Roslyn compilation assertion only checks DiagnosticSeverity.Error. This can miss warnings like CS0108 (member hiding) that will fail real builds because generator projects treat warnings as errors (TreatWarningsAsErrors=true). Consider asserting that there are no warnings as well so the regression better matches actual build behavior.
This issue also appears on line 652 of the same file.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs:655
- This compilation check filters only DiagnosticSeverity.Error, so warnings (which are treated as errors in the generator build) can slip through and make the regression less representative. Consider including DiagnosticSeverity.Warning in the assertion.
Assert.That(
compilation.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error),
Is.Empty,
"The generated model hierarchy should compile after the incompatible symbol-backed base restoration is skipped");
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The base-restoration property-collision guard currently treats private base properties as collisions, which can incorrectly block restoring a last-contract base type.
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:391
- HasDirectPropertyNameCollision treats private properties on a symbol-backed/base type as collisions. In C#, private base members are not inherited, so they cannot cause the CS0108 hiding warning/error and shouldn’t block restoring the last-contract base; this can incorrectly skip base restoration and break assignability compatibility.
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The change alters core model inheritance/back-compat behavior with many edge-case guards (resolution, constructors, collisions, discriminator hierarchies) that warrants final human review despite strong regression coverage.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
latest regen: Azure/azure-sdk-for-net#62687 |
There was a problem hiding this comment.
🟡 Changes recommended
There is a C# compile issue in ScmModelProvider where an override reduces method accessibility, which must be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces non-trivial base-type reconciliation logic that can affect broad model/serialization behaviors, so it warrants final human review despite strong regression coverage.
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
This intentionally does not materialize properties introduced only by a displaced, newer TypeSpec base. That broader reconciliation can be added when an SDK use case requires it.
Fixes #11816
Validation