Reconcile TypeSpec properties with custom model bases - #11752
Reconcile TypeSpec properties with custom model bases#11752Wei Hu (live1206) wants to merge 16 commits into
Conversation
microsoft-github-policy-service
Bot
added
the
emitter:client:csharp
commit: |
|
No changes needing a change description found. |
Wei Hu (live1206)
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes a gap in the C# generator’s model-building pipeline when a model’s effective CLR base type is customized to a narrower base than the original TypeSpec base chain. It preserves TypeSpec-inherited properties that are no longer provided by the effective CLR base by materializing them on the derived model, ensuring those properties flow consistently through canonical view ordering and constructor parameter generation.
Changes:
- Extend
ModelProviderto build properties/fields/canonical view from a reconciled property set that includes original TypeSpec-base properties when a custom CLR base replaces the TypeSpec base. - Improve customization filtering to account for inherited CLR base members (including
CodeGenMemberrenames) when deciding which spec properties should be suppressed. - Add/adjust regression tests covering (1) non-public base members not suppressing derived public properties and (2) spec-base properties missing from a narrower custom base.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelCustomizationTests/GeneratesSpecBasePropertiesMissingFromNarrowerCustomBase/MockInputModel.cs | Adds custom base test fixture to simulate narrower CLR base with CodeGenMember-mapped members. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelCustomizationTests/GeneratesPropertyWhenCustomizedBasePropertyIsNotPublic/BaseModel.cs | Adds customization fixture for a non-public base property scenario. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelCustomizationTests.cs | Updates an existing test to validate non-public base members don’t suppress derived public members; adds new regression test for narrower custom base reconciliation. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs | Updates property customization filtering to incorporate inherited base members more safely (type + visibility aware). |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs | Introduces reconciled property planning (GetPropertiesToBuild) and uses it for properties and fields; aligns canonical view input with reconciled properties. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/CanonicalTypeProvider.cs | Allows canonical view ordering to be driven by an explicit canonical input property list (supports reconciled-property scenarios). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:696
propertiesToBuildnow contains the originalInputModelPropertyinstances from ancestor models, butTypeFactory.CreatePropertycaches only byInputProperty, not by enclosing provider. Once both the original base and this derived model are built, they therefore share onePropertyProvider, whoseEnclosingType, name collision handling, and last-contract lookup belong to whichever model was built first. This makes full generation order-dependent and prevents the reconciled property from reliably being owned by the derived model. Create a distinct provider per(InputProperty, enclosingType)(or clone the ancestor property plan) before adding it here.
foreach (var property in propertiesToBuild)
{
var isDiscriminator = IsDiscriminator(property);
// Skip discriminator properties that already exist in the base class
// Check both by C# property name and by serialized name to handle cases where
// the derived model has a discriminator with a different C# name but the same wire name
if (isDiscriminator && (baseProperties.ContainsKey(property.Name) || skippedBasePropertyNames.Contains(property.Name) || (property.SerializedName is not null && baseDiscriminatorSerializedNames.Contains(property.SerializedName))))
{
continue;
}
var outputProperty = CodeModelGenerator.Instance.TypeFactory.CreateProperty(property, this);
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelCustomizationTests.cs:1694
- This regression contains no discriminator property, so it does not exercise the discriminator-specific reconciliation path or the reported failure where a renamed inherited discriminator was materialized again as a generic
Typeproperty. Add an original-base discriminator plus a differently namedCodeGenMembermember on the custom base, then assert that only the inherited member is used in properties, constructors, and serialization.
var specBaseModel = InputFactory.Model(
"trackedResource",
properties: [
InputFactory.Property("id", InputPrimitiveType.String, isRequired: true, isReadOnly: true),
InputFactory.Property("location", InputPrimitiveType.String, isRequired: true),
InputFactory.Property("tags", InputFactory.Dictionary(InputPrimitiveType.String)),
InputFactory.Property("sku", InputPrimitiveType.String, isRequired: true),
InputFactory.Property("tier", InputPrimitiveType.String, isRequired: true, isReadOnly: true),
InputFactory.Property("capacity", new InputNullableType(InputPrimitiveType.Int32)),
InputFactory.Property("status", InputPrimitiveType.String, isRequired: true, isReadOnly: true),
],
|
I simplified the implementation in
The added accessibility, type/nullability, getter/setter, assembly, and constructor-capability policies were removed. Those broader improvements are tracked in #11765. - by copilot |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:748
- The fallback re-enters the owner-only
PropertyCacheeven whenbasePropertycame from an ancestor reconciled ontobasePropertyProvider. If that reconciled member was filtered from the canonical view (for example because the effective custom base supplies it), building a downstream override first caches the ancestor property with the intermediate enclosing type; the original owner later reuses that incorrectly scoped provider. Select the cached path only for properties directly owned by this provider, as is already done above for the current model.
outputProperty.BaseProperty = basePropertyProvider.CanonicalView.Properties.FirstOrDefault(p =>
ReferenceEquals(p.InputProperty, baseProperty))
?? CodeModelGenerator.Instance.TypeFactory.CreateProperty(baseProperty, basePropertyProvider);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:475
BaseTypeProvider.Propertiesonly covers generated members declared on the direct provider; it misses members inCustomCodeViewand members inherited from further CLR bases. In those cases this method still creates local additional-properties storage and a full-constructor parameter, while customization filtering removes the public generated property. Deserialization then populates the hidden local dictionary instead of the base member, andAdditionalPropertiesserialization sees no local property, so unknown values can be lost on round-trip. Walk the effective base-provider chain (including each provider's custom properties) before materializing the open-model contract.
// Do not materialize the open-model contract when the effective CLR base already exposes it.
var baseProvidesAdditionalProperties = BaseTypeProvider?.Properties.Any(property =>
property.IsAdditionalProperties ||
property.Name == AdditionalPropertiesHelper.DefaultAdditionalPropertiesPropertyName) == true;
return baseProvidesAdditionalProperties ? null : inheritedAdditionalProperties;
…e-properties # Conflicts: # packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/PropertyProvider.cs
There was a problem hiding this comment.
🟡 Changes recommended
Direct additional properties can retain hidden duplicate state, and downstream reconciliation can emit invalid overrides.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:455
- This bypasses custom-member matching for additional properties declared directly on the model. If the effective custom base already exposes
AdditionalProperties, normal filtering removes the generated property later, butAdditionalPropertyFieldsand the full-constructor parameter have already been created, leaving hidden duplicate state. ApplyHasCustomMemberto the direct type as well as the recovered inherited type.
if (_inputModel.AdditionalProperties is not null)
{
return _inputModel.AdditionalProperties;
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The reconciliation paths are consistently integrated and covered by focused constructor, factory, inheritance, cache, and serialization regressions.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| var properties = new List<InputModelProperty>(); | ||
| foreach (var property in model.Properties) | ||
| { | ||
| var propertyName = PropertyProvider.GetPropertyName(property, generatedTypeProvider); |
There was a problem hiding this comment.
Could we preserve the original base property's emitted contract name here instead of resolving every ancestor property against generatedTypeProvider? GetPropertyName consults the enclosing provider's type name and LastContractView, so this renames inherited properties when they are materialized. For example, if BaseModel declares the TypeSpec property baseModel, it previously surfaces on a child as the inherited BaseModelProperty; after that child switches to a narrower custom base, this emits BaseModel instead. Existing callers using child.BaseModelProperty then stop compiling. The reconciliation entry needs to retain the declaring provider or previous canonical name, applying a new child-name collision adjustment only when necessary.
--generated by Copilot
| return null; | ||
| } | ||
|
|
||
| var inheritedAdditionalProperties = _inputModel.GetSelfAndBaseModels() |
There was a problem hiding this comment.
Could this retain the model or provider that declared the inherited additional-properties contract, rather than only its InputType? ShouldUseObjectAdditionalProperties() checks only the derived model's LastContractView.Properties, and the Roslyn-backed provider exposes only directly declared members. If the old TypeSpec base exposed IDictionary<string, object> AdditionalProperties and the child inherited it, replacing that base causes this path to materialize the property as IDictionary<string, BinaryData> instead. That changes the public property and constructor signatures. The object back-compat decision should also consult the declaring ancestor's last contract.
--generated by Copilot
Fixes #11727 by reconciling properties from the original TypeSpec base hierarchy when a custom CLR base replaces it. Properties supplied by the effective CLR base—including members renamed with
CodeGenMember—remain inherited, while missing properties are materialized on the derived model and flow through its canonical view and constructors.This intentionally uses the existing name-based custom member matching policy. Broader semantic matching improvements are tracked in #11765.
Validation
- by copilot