microsoft / microsoft/typespec
[http-client-csharp] Consolidate InputType graph walkers into a single visitor
- Dominant language
- Java
- Stars
- 5.9k
- Forks
- 394
- Avg merge
- 1d 23h
- Merged PRs (30d)
- 104
Description
### Background
While reviewing #10666 (dynamic loading of `ExternalTypes`), @JoshLove-msft noted that `ExternalTypeReferenceResolver.CollectExternalTypes`/`VisitType` duplicates the recursive type-graph traversal already implemented in `InputModelTypeConverter.MarkModelsAsDynamicRecursive`.
A third (or fourth) walker is likely needed in the future, so we should consolidate to a single canonical visitor.
### Current duplication
Two callers in the repo today:
1. **`InputModelTypeConverter.MarkModelsAsDynamicRecursive`** (`Microsoft.TypeSpec.Generator.Input/src/InputTypes/Serialization/InputModelTypeConverter.cs`)
- Seed: a single `InputModelType` (during deserialization, before the library is fully built)
- Side effect: sets `modelType.IsDynamicModel = true`
- Special rule: walks `BaseModel` only when the base has a `DiscriminatorProperty` / `DiscriminatorValue`
2. **`ExternalTypeReferenceResolver.CollectExternalTypes`/`VisitType`** (`Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs`)
- Seed: the whole `InputLibrary` (Models + Enums + Constants + every client method parameter & response body)
- Side effect: collects `InputExternalTypeMetadata` entries with non-null `Package` into a dictionary keyed by `(package, version, identity)`
Both walk: `InputModelType` → derived/discriminated subtypes → `BaseModel` → properties; and `InputArrayType` / `InputDictionaryType` / `InputNullableType` / `InputUnionType` value/variant types. Cycle detection via identity-based `HashSet`.
### Proposed shape
A `static class InputTypeWalker` (or instance `InputTypeVisitor`) in `Microsoft.TypeSpec.Generator.Input` that takes:
- A seed (`InputType`, `IEnumerable`, `InputNamespace`, or `InputLibrary` overloads)
- A visit callback that returns a `VisitAction` enum: `Continue` / `SkipChildren` / `Stop`
```csharp
public enum VisitAction { Continue, SkipChildren, Stop }
public static class InputTypeWalker
{
public static void Walk(InputType root, Func visit);
public static void Walk(InputNamespace ns, Func visit);
public static void Walk(InputLibrary library, Func visit);
}
```
Why a visitor-with-callback instead of `IEnumerable` on `InputLibrary` (the original suggestion):
- Each caller needs different control (side effects, pruning, cycle handling).
- `IEnumerable` would force callers to either accept the full enumeration cost or re-implement filtering.
- A callback also lets the special-case rules (e.g. `MarkModelsAsDynamicRecursive`'s base-discriminator walk) live inside a single owner-decision point rather than being scattered across the visitor and the consumer.
### Migration
1. Land `InputTypeWalker` with a comprehensive set of unit tests (cycles, every container type, every entry-point overload).
2. Migrate `MarkModelsAsDynamicRecursive` — must preserve the base-discriminator special rule and produce the same set of dynamically-marked models on the existing test corpus.
3. Migrate `ExternalTypeReferenceResolver.CollectExternalTypes` — must produce the same metadata set on the existing tests.
4. Delete the duplicated traversal code.
### Acceptance criteria
- One canonical type-graph walker in `Microsoft.TypeSpec.Generator.Input`.
- All existing `MarkModelsAsDynamicRecursive` and `ExternalTypeReferenceResolver` tests pass unchanged.
- New unit tests cover the walker directly (every node type, cycles, prune/stop semantics).
cc @JoshLove-msft (per discussion in microsoft/typespec#10666 (review thread r3231221827))
Contributor guide
Assessment
This issue has not been assessed yet.