Reset the cached full constructor when a type's identity changes

[JoshLove-msft]

Problem

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:

Site	Clears _constructors	Clears _fullConstructor
TypeProvider.Reset()	✅	✅ (via ModelProvider.Reset)
ResetMembersBasedOnIdentityChange	✅	❌

So a rename or namespace change 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 that 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 them twice:

#pragma warning disable SCME0001 // ...
#pragma warning disable SCME0001 // ...
internal ComputeFleetVmProfile(CapacityReservationProfile capacityReservation, in JsonPatch patch)
{
    CapacityReservation = capacityReservation;
    _patch = patch;
    _patch.SetPropagators(PropagateSet, PropagateGet);
    _patch = patch;
    _patch.SetPropagators(PropagateSet, PropagateGet);
}
#pragma warning restore SCME0001 // ...
#pragma warning restore SCME0001 // ...

Fix

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.

Validation

• Regenerated every test project (full Spector matrix + local projects): zero output changes. The fix only suppresses the duplicated members; it does not alter any currently-emitted code.
• Full generator suite green: Microsoft.TypeSpec.Generator.Tests 1916/1916, Microsoft.TypeSpec.Generator.ClientModel.Tests 1575/1575, Microsoft.TypeSpec.Generator.Input.Tests 177/177, TestProjects.Local.Tests 55/55. The 24 TestProjects.Spector.Tests failures reproduce identically on a clean main checkout (they need the Spector mock server) and are unrelated.
• Two new regression tests in ModelProviderTests; both fail without the fix:
  • TestUpdate_ResetsFullConstructor — asserts the invariant directly: after an identity change, Constructors contains the rebuilt FullConstructor and no longer contains the stale one.
  • TestUpdate_DoesNotReapplyConstructorMutationsAfterIdentityChange — reproduces the symptom via a ModelProvider subclass that mimics how ScmModelProvider post-processes the constructor list.

I also audited every site that caches or compares a FullConstructor instance, since replacing it is the main risk of this change:

• ScmModelProvider mutates it only from inside BuildConstructors, so a rebuild re-applies the mutation to a fresh instance — idempotent by construction.
• MrwSerializationTypeDefinition caches it in _serializationConstructor, but _serializationProviders is cleared in the same reset method, so the serialization provider is rebuilt alongside it.
• ModelFactoryProvider reads it live within a single call, and relies on Constructors.Contains(FullConstructor) — an invariant this change restores rather than breaks.

Context

Found while fixing Azure/azure-sdk-for-net#61851, where the duplication showed up in generated Azure management-plane models.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@typespec/http-client-csharp@11737

commit: ef7cdf1

[Copilot]

Pull request overview

Synchronizes cached full constructors with constructor-list invalidation during model identity changes.

Changes:

• Adds a constructor-cache reset hook to TypeProvider.
• Clears ModelProvider’s cached FullConstructor on rename/namespace updates.
• Adds regression tests for stale instances and duplicate mutations.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs	Updated as part of this pull request.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs	Updated as part of this pull request.
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs	Updated as part of this pull request.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

No changes needing a change description found.

[Jorge Rangel (jorgerangel-msft)]

Could we avoid introducing the ResetCachedConstructors() API here? Since ModelProvider owns _fullConstructor, could FullConstructor instead track the effective identity and rebuild itself when that identity changes?

private (string Name, string Namespace)? _fullConstructorIdentity;

public ConstructorProvider FullConstructor
{
    get
    {
        var identity = (Type.Name, Type.Namespace);
        if (_fullConstructor is null || _fullConstructorIdentity != identity)
        {
            _fullConstructor = BuildFullConstructor();
            _fullConstructorIdentity = identity;
        }

        return _fullConstructor;
    }
}

This would keep invalidation local to the cache owner while preserving the selective identity-reset behavior. Will this work?

🤖 Generated by Jorge's Copilot

[JoshLove-msft]

Updated based on review feedback:

No new public surface. ResetCachedConstructors is now private protected virtual, so it is not part of the public extensibility surface — only ModelProvider, in the same assembly, overrides it. This matches the existing private protected virtual ShouldUseFullConstructorInDerivedTypes on ModelProvider.

Why not just call Reset() from the identity-change path? I tried it. Reset() additionally clears _methods, _properties, _fields, _implements, _nestedTypes, _xmlDocs, _relativeFilePath, _rawDataField, _additionalPropertyFields, _additionalPropertyProperties and _isMultiLevelDiscriminator — none of which a rename or namespace change invalidates. That variant fails 15 tests in Microsoft.TypeSpec.Generator.Tests (for example CreateEnum_WithCustomCodeAsExtensible_ReturnsExtensibleEnum, where the reset discards already-applied custom code state), because visitors that have already mutated properties and methods lose those mutations. The targeted hook keeps the identity-change path clearing exactly what an identity change affects.

Full suite is green with the current change: Microsoft.TypeSpec.Generator.Tests 1916/1916, Microsoft.TypeSpec.Generator.ClientModel.Tests 1575/1575, Microsoft.TypeSpec.Generator.Input.Tests 177/177, TestProjects.Local.Tests 55/55, TestProjects.Spector.Tests 0 failed. Regenerating all test projects still produces no output changes.

--generated by Copilot

[Jorge Rangel (jorgerangel-msft)]

Updated based on review feedback:

No new public surface. ResetCachedConstructors is now private protected virtual, so it is not part of the public extensibility surface — only ModelProvider, in the same assembly, overrides it. This matches the existing private protected virtual ShouldUseFullConstructorInDerivedTypes on ModelProvider.

Why not just call Reset() from the identity-change path? I tried it. Reset() additionally clears _methods, _properties, _fields, _implements, _nestedTypes, _xmlDocs, _relativeFilePath, _rawDataField, _additionalPropertyFields, _additionalPropertyProperties and _isMultiLevelDiscriminator — none of which a rename or namespace change invalidates. That variant fails 15 tests in Microsoft.TypeSpec.Generator.Tests (for example CreateEnum_WithCustomCodeAsExtensible_ReturnsExtensibleEnum, where the reset discards already-applied custom code state), because visitors that have already mutated properties and methods lose those mutations. The targeted hook keeps the identity-change path clearing exactly what an identity change affects.

Full suite is green with the current change: Microsoft.TypeSpec.Generator.Tests 1916/1916, Microsoft.TypeSpec.Generator.ClientModel.Tests 1575/1575, Microsoft.TypeSpec.Generator.Input.Tests 177/177, TestProjects.Local.Tests 55/55, TestProjects.Spector.Tests 0 failed. Regenerating all test projects still produces no output changes.

--generated by Copilot

I thought the issue was the an Update call in a downstream emitter was not updating the full ctor. Won't my proposal solve that if either Name or Ns changes ? 🤔

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[JoshLove-msft]

You're right on both counts — I've switched the PR to your approach.

To your follow-up: yes, your proposal solves the reported case. The downstream Update path (ResetMembersBasedOnIdentityChange) calls Type.Update(name:, @namespace:) last, after _constructors = null, so by the time anything reads FullConstructor again the identity comparison already sees the new value and rebuilds. I confirmed this by implementing your version verbatim and running it against the two regression tests in this PR — both pass without the hook.

The one thing I wanted to check before dropping the hook was a decoupling concern. My version invalidated _fullConstructor and _constructors together, atomically. Yours invalidates them independently: _constructors eagerly, _fullConstructor lazily on next read. That opens a theoretical window where Constructors holds the old instance while the getter hands back a new one, which would break Constructors.Contains(FullConstructor) in ModelFactoryProvider.GetBinaryDataParamAndFullCtorForFactoryMethod and silently push it down the "suppressed or customized" branch.

That window turns out not to be reachable:

• BuildConstructors itself reads FullConstructor, so any rebuild of the list picks up the rebuilt instance. The two stay consistent through the normal path.
• The only way to desynchronize them is to change Type's identity without invalidating _constructors — i.e. calling CSharpType.Update directly. That method is internal, its only non-test call site in the generator is TypeProvider.cs:836 (inside ResetMembersBasedOnIdentityChange, which nulls _constructors anyway), and InternalsVisibleTo only covers the test assemblies — Microsoft.TypeSpec.Generator.ClientModel isn't on the list. So a downstream emitter can't reach it.

Given that, your version is strictly better: no new API, the invalidation lives with the cache that owns it rather than making TypeProvider aware that ModelProvider keeps a side cache, and it's self-healing if some future path changes identity without going through ResetMembersBasedOnIdentityChange — mine only fixed the one path.

Changes from your sketch: I also clear _fullConstructorIdentity in Reset() (the null check already covers it, but leaving a stale tuple behind was untidy), and added a remark on FullConstructor explaining why the tracking is there, since the failure mode is not obvious from the code.

Full suite green: 1916 core, 1575 ClientModel, 177 Input, 55 Local, Spector 0 failed. Regenerating all test projects to reconfirm zero output drift.

--generated by Copilot

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 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:253

• Comparing only the resulting name/namespace does not cover every constructor-list invalidation. TypeProvider.ResetMembersBasedOnIdentityChange unconditionally clears _constructors for any Update(name: ...)/Update(@namespace: ...), even when the supplied value is unchanged (or a customization maps it back to the same identity). This getter then reuses the already post-processed _fullConstructor, so a subsequent ScmModelProvider.BuildConstructors can append its suppression and SetPropagators statements again. Invalidate this cache from the same reset path (for example via the virtual hook described in the PR) rather than inferring invalidation only from the tuple.

                if (_fullConstructor is null || _fullConstructorIdentity != identity)
                {
                    _fullConstructor = BuildFullConstructor();
                    _fullConstructorIdentity = identity;
                }

[JoshLove-msft]

Correction to my earlier analysis: the identity-based invalidation handles real name/namespace changes, but it misses same-value identity updates. TypeProvider.Update(name: provider.Name) still clears the Constructors cache; because the identity tuple is unchanged, FullConstructor returns the previously mutated instance and derived BuildConstructors implementations can apply their mutations again.

The management generator reaches this path through visitors that reassert an already-final namespace. I reproduced the duplicate JsonPatch assignments and pragmas against the published 20260821.2 build and opened #11745 to invalidate FullConstructor whenever the constructor list is invalidated. Its full CI is green.

--generated by Copilot