Fix MRW context base model discovery

[live1206]

Description

Fixes ModelReaderWriterContextDefinition so recursive base-model traversal still discovers framework model reader/writer types when an equivalent provider name was already visited from the output library roots.

This prevents context attributes such as ModelReaderWriterBuildable from being dropped for framework MRW types referenced through base provider properties.

Validation

• dotnet test Microsoft.TypeSpec.Generator.ClientModel/test/Microsoft.TypeSpec.Generator.ClientModel.Tests.csproj --filter FullyQualifiedName~ModelReaderWriterContextDefinitionTests.ValidateDuplicateBaseProviderNameStillDiscoversFrameworkTypes --no-restore
• dotnet test Microsoft.TypeSpec.Generator.ClientModel/test/Microsoft.TypeSpec.Generator.ClientModel.Tests.csproj --no-restore
• git diff --check

[pkg-pr-new]

Open in StackBlitz

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

commit: 8372941

[github-actions]

No changes needing a change description found.

[live1206]

No diff in regen: Azure/azure-sdk-for-net#60155

[JoshLove-msft]

Review

Verdict: correct fix for the reported bug, with one recursion-safety concern worth raising.

Why the fix works ✅

visitedTypeProviders is built with a name comparer (s_typeProviderNameComparer). When a derived model''s BaseModelProvider shares a type name with a provider already visited from the output-library roots, the old call to CollectBuildableTypeProvidersRecursive hit visitedTypeProviders.Add(...) == false and returned early — so the base provider''s properties (e.g. the FrameworkModelWithMRW property) were never traversed and the ModelReaderWriterBuildable attribute was dropped. Calling CollectBuildableTypesRecursiveCore directly bypasses the name-based dedup and traverses the base. The new test reproduces this precisely. Build is clean and all ModelReaderWriterContextDefinitionTests pass.

Base models are still never emitted as standalone context entries (buildableProviders is only populated in the root loop), so the comment above the changed line remains accurate. ✅

🟡 Concern: base traversal loses its only cycle guard

The changed line now recurses into the base chain via Core with no visitedTypeProviders add. The old path''s name-based Add check doubled as the cycle-breaker. If a BaseModelProvider chain ever cycles (a real possibility through custom code — note ModelProvider.HasBaseModelProviderCycle exists specifically to guard FullConstructor against exactly this), this will now infinitely recurse → StackOverflowException, where it previously terminated.

Suggestion: add a reference-equality recursion guard for the base traversal, e.g.:

if (provider is ModelProvider modelProvider && modelProvider.BaseModelProvider != null
    && visitedBaseProviders.Add(modelProvider.BaseModelProvider)) // ReferenceEqualityComparer set
{
    CollectBuildableTypesRecursiveCore(modelProvider.BaseModelProvider, ...);
}

This fixes the name-collision bug and preserves protection against cyclic base chains. A reference-equality guard won''t reintroduce the original bug because the two same-named base instances are distinct references.

🟢 Minor (non-blocking)

Shared base subtrees are now re-traversed once per derived model (the base no longer gets memoized in visitedTypeProviders). Output is still correct (buildableTypes/visitedTypes dedup by name), so this is only redundant work — negligible unless hierarchies are very wide/deep. The reference-equality guard above would also eliminate this.

--generated by Copilot

[live1206]

Addressed in 7f24085: added a reference-equality visitedBaseProviders guard for base traversal so same-named distinct providers still get traversed, while actual base-provider cycles stop. Also added ValidateCyclicBaseProviderTraversalStops to cover the cycle case.

Validation:

• targeted duplicate-name + cycle tests passed
• full Microsoft.TypeSpec.Generator.ClientModel.Tests passed (1438 tests)

[JoshLove-msft]

Review — LGTM ✅

I reviewed the fix and verified it locally (built + ran the two new tests, and re-ran them against the pre-fix source).

Correctness

The change replaces base-model recursion with a CollectBuildableTypesRecursiveCore call guarded by a new visitedBaseProviders set using ReferenceEqualityComparer. This fixes two real issues at once:

• Dropped types: name-based dedup (s_typeProviderNameComparer) previously suppressed traversal of a base provider whose type name was already visited from an output-library root, so framework MRW types reachable only through that base provider's properties (e.g. ModelReaderWriterBuildable for FrameworkModelWithMRW) were lost. Reference equality no longer over-collapses distinct same-named base instances.
• Cycle safety: the pre-fix base traversal had no cycle guard — I confirmed ValidateCyclicBaseProviderTraversalStops triggers a StackOverflowException (test run aborted) against the old source, and terminates correctly with the fix.

Verification

• ValidateDuplicateBaseProviderNameStillDiscoversFrameworkTypes and ValidateCyclicBaseProviderTraversalStops both pass with the fix and fail/abort without it — genuine regression guards.
• Confirmed new HashSet<TypeProvider>(ReferenceEqualityComparer.Instance) compiles (net8+).

Minor (non-blocking)

• visitedBaseProviders is created once per CollectBuildableTypes() and reference-keyed, so a base provider instance shared across multiple derived models is traversed only once globally. That's correct since buildable types are accumulated globally, but worth noting it relies on shared base providers being the same instance.

Nice, surgical fix with good coverage.

--generated by Copilot