From 6cbf077bd3d170d81ebd95e5631d2a0652fe0306 Mon Sep 17 00:00:00 2001 From: jolov Date: Thu, 20 Aug 2026 14:49:28 -0700 Subject: [PATCH 1/3] Reset the cached full constructor when a type's identity changes `ModelProvider.BuildConstructors` returns the cached `FullConstructor` instance as part of the constructor list. `TypeProvider.Reset` clears `_constructors` and `_fullConstructor` together, but `ResetMembersBasedOnIdentityChange` (reached from `Update(name:)` and `Update(@namespace:)`) cleared only `_constructors`. That left the stale `FullConstructor` instance in the rebuilt constructor list. Generators that post-process the constructors returned from `BuildConstructors` then re-applied their mutations to the same instance. `ScmModelProvider.BuildConstructors` does exactly this for dynamic models: it appends `_patch = patch;` and `SetPropagators(...)` to the body and prepends an SCME0001 suppression, so a rebuild emitted those statements and the surrounding `#pragma` directives twice. Add a `ResetCachedConstructors` hook on `TypeProvider`, called wherever the constructor list is invalidated on an identity change, and override it in `ModelProvider` to clear `_fullConstructor`. This makes the identity-change path consistent with `Reset`. Regenerating every test project produces no output changes, so this only affects the duplicated-member case. Fixes the generated-code duplication reported in https://github.com/Azure/azure-sdk-for-net/issues/61851. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: aa3d7684-f4ce-4618-94eb-63c1254173ca --- .../src/Providers/ModelProvider.cs | 9 +++ .../src/Providers/TypeProvider.cs | 18 +++++ .../ModelProviders/ModelProviderTests.cs | 70 +++++++++++++++++++ 3 files changed, 97 insertions(+) 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 9e726fb171f..707a53893a6 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 @@ -192,6 +192,15 @@ public override void Reset() _isMultiLevelDiscriminator = null; } + /// + protected override void ResetCachedConstructors() + { + base.ResetCachedConstructors(); + // BuildConstructors returns the cached FullConstructor instance, so it has to be invalidated + // together with the constructor list. + _fullConstructor = null; + } + protected FieldProvider? RawDataField { get diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs index 4687727365e..3ac11bd4335 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs @@ -831,11 +831,29 @@ private void ResetMembersBasedOnIdentityChange(string? name = null, string? @nam _declarationModifiers = null; // constructors might change based on declaration modifier changes _constructors = null; + // constructor instances cached separately by derived types are part of the constructor + // list, so they must be invalidated alongside it + ResetCachedConstructors(); // serialization providers need to reflect the new type name/namespace _serializationProviders = null; Type.Update(name: name, @namespace: @namespace); } + /// + /// Clears any individual constructor instances that this type provider caches outside of + /// . + /// + /// + /// may return cached constructor instances, and callers are free to + /// mutate the constructors they receive. If is invalidated without also + /// invalidating those caches, a subsequent rebuild reuses the same instances and re-applies any + /// mutation, producing duplicated members. already clears both; this hook keeps + /// the identity-change path consistent with it. + /// + protected virtual void ResetCachedConstructors() + { + } + public IReadOnlyList EnumValues => _enumValues ??= BuildEnumValues(); protected virtual IReadOnlyList BuildEnumValues() => throw new InvalidOperationException("Not an EnumProvider type"); 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 66ce8b8954e..81ac253033e 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 @@ -2357,6 +2357,76 @@ public void TestUpdate_ResetsSerializationProviders() Assert.AreEqual(1, newerSerializationProviders.Count); } + // BuildConstructors returns the cached FullConstructor instance, so invalidating the constructor + // list on an identity change without invalidating FullConstructor left the stale instance in the + // rebuilt list. Derived generators mutate the constructors they get back from BuildConstructors, + // and reusing the same instance caused those mutations to be applied more than once. + [Test] + public void TestUpdate_ResetsFullConstructor() + { + MockHelpers.LoadMockGenerator(); + var inputModel = InputFactory.Model("TestModel", properties: [InputFactory.Property("prop1", InputPrimitiveType.String)]); + var modelProvider = new ModelProvider(inputModel); + + var fullConstructor = modelProvider.FullConstructor; + Assert.IsTrue(modelProvider.Constructors.Contains(fullConstructor)); + + // Change name + modelProvider.Update(name: "NewName"); + var newFullConstructor = modelProvider.FullConstructor; + Assert.AreNotSame(fullConstructor, newFullConstructor); + // The rebuilt constructor list must contain the rebuilt full constructor, not the stale one + Assert.IsTrue(modelProvider.Constructors.Contains(newFullConstructor)); + Assert.IsFalse(modelProvider.Constructors.Contains(fullConstructor)); + + // Change namespace + modelProvider.Update(@namespace: "NewNamespace"); + var newerFullConstructor = modelProvider.FullConstructor; + Assert.AreNotSame(newFullConstructor, newerFullConstructor); + Assert.IsTrue(modelProvider.Constructors.Contains(newerFullConstructor)); + } + + // Regression coverage for the duplication that the stale FullConstructor caused: a generator that + // adds a suppression to the full constructor every time it builds the constructor list must not see + // its additions accumulate when an identity change triggers a rebuild. + [Test] + public void TestUpdate_DoesNotReapplyConstructorMutationsAfterIdentityChange() + { + MockHelpers.LoadMockGenerator(); + var inputModel = InputFactory.Model("TestModel", properties: [InputFactory.Property("prop1", InputPrimitiveType.String)]); + var modelProvider = new MutatingModelProvider(inputModel); + + _ = modelProvider.Constructors; + Assert.AreEqual(1, modelProvider.FullConstructor.Suppressions.Count); + + modelProvider.Update(name: "NewName"); + _ = modelProvider.Constructors; + + Assert.AreEqual(1, modelProvider.FullConstructor.Suppressions.Count); + } + + // Mimics how ScmModelProvider post-processes the constructors returned from the base implementation. + private class MutatingModelProvider : ModelProvider + { + public MutatingModelProvider(InputModelType inputModel) : base(inputModel) + { + } + + protected internal override ConstructorProvider[] BuildConstructors() + { + var constructors = base.BuildConstructors(); + foreach (var constructor in constructors) + { + if (ReferenceEquals(constructor, FullConstructor)) + { + var suppression = new SuppressionStatement(null, Generator.Snippets.Snippet.Literal("TEST0001"), "Test suppression."); + constructor.Update(suppressions: [suppression, .. constructor.Suppressions]); + } + } + return constructors; + } + } + private class TestModelProvider : ModelProvider { private readonly string? _name; From 95780a3c42099a80185c1cfad37210239f8b2e39 Mon Sep 17 00:00:00 2001 From: x Date: Thu, 20 Aug 2026 15:45:24 -0700 Subject: [PATCH 2/3] Make ResetCachedConstructors private protected Keeps the fix out of the public extensibility surface: only ModelProvider, in the same assembly, needs to override it. Matches the existing private protected virtual ShouldUseFullConstructorInDerivedTypes on ModelProvider. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: aa3d7684-f4ce-4618-94eb-63c1254173ca --- .../src/Providers/ModelProvider.cs | 2 +- .../src/Providers/TypeProvider.cs | 9 +++++---- 2 files changed, 6 insertions(+), 5 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 707a53893a6..ca130faee7e 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 @@ -193,7 +193,7 @@ public override void Reset() } /// - protected override void ResetCachedConstructors() + private protected override void ResetCachedConstructors() { base.ResetCachedConstructors(); // BuildConstructors returns the cached FullConstructor instance, so it has to be invalidated diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs index 3ac11bd4335..3b48d8c1d0d 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs @@ -831,8 +831,8 @@ private void ResetMembersBasedOnIdentityChange(string? name = null, string? @nam _declarationModifiers = null; // constructors might change based on declaration modifier changes _constructors = null; - // constructor instances cached separately by derived types are part of the constructor - // list, so they must be invalidated alongside it + // constructor instances cached separately are part of the constructor list, so they must + // be invalidated alongside it ResetCachedConstructors(); // serialization providers need to reflect the new type name/namespace _serializationProviders = null; @@ -848,9 +848,10 @@ private void ResetMembersBasedOnIdentityChange(string? name = null, string? @nam /// mutate the constructors they receive. If is invalidated without also /// invalidating those caches, a subsequent rebuild reuses the same instances and re-applies any /// mutation, producing duplicated members. already clears both; this hook keeps - /// the identity-change path consistent with it. + /// the identity-change path consistent with it without resetting the members that an identity + /// change does not affect. /// - protected virtual void ResetCachedConstructors() + private protected virtual void ResetCachedConstructors() { } From ef7cdf180c9ed20a6c323a37a1898eebf29fa48f Mon Sep 17 00:00:00 2001 From: jolov Date: Thu, 20 Aug 2026 16:57:46 -0700 Subject: [PATCH 3/3] Reset the cached full constructor when a type's identity changes `ModelProvider.BuildConstructors` returns the cached `FullConstructor` instance as part of the constructor list, so `Constructors` contains `_fullConstructor`. `TypeProvider.Reset` clears both, but the identity-change path (`Update(name:)` / `Update(@namespace:)`) cleared only `_constructors`, leaving the stale instance to be picked back up on the next rebuild. Derived providers may mutate the constructors they build. `ScmModelProvider` appends JSON patch statements and prepends an SCME0001 suppression inside `BuildConstructors`, so re-running it against the same instance applies those mutations a second time, emitting duplicated statements and duplicated `#pragma` directives in generated code. Make `FullConstructor` track the type identity it was built for and rebuild when that identity no longer matches, so the cache invalidates itself regardless of which path changes the name or namespace. Fixes duplicated constructor members reported downstream in Azure/azure-sdk-for-net#61851. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: aa3d7684-f4ce-4618-94eb-63c1254173ca --- .../src/Providers/ModelProvider.cs | 35 +++++++++++++------ .../src/Providers/TypeProvider.cs | 19 ---------- .../ModelProviders/ModelProviderTests.cs | 3 +- 3 files changed, 27 insertions(+), 30 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 ca130faee7e..6396b784520 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 @@ -67,6 +67,7 @@ protected override FormattableString BuildDescription() private List? _additionalPropertyProperties; private ModelProvider? _baseModelProvider; private ConstructorProvider? _fullConstructor; + private (string Name, string Namespace)? _fullConstructorIdentity; internal PropertyProvider? DiscriminatorProperty { get; private set; } private readonly bool _isDiscriminatedBaseType; @@ -189,18 +190,10 @@ public override void Reset() _additionalPropertyFields = null; _additionalPropertyProperties = null; _fullConstructor = null; + _fullConstructorIdentity = null; _isMultiLevelDiscriminator = null; } - /// - private protected override void ResetCachedConstructors() - { - base.ResetCachedConstructors(); - // BuildConstructors returns the cached FullConstructor instance, so it has to be invalidated - // together with the constructor list. - _fullConstructor = null; - } - protected FieldProvider? RawDataField { get @@ -239,7 +232,29 @@ protected FieldProvider? RawDataField protected internal bool SupportsBinaryDataAdditionalProperties => AdditionalPropertyProperties.Any(p => p.Type.ElementType.Equals(_additionalPropsUnknownType) || (p.Type.ElementType.IsFrameworkType && p.Type.ElementType.FrameworkType == typeof(object))); - public ConstructorProvider FullConstructor => _fullConstructor ??= BuildFullConstructor(); + /// + /// The constructor that takes every serializable property. + /// + /// + /// This instance is also returned as part of , and callers are free to + /// mutate the constructors they receive. An identity change invalidates the constructor list, so the cached + /// instance is rebuilt alongside it; otherwise a rebuild would reuse the same instance and re-apply any + /// mutation, producing duplicated members. + /// + public ConstructorProvider FullConstructor + { + get + { + var identity = (Type.Name, Type.Namespace); + if (_fullConstructor is null || _fullConstructorIdentity != identity) + { + _fullConstructor = BuildFullConstructor(); + _fullConstructorIdentity = identity; + } + + return _fullConstructor; + } + } protected override string BuildNamespace() => string.IsNullOrEmpty(_inputModel.Namespace) ? // TODO remove null check once https://github.com/Azure/typespec-azure/issues/2209 is fixed. diff --git a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs index 3b48d8c1d0d..4687727365e 100644 --- a/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs +++ b/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs @@ -831,30 +831,11 @@ private void ResetMembersBasedOnIdentityChange(string? name = null, string? @nam _declarationModifiers = null; // constructors might change based on declaration modifier changes _constructors = null; - // constructor instances cached separately are part of the constructor list, so they must - // be invalidated alongside it - ResetCachedConstructors(); // serialization providers need to reflect the new type name/namespace _serializationProviders = null; Type.Update(name: name, @namespace: @namespace); } - /// - /// Clears any individual constructor instances that this type provider caches outside of - /// . - /// - /// - /// may return cached constructor instances, and callers are free to - /// mutate the constructors they receive. If is invalidated without also - /// invalidating those caches, a subsequent rebuild reuses the same instances and re-applies any - /// mutation, producing duplicated members. already clears both; this hook keeps - /// the identity-change path consistent with it without resetting the members that an identity - /// change does not affect. - /// - private protected virtual void ResetCachedConstructors() - { - } - public IReadOnlyList EnumValues => _enumValues ??= BuildEnumValues(); protected virtual IReadOnlyList BuildEnumValues() => throw new InvalidOperationException("Not an EnumProvider type"); 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 81ac253033e..4933eddc500 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 @@ -11,6 +11,7 @@ using Microsoft.TypeSpec.Generator.Input; using Microsoft.TypeSpec.Generator.Primitives; using Microsoft.TypeSpec.Generator.Providers; +using Microsoft.TypeSpec.Generator.Snippets; using Microsoft.TypeSpec.Generator.Statements; using Microsoft.TypeSpec.Generator.Tests.Common; using Microsoft.TypeSpec.Generator.Utilities; @@ -2419,7 +2420,7 @@ protected internal override ConstructorProvider[] BuildConstructors() { if (ReferenceEquals(constructor, FullConstructor)) { - var suppression = new SuppressionStatement(null, Generator.Snippets.Snippet.Literal("TEST0001"), "Test suppression."); + var suppression = new SuppressionStatement(null, Snippet.Literal("TEST0001"), "Test suppression."); constructor.Update(suppressions: [suppression, .. constructor.Suppressions]); } }