From b6e0523ccedf5ff1e4f5041465777133eaf10aa6 Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Fri, 8 May 2026 15:41:05 +0800 Subject: [PATCH 1/4] Fix ModelProvider base ctor virtually dispatching BuildBaseType via AddTypeToKeep ModelProvider.ctor called CodeModelGenerator.AddTypeToKeep(this), which in turn evaluated this.Type -> this.BaseType -> virtual BuildBaseType(). When a derived ModelProvider's BuildBaseType reads any field assigned in the derived constructor body, that field is still null (base ctor runs before derived field assignment), causing a NullReferenceException deep in framework code. This is the classic CA2214 anti-pattern and is especially harmful for an explicit extension point. Two complementary fixes: 1. Move the AddTypeToKeep(this) registration out of ModelProvider..ctor and into TypeFactory.CreateModel, mirroring the existing EnumProvider / TypeFactory.CreateEnum lifecycle. Registration now happens after the provider is fully constructed and after PreVisitModel runs. 2. Make AddTypeToKeep(TypeProvider) defer FQN resolution by storing the TypeProvider reference and resolving .Type.FullyQualifiedName lazily when AdditionalRootTypes / NonRootTypes are queried (the keep set is only consumed by the post-processor, well after construction). This hardens the API against any future ctor-time caller. Adds a regression test that constructs a derived ModelProvider whose BuildBaseType reads a derived field, and verifies AddTypeToKeep(TypeProvider) preserves the FQN once the keep set is materialized. Resolves #10626 --- .../src/CodeModelGenerator.cs | 56 +++++++++++++++++-- .../src/Providers/ModelProvider.cs | 5 -- .../src/TypeFactory.cs | 5 ++ .../ModelProviders/ModelProviderTests.cs | 47 ++++++++++++++++ 4 files changed, 103 insertions(+), 10 deletions(-) diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/CodeModelGenerator.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/CodeModelGenerator.cs index df1ac4a27de..362d09807d5 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/CodeModelGenerator.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/CodeModelGenerator.cs @@ -160,9 +160,38 @@ public virtual void AddSharedSourceDirectory(string sharedSourceDirectory) _sharedSourceDirectories.Add(sharedSourceDirectory); } - internal HashSet AdditionalRootTypes { get; } = []; + private readonly HashSet _additionalRootTypeNames = []; + private readonly HashSet _nonRootTypeNames = []; + private readonly HashSet _additionalRootTypeProviders = []; + private readonly HashSet _nonRootTypeProviders = []; - internal HashSet NonRootTypes { get; } = []; + /// + /// The set of fully qualified type names to keep as roots. Resolved lazily so that + /// entries added via + /// are not forced to materialize their at registration time + /// (which would dispatch virtual Build* methods on partially constructed providers). + /// + internal HashSet AdditionalRootTypes => MaterializeKeepSet(_additionalRootTypeNames, _additionalRootTypeProviders); + + /// + /// The set of fully qualified type names to keep as non-roots. Resolved lazily; see + /// for rationale. + /// + internal HashSet NonRootTypes => MaterializeKeepSet(_nonRootTypeNames, _nonRootTypeProviders); + + private static HashSet MaterializeKeepSet(HashSet names, HashSet providers) + { + if (providers.Count == 0) + { + return names; + } + var result = new HashSet(names); + foreach (var provider in providers) + { + result.Add(provider.Type.FullyQualifiedName); + } + return result; + } /// /// Adds a type to the list of types to keep. @@ -174,21 +203,38 @@ public void AddTypeToKeep(string typeName, bool isRoot = true) { if (isRoot) { - AdditionalRootTypes.Add(typeName); + _additionalRootTypeNames.Add(typeName); } else { - NonRootTypes.Add(typeName); + _nonRootTypeNames.Add(typeName); } } /// /// Adds a type to the list of types to keep. /// + /// + /// The provider's fully qualified name is resolved lazily, when the keep list is consumed during + /// post-processing. This makes it safe to call this method from a + /// constructor (including base constructors that run before the derived constructor body), since + /// it does not force evaluation of — which would dispatch virtual + /// Build* methods on a not-yet-fully-constructed instance. + /// /// The type provider representing the type. /// Whether to treat the type as a root type. Any dependencies of root types will /// not have their accessibility changed regardless of the 'unreferenced-types-handling' value. - public void AddTypeToKeep(TypeProvider type, bool isRoot = true) => AddTypeToKeep(type.Type.FullyQualifiedName, isRoot); + public void AddTypeToKeep(TypeProvider type, bool isRoot = true) + { + if (isRoot) + { + _additionalRootTypeProviders.Add(type); + } + else + { + _nonRootTypeProviders.Add(type); + } + } /// /// Writes additional output files (e.g. configuration schemas) after the main code generation is complete. diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs index e916fb35c29..f09fd030a9f 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs @@ -77,11 +77,6 @@ public ModelProvider(InputModelType inputModel) : base(inputModel) { DiscriminatorValueExpression = EnsureDiscriminatorValueExpression(); } - - if (_inputModel.Access == "public") - { - CodeModelGenerator.Instance.AddTypeToKeep(this); - } } public bool IsUnknownDiscriminatorModel => _inputModel.IsUnknownDiscriminatorModel; diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/TypeFactory.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/TypeFactory.cs index c5f46a399d5..2d8317cb4bc 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/TypeFactory.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/TypeFactory.cs @@ -191,6 +191,11 @@ protected internal TypeFactory() if (modelProvider != null) { + if (model.Access == "public") + { + CodeModelGenerator.Instance.AddTypeToKeep(modelProvider); + } + CSharpTypeMap[modelProvider.Type] = modelProvider; TypeProvidersByName[modelProvider.Type.Name] = modelProvider; } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs index 39f189fb0af..71177a0db3b 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs @@ -1480,6 +1480,53 @@ public void InternalModelsAreNotIncludedInAdditionalRootTypes() Assert.IsFalse(rootTypes.Contains("Sample.Models.MockInputModel")); } + // Regression test for two complementary fixes: + // + // 1. ModelProvider no longer registers itself with AddTypeToKeep from its constructor; + // registration is performed by TypeFactory.CreateModel after construction completes. + // This mirrors the EnumProvider lifecycle and prevents a virtual call chain + // (AddTypeToKeep -> TypeProvider.Type -> BaseType -> virtual BuildBaseType()) from + // being dispatched on a partially-constructed derived ModelProvider whose override + // reads derived-class fields that are still uninitialized. + // + // 2. AddTypeToKeep(TypeProvider) defers FQN resolution until the keep set is consumed, + // so even ctor-time callers cannot force premature TypeProvider.Type evaluation. + [Test] + public void DerivedModelProviderConstructionDoesNotForceTypeEvaluation() + { + var inputModel = InputFactory.Model("MockInputModel", access: "public"); + MockHelpers.LoadMockGenerator(inputModelTypes: [inputModel]); + + // (1) Constructing a derived ModelProvider whose BuildBaseType reads a derived field + // must not throw. + DerivedModelProviderReadingOwnField? provider = null; + Assert.DoesNotThrow(() => provider = new DerivedModelProviderReadingOwnField(inputModel)); + + // (2) AddTypeToKeep(TypeProvider) must not throw and the provider's FQN must appear + // once the keep set is materialized. + Assert.DoesNotThrow(() => CodeModelGenerator.Instance.AddTypeToKeep(provider!)); + var rootTypes = CodeModelGenerator.Instance.AdditionalRootTypes; + Assert.IsTrue(rootTypes.Contains(provider!.Type.FullyQualifiedName)); + } + + private sealed class DerivedModelProviderReadingOwnField : ModelProvider + { + private readonly InputModelType _derivedInputModel; + + public DerivedModelProviderReadingOwnField(InputModelType inputModel) : base(inputModel) + { + _derivedInputModel = inputModel; + } + + protected override CSharpType? BuildBaseType() + { + // Reading a derived-class field that base(...) cannot have populated yet. + // If the framework forces Type evaluation during base ctor, this NREs. + _ = _derivedInputModel.DiscriminatorValue; + return base.BuildBaseType(); + } + } + [TestCase(true, true, InputModelTypeUsage.Output, true, false)] [TestCase(true, false, InputModelTypeUsage.Output, true, false)] [TestCase(false, true, InputModelTypeUsage.Output, true, false)] From db9acbd7b6ed59b42241ba9ec50a4eddc3799a6c Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Fri, 8 May 2026 17:01:41 +0800 Subject: [PATCH 2/4] Defer DiscriminatorValueExpression to fix second virtual-call-in-ctor site ModelProvider..ctor was eagerly computing DiscriminatorValueExpression when the input model had a non-null BaseModel. EnsureDiscriminatorValueExpression() reads BaseModelProvider, which calls BuildBaseModelProvider -> get_BaseType -> virtual BuildBaseType() onto a partially-constructed derived class - the same anti-pattern as the AddTypeToKeep(this) site, just a different call chain. Surfaced while validating the Cdn provisioning migration after the keep-set fix landed: regen still failed with an NRE in ProvisioningResourceProvider.BuildBaseType reading derived state. Fix: convert DiscriminatorValueExpression from eager-init property to a Lazy that materializes on first read. No external callers write to it (verified across typespec + azure-sdk-for-net repos). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/Providers/ModelProvider.cs | 11 +++--- .../ModelProviders/ModelProviderTests.cs | 35 +++++++++++++++++++ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs index f09fd030a9f..732d9c875cc 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs @@ -72,17 +72,16 @@ public ModelProvider(InputModelType inputModel) : base(inputModel) _inputModel = inputModel; _isMultiLevelDiscriminator = ComputeIsMultiLevelDiscriminator(); _useObjectAdditionalProperties = new Lazy(ShouldUseObjectAdditionalProperties); - - if (_inputModel.BaseModel is not null) - { - DiscriminatorValueExpression = EnsureDiscriminatorValueExpression(); - } + _discriminatorValueExpression = new Lazy(() => + _inputModel.BaseModel is not null ? EnsureDiscriminatorValueExpression() : null); } public bool IsUnknownDiscriminatorModel => _inputModel.IsUnknownDiscriminatorModel; public string? DiscriminatorValue => _inputModel.DiscriminatorValue; - public ValueExpression? DiscriminatorValueExpression { get; init; } + + private readonly Lazy _discriminatorValueExpression; + public ValueExpression? DiscriminatorValueExpression => _discriminatorValueExpression.Value; private IReadOnlyList? _derivedModels; public IReadOnlyList DerivedModels => _derivedModels ??= BuildDerivedModels(); diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs index 71177a0db3b..b00b2c5ddc7 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs @@ -1527,6 +1527,41 @@ public DerivedModelProviderReadingOwnField(InputModelType inputModel) : base(inp } } + // Regression for the second virtual-call-in-ctor offender: ModelProvider..ctor used to + // eagerly compute DiscriminatorValueExpression, which read BaseModelProvider and thus + // virtually dispatched BuildBaseType()/BuildBaseModel() onto a partially-constructed + // derived class. Surfaced while validating the Cdn provisioning migration (the keep-set + // fix alone was not sufficient when the model has a base + discriminator value). + [Test] + public void DerivedModelProviderConstructionDoesNotForceDiscriminatorEvaluation() + { + var discriminatorEnum = InputFactory.StringEnum("kindEnum", [("One", "one"), ("Two", "two")]); + var baseInputModel = InputFactory.Model( + "BaseModel", + properties: + [ + InputFactory.Property("kind", discriminatorEnum, isRequired: false, isDiscriminator: true), + ]); + var derivedInputModel = InputFactory.Model( + "DerivedModel", + baseModel: baseInputModel, + discriminatedKind: "one", + properties: + [ + InputFactory.Property("kind", InputFactory.EnumMember.String("One", "one", discriminatorEnum), isRequired: true, isDiscriminator: true), + ]); + MockHelpers.LoadMockGenerator(inputModelTypes: [baseInputModel, derivedInputModel]); + + // Constructing a derived ModelProvider whose BuildBaseType reads a derived field + // must not throw, even when the input model has a base + discriminator value. + DerivedModelProviderReadingOwnField? provider = null; + Assert.DoesNotThrow(() => provider = new DerivedModelProviderReadingOwnField(derivedInputModel)); + + // The discriminator expression must still be available once consumed lazily + // (callers under emission/serialization rely on it). + Assert.DoesNotThrow(() => { _ = provider!.DiscriminatorValueExpression; }); + } + [TestCase(true, true, InputModelTypeUsage.Output, true, false)] [TestCase(true, false, InputModelTypeUsage.Output, true, false)] [TestCase(false, true, InputModelTypeUsage.Output, true, false)] From 2c83713104abb13f95e4a7da5892b8ca042f56cc Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Fri, 8 May 2026 17:18:16 +0800 Subject: [PATCH 3/4] Simplify DiscriminatorValueExpression deferral to ??= pattern Match the style already used for other deferred fields in ModelProvider (e.g. DerivedModels). Behavior is identical - the property still computes on first read instead of in the ctor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/Providers/ModelProvider.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs index 732d9c875cc..01275b3c0a9 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs @@ -72,16 +72,17 @@ public ModelProvider(InputModelType inputModel) : base(inputModel) _inputModel = inputModel; _isMultiLevelDiscriminator = ComputeIsMultiLevelDiscriminator(); _useObjectAdditionalProperties = new Lazy(ShouldUseObjectAdditionalProperties); - _discriminatorValueExpression = new Lazy(() => - _inputModel.BaseModel is not null ? EnsureDiscriminatorValueExpression() : null); } public bool IsUnknownDiscriminatorModel => _inputModel.IsUnknownDiscriminatorModel; public string? DiscriminatorValue => _inputModel.DiscriminatorValue; - private readonly Lazy _discriminatorValueExpression; - public ValueExpression? DiscriminatorValueExpression => _discriminatorValueExpression.Value; + private ValueExpression? _discriminatorValueExpression; + public ValueExpression? DiscriminatorValueExpression => + _inputModel.BaseModel is not null + ? _discriminatorValueExpression ??= EnsureDiscriminatorValueExpression() + : null; private IReadOnlyList? _derivedModels; public IReadOnlyList DerivedModels => _derivedModels ??= BuildDerivedModels(); From a17062e28aa083bfe183c2413dd598452f8ccc24 Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Mon, 11 May 2026 10:45:01 +0800 Subject: [PATCH 4/4] fix(http-client-csharp): cache materialized keep sets Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/CodeModelGenerator.cs | 45 ++++++++++++------- .../ModelProviders/ModelProviderTests.cs | 18 ++++++++ 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/CodeModelGenerator.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/CodeModelGenerator.cs index 362d09807d5..c7a7574552b 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/CodeModelGenerator.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/CodeModelGenerator.cs @@ -160,10 +160,13 @@ public virtual void AddSharedSourceDirectory(string sharedSourceDirectory) _sharedSourceDirectories.Add(sharedSourceDirectory); } - private readonly HashSet _additionalRootTypeNames = []; - private readonly HashSet _nonRootTypeNames = []; - private readonly HashSet _additionalRootTypeProviders = []; - private readonly HashSet _nonRootTypeProviders = []; + private record KeptTypesInfo(HashSet TypeNames, HashSet TypeProviders); + + private readonly KeptTypesInfo _additionalRootTypeInfo = new([], []); + private readonly KeptTypesInfo _nonRootTypeInfo = new([], []); + + private HashSet? _additionalRootTypes; + private HashSet? _nonRootTypes; /// /// The set of fully qualified type names to keep as roots. Resolved lazily so that @@ -171,22 +174,22 @@ public virtual void AddSharedSourceDirectory(string sharedSourceDirectory) /// are not forced to materialize their at registration time /// (which would dispatch virtual Build* methods on partially constructed providers). /// - internal HashSet AdditionalRootTypes => MaterializeKeepSet(_additionalRootTypeNames, _additionalRootTypeProviders); + internal HashSet AdditionalRootTypes => _additionalRootTypes ??= MaterializeKeepSet(_additionalRootTypeInfo); /// /// The set of fully qualified type names to keep as non-roots. Resolved lazily; see /// for rationale. /// - internal HashSet NonRootTypes => MaterializeKeepSet(_nonRootTypeNames, _nonRootTypeProviders); + internal HashSet NonRootTypes => _nonRootTypes ??= MaterializeKeepSet(_nonRootTypeInfo); - private static HashSet MaterializeKeepSet(HashSet names, HashSet providers) + private static HashSet MaterializeKeepSet(KeptTypesInfo info) { - if (providers.Count == 0) + if (info.TypeProviders.Count == 0) { - return names; + return info.TypeNames; } - var result = new HashSet(names); - foreach (var provider in providers) + var result = new HashSet(info.TypeNames); + foreach (var provider in info.TypeProviders) { result.Add(provider.Type.FullyQualifiedName); } @@ -203,11 +206,17 @@ public void AddTypeToKeep(string typeName, bool isRoot = true) { if (isRoot) { - _additionalRootTypeNames.Add(typeName); + if (_additionalRootTypeInfo.TypeNames.Add(typeName)) + { + _additionalRootTypes = null; + } } else { - _nonRootTypeNames.Add(typeName); + if (_nonRootTypeInfo.TypeNames.Add(typeName)) + { + _nonRootTypes = null; + } } } @@ -228,11 +237,17 @@ public void AddTypeToKeep(TypeProvider type, bool isRoot = true) { if (isRoot) { - _additionalRootTypeProviders.Add(type); + if (_additionalRootTypeInfo.TypeProviders.Add(type)) + { + _additionalRootTypes = null; + } } else { - _nonRootTypeProviders.Add(type); + if (_nonRootTypeInfo.TypeProviders.Add(type)) + { + _nonRootTypes = null; + } } } diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs index b00b2c5ddc7..68f586b8547 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs @@ -1480,6 +1480,24 @@ public void InternalModelsAreNotIncludedInAdditionalRootTypes() Assert.IsFalse(rootTypes.Contains("Sample.Models.MockInputModel")); } + [Test] + public void KeepSetsReflectTypeProvidersAddedAfterFirstAccess() + { + var inputModel = InputFactory.Model("MockInputModel", access: "public"); + MockHelpers.LoadMockGenerator(inputModelTypes: [inputModel]); + var provider = new DerivedModelProviderReadingOwnField(inputModel); + + _ = CodeModelGenerator.Instance.AdditionalRootTypes; + _ = CodeModelGenerator.Instance.NonRootTypes; + + CodeModelGenerator.Instance.AddTypeToKeep(provider); + CodeModelGenerator.Instance.AddTypeToKeep(provider, isRoot: false); + + var fullyQualifiedName = provider.Type.FullyQualifiedName; + Assert.IsTrue(CodeModelGenerator.Instance.AdditionalRootTypes.Contains(fullyQualifiedName)); + Assert.IsTrue(CodeModelGenerator.Instance.NonRootTypes.Contains(fullyQualifiedName)); + } + // Regression test for two complementary fixes: // // 1. ModelProvider no longer registers itself with AddTypeToKeep from its constructor;