[WIP] Add support for deterministic naming in model suffix - #11999
Jorge Rangel (jorgerangel-msft) with Copilot wants to merge 9 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
commit: |
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
| var resultName = $"{normalizedName[..^responseSuffix.Length]}Result"; | ||
| var inputNamespace = CodeModelGenerator.Instance.InputLibrary.InputNamespace; | ||
| // Model and enum files share a flat output directory, even across namespaces. | ||
| return inputNamespace.Models.Any(model => HasConflictingName(model, model.Namespace)) || |
There was a problem hiding this comment.
Copilot why do we need to check with conflicting names?
There was a problem hiding this comment.
The rename can introduce a collision that wasn’t in the input: WidgetResponse and WidgetResult would both become WidgetResult. Models and enums also share the flat Generated/Models directory, so even different namespaces can collide on filenames. The checks retain WidgetResponse when the target is already occupied, including through a customization, rather than emitting duplicate types or overwriting generated files.
There was a problem hiding this comment.
Copilot is that something we have to check though? doesn't our codewriter and type resolution handle that for us ?
There was a problem hiding this comment.
Not for this kind of collision. I traced the paths:
CodeWriterqualifies type references, but writes declaration names unchanged. Its unique-name allocation applies toCodeWriterDeclarationidentifiers, not model types.TypeFactoryregisters the resulting names; it doesn’t allocate a different name on collision.ModelProviderusesGenerated/Models/{Name}.cs, andCSharpGenwrites directly to that path.
So namespace qualification handles A.WidgetResult versus B.WidgetResult references, but not two A.WidgetResult declarations—or duplicate output paths. We need prevention somewhere before emission; it doesn’t necessarily have to live in this naming helper.
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Jorge Rangel (jorgerangel-msft)
left a comment
There was a problem hiding this comment.
Copilot I'd like to see unit tests where a model name is not normalized if it is found in the last contract. Similarly, tests to validate that the isExactName flag is honored for model names
…lity Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Added dedicated |
| return inputNamespace.Models.Any(model => HasConflictingName(model, model.Namespace, resultName)) || | ||
| inputNamespace.Enums.Any(@enum => HasConflictingName(@enum, @enum.Namespace, resultName)) || | ||
| inputNamespace.Clients.Any(client => HasConflictingName(client, typeNamespace, resultName)) | ||
| ? normalizedName | ||
| : resultName; |
There was a problem hiding this comment.
Copilot is this check deterministic ? that is, will the outcome of this always be the same given the same set of types ?
There was a problem hiding this comment.
This check is deterministic because it only asks whether any model/enum/client reserves the target Result name; the boolean outcome does not depend on traversal order. In 9d543b68, I extracted it into HasConflictingResultName to make that scope clearer.
| return false; | ||
| } | ||
|
|
||
| customType = sourceInputModel.FindForTypeInCurrentCompilation( |
There was a problem hiding this comment.
Copilot why do we have to call this 3 separate times ?
There was a problem hiding this comment.
Addressed in 9d543b68 by consolidating the current-compilation probes behind FindCustomizationType and a single ordered lookup-name helper.
| sourceInputModel.FindForTypeInLastContract(otherNamespace, otherName) is null && | ||
| (normalizedOtherName == otherName || | ||
| sourceInputModel.FindForTypeInLastContract(otherNamespace, normalizedOtherName) is null); |
There was a problem hiding this comment.
Copilot why do we have to call this more than once ?
There was a problem hiding this comment.
Addressed in 9d543b68 by moving the last-contract probes into HasLastContractName, so the call sites no longer duplicate that lookup logic.
| private protected override TypeProvider? BuildCustomCodeView(string? generatedTypeName = null, string? generatedTypeNamespace = null) | ||
| { | ||
| var typeNamespace = generatedTypeNamespace ?? BuildNamespace(); | ||
| var typeName = generatedTypeName ?? BuildName(); | ||
| var customCodeView = base.BuildCustomCodeView(typeName, typeNamespace); | ||
| if (customCodeView is not null || _inputModel.IsExactName) | ||
| { | ||
| return customCodeView; | ||
| } | ||
|
|
||
| var originalName = _inputModel.Name.ToIdentifierName(); | ||
| if (!originalName.EndsWith(ResponseSuffix, StringComparison.Ordinal) || | ||
| originalName == typeName || | ||
| typeName != NormalizeTypeName(originalName)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return base.BuildCustomCodeView(originalName, typeNamespace) ?? | ||
| base.BuildCustomCodeView(originalName.NormalizeCSharpAcronyms(), typeNamespace); | ||
| } | ||
|
|
||
| private protected override TypeProvider? BuildLastContractView(string? generatedTypeName = null, string? generatedTypeNamespace = null) | ||
| { | ||
| var typeNamespace = generatedTypeNamespace ?? CustomCodeView?.Type.Namespace ?? BuildNamespace(); | ||
| var typeName = generatedTypeName ?? CustomCodeView?.Name ?? BuildName(); | ||
| var lastContractView = base.BuildLastContractView(typeName, typeNamespace); | ||
| if (lastContractView is not null || _inputModel.IsExactName) | ||
| { | ||
| return lastContractView; | ||
| } | ||
|
|
||
| var originalName = _inputModel.Name.ToIdentifierName(); | ||
| if (!originalName.EndsWith(ResponseSuffix, StringComparison.Ordinal) || | ||
| originalName == typeName || | ||
| typeName != NormalizeTypeName(originalName)) | ||
| { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Copilot can we consolidate some of this shared logic into a private helper ?
There was a problem hiding this comment.
Consolidated in 9d543b68: custom/current lookup candidates now share FindCustomizationType/GetCustomizationLookupNames, and custom-code plus last-contract response fallbacks share BuildResponseSuffixFallbackView.
|
|
||
| namespace Sample.Models | ||
| { | ||
| [CodeGenType("GadgetResult")] |
There was a problem hiding this comment.
Copilot lets consider this other scenario for a unit test:
A model that doesn't have a previous GA surface is called FooModel and contained a custom implmentation with a [CodeGenType("FooResponse")] annotation to do the rename. Will this rename still be honored with our changes? if not, should it ?
There was a problem hiding this comment.
Added this coverage in 9d543b68. A custom FooModel with [CodeGenType("FooResponse")] and no last contract is still honored; the generated model uses FooModel through CustomCodeView.
There was a problem hiding this comment.
🟡 Changes recommended
Collision checks must account for customized CLR names before preserving the Response suffix.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds deterministic C# model naming by converting eligible Response suffixes to Result while preserving compatibility and avoiding known collisions.
Changes:
- Adds response-to-result normalization and compatibility lookup logic.
- Expands naming, customization, collision, and enum tests.
- Regenerates affected C# clients, models, serializers, and paging helpers.
File summaries
| File | Description |
|---|---|
| packages/http-client-csharp/generator/TestProjects/Spector/http/type/union/src/Generated/TypeUnionModelFactory.cs | Updates model factory naming. |
| packages/http-client-csharp/generator/TestProjects/Spector/http/type/union/src/Generated/StringsOnly.cs | Uses the renamed result model. |
| packages/http-client-csharp/generator/TestProjects/Spector/http/type/union/src/Generated/Models/GetResult.Serialization.cs | Adds renamed model serialization. |
| packages/http-client-csharp/generator/TestProjects/Spector/http/type/union/src/Generated/Models/GetResult.cs | Renames the generated model. |
| packages/http-client-csharp/generator/TestProjects/Spector/http/type/union/src/Generated/Models/GetResponse.Serialization.cs | Removes obsolete serialization output. |
| packages/http-client-csharp/generator/TestProjects/Spector/http/type/union/src/Generated/Models/_TypeUnionContext.cs | Registers the renamed model. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/SampleTypeSpecModelFactory.cs | Renames model factory methods. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/SampleTypeSpecClient.cs | Updates anonymous-model return types. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Notebooks.cs | Updates notebook return types. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/SampleTypeSpecContext.cs | Registers renamed sample models. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/ReturnsAnonymousModelResult.Serialization.cs | Renames serialization members. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/ReturnsAnonymousModelResult.cs | Renames the anonymous result model. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/ListWithStringNextLinkResult.Serialization.cs | Renames string-link serialization. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/ListWithStringNextLinkResult.cs | Renames the string-link model. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/ListWithNextLinkResult.Serialization.cs | Renames next-link serialization. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/ListWithNextLinkResult.cs | Renames the next-link model. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/ListWithContinuationTokenResult.Serialization.cs | Renames token serialization. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/ListWithContinuationTokenResult.cs | Renames the token model. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/ListWithContinuationTokenHeaderResponseResult.Serialization.cs | Renames header-result serialization. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/ListWithContinuationTokenHeaderResponseResult.cs | Renames the header-result model. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/GetWidgetMetricsResult.Serialization.cs | Renames metrics serialization. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/GetWidgetMetricsResult.cs | Renames the metrics model. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/GetNotebookResult.Serialization.cs | Renames notebook serialization. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Models/GetNotebookResult.cs | Renames the notebook model. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/Metrics.cs | Updates metrics return types. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithStringNextLinkCollectionResultOfT.cs | Updates typed string-link paging casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithStringNextLinkCollectionResult.cs | Updates string-link paging casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithStringNextLinkAsyncCollectionResultOfT.cs | Updates typed async paging casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithStringNextLinkAsyncCollectionResult.cs | Updates async string-link casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithNextLinkCollectionResultOfT.cs | Updates typed next-link casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithNextLinkCollectionResult.cs | Updates next-link paging casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithNextLinkAsyncCollectionResultOfT.cs | Updates typed async next-link casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithNextLinkAsyncCollectionResult.cs | Updates async next-link casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithContinuationTokenHeaderResponseCollectionResultOfT.cs | Updates header paging casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithContinuationTokenHeaderResponseAsyncCollectionResultOfT.cs | Updates async header paging casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithContinuationTokenCollectionResultOfT.cs | Updates typed token paging casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithContinuationTokenCollectionResult.cs | Updates token paging casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithContinuationTokenAsyncCollectionResultOfT.cs | Updates typed async token casts. |
| packages/http-client-csharp/generator/TestProjects/Local/Sample-TypeSpec/src/Generated/CollectionResults/SampleTypeSpecClientGetWithContinuationTokenAsyncCollectionResult.cs | Updates async token paging casts. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/TestBuildName_ResponseSuffixPreservesExistingName/ExistingModels.cs | Supplies existing-contract fixtures. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/TestBuildName_ResponseSuffixPreservesCustomName/CustomizedModels.cs | Supplies customization fixtures. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/TestBuildName_ResponseSuffixIgnoresResultAliasForShippedModel/WidgetResult.cs | Tests result aliases with shipped models. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/TestBuildName_ResponseSuffixIgnoresResultAliasForShippedModel(LastContract)/IPResponse.cs | Supplies the prior-contract fixture. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/TestBuildName_ResponseSuffixAvoidsResultCustomizationAliasCollision/WidgetResult.cs | Tests result-alias collisions. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/TestData/ModelProviderTests/TestBuildName_ResponseSuffixAvoidsCustomizationCollision/WidgetResult.cs | Tests customization collisions. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/ModelProviders/ModelProviderTests.cs | Adds deterministic naming tests. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Providers/EnumProviders/EnumProviderTests.cs | Confirms enums remain unaffected. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs | Makes type normalization overridable. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs | Implements response-to-result naming. |
| packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/ReferenceMap/ClientBodyDependencyReferenceMapTests.cs | Updates reference-map expectations. |
Review details
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:429
- The response-suffix compatibility lookup is duplicated here and again in
BuildLastContractView(lines 432-451), including the same candidate-name and fallback sequence. Keeping two copies means a future fix to customization matching can silently diverge from last-contract matching; extract the shared candidate-resolution logic and pass the view-specific lookup into it.
var originalName = _inputModel.Name.ToIdentifierName();
if (!originalName.EndsWith(ResponseSuffix, StringComparison.Ordinal) ||
originalName == typeName ||
typeName != NormalizeTypeName(originalName))
{
return null;
}
return base.BuildCustomCodeView(originalName, typeNamespace) ??
base.BuildCustomCodeView(originalName.NormalizeCSharpAcronyms(), typeNamespace);
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:344
NormalizeTypeNameruns this scan for everyResponsemodel, and each sibling can trigger up to three Roslyn compilation lookups below, making provider construction quadratic in the number of input types for large libraries. Cache the current/last-contract customization names (or build a single lookup set) once per generator run instead of querying the compilations inside the nested scan.
return inputNamespace.Models.Any(model => HasConflictingName(model, model.Namespace, resultName)) ||
inputNamespace.Enums.Any(@enum => HasConflictingName(@enum, @enum.Namespace, resultName)) ||
inputNamespace.Clients.Any(client => HasConflictingName(client, typeNamespace, resultName))
? normalizedName
: resultName;
- Files reviewed: 11/50 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| var otherName = inputType.IsExactName ? inputType.Name : inputType.Name.ToIdentifierName(); | ||
| // Acronym normalization only changes casing, so this also covers the normalized filename. | ||
| if (string.Equals(otherName, resultName, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| var sourceInputModel = CodeModelGenerator.Instance.SourceInputModel; | ||
| if (sourceInputModel.Customization is null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| var otherNamespace = string.IsNullOrEmpty(inputTypeNamespace) | ||
| ? CodeModelGenerator.Instance.TypeFactory.PrimaryNamespace | ||
| : CodeModelGenerator.Instance.TypeFactory.GetCleanNameSpace(inputTypeNamespace); | ||
| var customType = sourceInputModel.FindForTypeInCurrentCompilation(otherNamespace, otherName); | ||
| if (customType is not null || inputType.IsExactName) | ||
| { | ||
| return string.Equals(customType?.Name, resultName, StringComparison.OrdinalIgnoreCase); | ||
| } |
There was a problem hiding this comment.
Copilot I believe this is the question I asked earlier. I agree with the assessment here
There was a problem hiding this comment.
Agreed and fixed in 65849681. The model collision check now resolves a sibling customization first and only falls back to raw input-name collision when no customization is found. I also added a regression test where WidgetResponse normalizes to WidgetResult while the sibling WidgetResult is customized to CustomWidgetResult.
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.