microsoft / microsoft/typespec

[http-client-csharp] AddTypeToKeep stores stale FQN after visitor namespace changes, causing public models to be internalized

Open
#10,272 0 comments 1 reaction 2 assignees Claimed by @haiyuazhang View on GitHub
bug emitter:client:csharp
Dominant language
Java
Stars
5.9k
Forks
394
Avg merge
1d 23h
Merged PRs (30d)
104

Description

## Bug Report

### Description

`AddTypeToKeep(TypeProvider)` eagerly resolves `type.Type.FullyQualifiedName` to a string at call time. When a visitor (e.g. `NamespaceVisitor`) later changes the type's namespace, the stored FQN becomes stale. `PostProcessor.ShouldKeepType()` then fails to match the type, causing it to be internalized even though `@@access(public)` was set.

This was discovered during the **ProviderHub** management SDK migration, where models like `ManifestLevelPropertyBag` and `ProviderFeaturesRule` were silently internalized despite explicit `@@access(Access.public)` decorators.

### Root Cause

In `CodeModelGenerator.cs`:
```csharp
public void AddTypeToKeep(TypeProvider type, bool isRoot = true)
=> AddTypeToKeep(type.Type.FullyQualifiedName, isRoot);
```

This is called from `ModelProvider` constructor (for `Access == "public"`) and `TypeFactory` (for public enums) — both run **before** visitors execute.

The pipeline order in `CSharpGen.ExecuteAsync()`:
1. Build TypeProviders (constructors run → `AddTypeToKeep` stores pre-visitor FQN)
2. Run visitors (`NamespaceVisitor` changes namespace → FQN changes)
3. Write generated files
4. `PostProcessAsync()` → `PostProcessor` receives stale FQNs in `typesToKeep`

In `PostProcessor.ShouldKeepType()`, both checks fail:
- Simple name `"MyModel"` vs typesToKeep containing `"OldNamespace.MyModel"` → no match
- Roslyn FQN `"NewNamespace.Models.MyModel"` vs `"OldNamespace.MyModel"` → no match

### Impact

Models with `@@access(public)` get silently internalized when `model-namespace` is true (the mgmt default). This affects management SDK migrations where models like `ManifestLevelPropertyBag` (ProviderHub service) become internal despite explicit public access decorators.

### Reproduction

```csharp
[Test]
public void PublicModelTypeToKeepUpdatesAfterNamespaceChange()
{
var inputModel = InputFactory.Model(
"MockInputModel",
access: "public");

MockHelpers.LoadMockGenerator(
inputModelTypes: [inputModel]);

var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders
.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider;
Assert.IsNotNull(modelProvider);

var originalFqn = modelProvider!.Type.FullyQualifiedName;
Assert.AreEqual("Sample.Models.MockInputModel", originalFqn);

// Simulate a visitor changing the namespace (e.g., NamespaceVisitor appending ".Models")
modelProvider.Update(@namespace: "NewNamespace.Models");
var newFqn = modelProvider.Type.FullyQualifiedName;
Assert.AreEqual("NewNamespace.Models.MockInputModel", newFqn);

// After namespace change, the resolved root types should contain the updated FQN
// so that PostProcessor.ShouldKeepType can match it against the generated code.
var rootTypes = CodeModelGenerator.Instance.AdditionalRootTypes;
Assert.IsTrue(rootTypes.Contains(newFqn),
$"AdditionalRootTypes should contain the post-visitor FQN '{newFqn}' "
+ $"but only contains: [{string.Join(", ", rootTypes)}]");
}
```

### Proposed Fix

Store `TypeProvider` references instead of eagerly resolving FQNs. Resolve lazily when `AdditionalRootTypes`/`NonRootTypes` are accessed (after visitors have run).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.