Skip to content

[csharp] Derive ModelProvider.BaseModelProvider from BaseType #10486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ArcturusZhang wants to merge 1 commit into
base: main
Choose a base branch
from
+104 −54
Draft

[csharp] Derive ModelProvider.BaseModelProvider from BaseType #10486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 56 additions & 54 deletions .../http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,53 @@ protected override string BuildNamespace() => string.IsNullOrEmpty(_inputModel.N

protected override CSharpType? BuildBaseType()
{
return BaseModelProvider?.Type;
// BuildBaseType is the single source of truth for the model's base. Both BaseType
// (in TypeProvider) and BaseModelProvider read from it, so any subclass override
// automatically keeps the two in sync.
//
// Resolution order:
// 1. CustomCodeView (hand-written `partial class Foo : Bar` wins over spec).
// 2. _inputModel.BaseModel (spec-declared base).
//
// For the CustomCodeView path we may need to resolve a base type whose namespace
// Roslyn could not determine (because the parent is generated, not hand-written).
if (CustomCodeView?.BaseType is { } customBase)
{
if (string.IsNullOrEmpty(customBase.Namespace))
{
// Cheap check: the base model may already be created and registered under the right name.
if (CodeModelGenerator.Instance.TypeFactory.TypeProvidersByName.TryGetValue(
customBase.Name, out var resolvedProvider) &&
resolvedProvider is ModelProvider resolvedModel)
{
return resolvedModel.Type;
}

// Force-create all input models so that visitors run (which may rename models
// via TypeProvider.Update) and TypeProvidersByName is fully populated.
// This is a no-op for models that have already been created.
foreach (var model in CodeModelGenerator.Instance.InputLibrary.InputNamespace.Models)
{
CodeModelGenerator.Instance.TypeFactory.CreateModel(model);
}

if (CodeModelGenerator.Instance.TypeFactory.TypeProvidersByName.TryGetValue(
customBase.Name, out resolvedProvider) &&
resolvedProvider is ModelProvider resolvedAfterCreate)
{
return resolvedAfterCreate.Type;
}
}

return customBase;
}

if (_inputModel.BaseModel == null)
{
return null;
}

return CodeModelGenerator.Instance.TypeFactory.CreateModel(_inputModel.BaseModel)?.Type;
}

protected override TypeProvider[] BuildSerializationProviders()
Expand Down Expand Up @@ -291,63 +337,19 @@ private static bool IsDiscriminator(InputProperty property)

private ModelProvider? BuildBaseModelProvider()
{
// consider models that have been customized to inherit from a different generated model
if (CustomCodeView?.BaseType != null)
{
var baseType = CustomCodeView.BaseType;

// If the custom base type doesn't have a resolved namespace, then try to resolve it from the input model map.
// This will happen if a model is customized to inherit from another generated model, but that generated model
// was not also defined in custom code so Roslyn does not recognize it.
if (string.IsNullOrEmpty(baseType.Namespace))
{
// Cheap check: the base model may already be created and registered under the right name.
if (CodeModelGenerator.Instance.TypeFactory.TypeProvidersByName.TryGetValue(
baseType.Name, out var resolvedProvider) &&
resolvedProvider is ModelProvider resolvedModel)
{
return resolvedModel;
}

// Force-create all input models so that visitors run (which may rename models
// via TypeProvider.Update) and TypeProvidersByName is fully populated.
// This is a no-op for models that have already been created.
foreach (var model in CodeModelGenerator.Instance.InputLibrary.InputNamespace.Models)
{
CodeModelGenerator.Instance.TypeFactory.CreateModel(model);
}

if (CodeModelGenerator.Instance.TypeFactory.TypeProvidersByName.TryGetValue(
baseType.Name, out resolvedProvider) &&
resolvedProvider is ModelProvider resolvedAfterCreate)
{
return resolvedAfterCreate;
}
}

// Try to find the base type in the CSharpTypeMap
if (baseType != null && CodeModelGenerator.Instance.TypeFactory.CSharpTypeMap.TryGetValue(
baseType,
out var customBaseType) &&
customBaseType is ModelProvider customBaseModel)
{
return customBaseModel;
}

// If the custom base type has a namespace (external type), we don't return it here
// as it's handled by BuildBaseTypeProvider() which returns a TypeProvider
if (!string.IsNullOrEmpty(baseType?.Namespace))
{
return null;
}
}

if (_inputModel.BaseModel == null)
// Thin wrapper over BaseType. BaseType is the single source of truth (resolved in
// BuildBaseType), so BaseModelProvider always agrees with it. If BaseType points at
// a non-generated type (subclass override, hand-written external base, system type),
// the CSharpTypeMap lookup misses and BaseModelProvider is null - which is correct,
// because callers walk it to discover generated parent properties/fields/etc.
if (BaseType is not { } baseType)
{
return null;
}

return CodeModelGenerator.Instance.TypeFactory.CreateModel(_inputModel.BaseModel);
return CodeModelGenerator.Instance.TypeFactory.CSharpTypeMap.TryGetValue(baseType, out var baseProvider)
? baseProvider as ModelProvider
: null;
}

private List<FieldProvider> BuildAdditionalPropertyFields()
Expand Down
48 changes: 48 additions & 0 deletions ...enerator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,54 @@ public void BuildBaseType()
Assert.AreEqual(baseModel!.Type, derivedModel!.Type.BaseType);
}

[Test]
public void OverridingBuildBaseTypeKeepsBaseModelProviderConsistent()
{
// A derived ModelProvider that overrides BuildBaseType to point at an external
// (hand-written) base type — emulating what downstream emitters such as the Azure
// mgmt/provisioning generators do when they replace the spec inheritance with a
// hand-written base class. Before the fix, BaseType pointed at the override but
// BaseModelProvider was still computed from InputModelType.BaseModel, so visitors
// that walked BaseModelProvider saw a parent that wasn't actually in the C#
// inheritance chain.
var inputBase = InputFactory.Model(
"specBaseModel",
usage: InputModelTypeUsage.Input,
properties: [InputFactory.Property("baseProp", InputPrimitiveType.String)]);
var inputChild = InputFactory.Model(
"childModel",
usage: InputModelTypeUsage.Input,
properties: [InputFactory.Property("childProp", InputPrimitiveType.String)],
baseModel: inputBase);

MockHelpers.LoadMockGenerator(inputModelTypes: [inputBase, inputChild]);

var externalBase = new CSharpType(typeof(object));
var childProvider = new ModelProviderWithExternalBase(inputChild, externalBase);

// BaseType honors the override.
Assert.AreEqual(externalBase, childProvider.BaseType);

// BaseModelProvider is consistent with BaseType: since the external base is not a
// generated ModelProvider, BaseModelProvider must be null. Before the fix it was
// still resolving to the spec base ("SpecBaseModel"), causing properties to be
// double-counted by visitors that walked BaseModelProvider.
Assert.IsNull(childProvider.BaseModelProvider);
}

private class ModelProviderWithExternalBase : ModelProvider
{
private readonly CSharpType _externalBase;

public ModelProviderWithExternalBase(InputModelType inputModel, CSharpType externalBase)
: base(inputModel)
{
_externalBase = externalBase;
}

protected override CSharpType? BuildBaseType() => _externalBase;
}

[Test]
public void BuildModelAsStruct()
{
Expand Down
Loading