Honor exact C# names over back-compat naming - #11663
Honor exact C# names over back-compat naming#11663Jorge Rangel (jorgerangel-msft) merged 25 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>
There was a problem hiding this comment.
Pull request overview
Ensures exact C# parameter names take precedence over names restored from previous contracts.
Changes:
- Skips back-compat renaming for exact service and property-derived parameters.
- Prevents duplicate model factory compatibility overloads.
- Adds regression tests for RestClient and model factory paths.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.../test/Providers/TypeProviderTests.cs |
Tests exact-name precedence. |
.../BuildMethodsForBackCompatibilityExactNameTakesPrecedence(Last)/TestClient.cs |
Defines the prior contract fixture. |
.../BuildMethodsForBackCompatibilityExactNameTakesPrecedence.cs |
Captures expected exact-name output. |
.../BackCompatibility_ExactPropertyNameTakesPrecedence(Last)/SampleNamespaceModelFactory.cs |
Defines the prior model factory contract. |
.../BackCompatibility_ExactPropertyNameTakesPrecedence.cs |
Captures expected model factory output. |
.../ModelFactoryProviderTests.cs |
Tests mixed exact and restored property names. |
.../Utilities/BackCompatHelper.cs |
Excludes exact parameters from name restoration. |
.../Providers/ParameterProvider.cs |
Exposes exact-name metadata for parameters. |
.../Providers/ModelFactoryProvider.cs |
Treats exact-name differences as compatible. |
.../ExactParameterNameTakesPrecedenceOverLastContractView(Last)/TestClient.cs |
Defines the prior RestClient contract. |
.../ExactParameterNameTakesPrecedenceOverLastContractView.cs |
Captures expected RestClient output. |
.../RestClientProviderTests.cs |
Adds RestClient naming regression coverage. |
.../RestClientProvider.cs |
Prevents last-contract names overriding exact names. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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 pull latest from main and address the merge conflicts
…n order Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
…nature comparison Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
JoshLove-msft
left a comment
There was a problem hiding this comment.
Review: honor exact C# names over back-compat naming
How I verified this. I checked out the PR into a worktree and ran the full Microsoft.TypeSpec.Generator.Tests suite — 2087/2087 pass. I then wrote throwaway probe tests against ModelProvider back-compat and instrumented ModelFactoryProvider to confirm reachability claims empirically rather than by reading. Everything below is backed by generated output or an instrumented run, not inference.
What's solid. The enum path is complete and correct end-to-end: GetGeneratedValueName already honored IsExactName, ExtensibleEnumProvider derives both the backing field and the public property from the same valueName, and all three GetBackCompatibleName call sites were updated. The IsExactName propagation on ParameterProvider is also complete — the property is private init, so every clone site must live in ParameterProvider.cs, there are exactly two (BuildInputVariant, WithRef), and both were updated.
The concerns below are about the shape of the guard in the two constructor/method back-compat paths, plus one finding about pre-existing fragility that this PR now depends on.
1. The guard is keyed on the wrong condition, and it hides a pre-existing silent mis-binding
The ModelProvider positional-restoration path can silently bind constructor arguments to the wrong properties, and it does so with no exact names involved at all. Probe: last contract MockInputModel(string oldName, string other), current properties other and newName (both ordinary, non-exact renames). Generated:
public MockInputModel(string oldName, string other)
{
Other = oldName;
NewName = other;
}A caller writing new MockInputModel(oldName: a, other: b) now silently gets the values swapped — it still compiles, and it is wrong.
The real precondition for "position can no longer be trusted" is "a previous parameter name appears in the current signature at a different index" — here other was at previous index 1 and is at current index 0. IsExactName is at best a proxy for that, and as shown it is neither necessary nor sufficient. The comment in BackCompatHelper says the concern is reordering; reordering is directly detectable. Keying on the reorder condition instead would fix the exact case and this non-exact case, and would remove the need for the type-keyed suppression in #2.
2. The type-keyed bail-out is over-broad and silent
Probe: last MockInputModel(string oldUnchanged, string oldName); current unchanged (non-exact rename) + new_name (exact). Result:
params = string unchanged, string newName
The unrelated oldUnchanged → unchanged rename is not restored, so the PR silently ships a source break that the feature exists to prevent. There is no ambiguity in this case — neither previous name appears anywhere in the current signature. Because the key is the type and string is ubiquitous, a single exact rename disables restoration for every string parameter in the constructor. The Emitter.Debug / ParameterNamePreserved message is skipped too, so nothing surfaces in the emitter log.
3. HasMatchingExactParameterNames only works because of an Equals/GetHashCode contract violation
I first assumed this helper was unreachable, since it opens by asserting MethodSignature.MethodSignatureComparer.Equals(current, previous) — the same comparer used to build currentMethodSignatures, which was already probed with Contains a few lines earlier. Deleting the clause proves otherwise: BackCompatibility_ExactPropertyNameTakesPrecedence fails, so it is load-bearing. But the reason is a bug:
##NOTFOUND PublicModel1(string oldStringProp, Thing oldModelProp, IEnumerable<string> listProp, IDictionary<string, string> dictProp)
ret=global::.PublicModel1 linearEquals=True
Contains returned false while a linear scan with the same comparer returned true. Cause: MethodSignatureBaseEqualityComparer.Equals ignores ReturnType for non-operators, but GetHashCode is HashCode.Combine(obj.Name, obj.ReturnType) — and the last-contract return type renders as global::.PublicModel1 (empty namespace) versus the generated global::Sample.Models.PublicModel1, so the two land in different buckets.
This is pre-existing and not caused by this PR, but it's worth flagging here because the whole rename-back-compat block — the new clause and the existing HaveSameParametersInSameOrder path — is reachable only when that hash lookup misses. Contains compares types and ignores names entirely, so if the hashing were ever normalized, it would continue on every pure rename and this entire block would silently go dead. Worth at least a comment, ideally a fix to the comparer.
4. "Exact" is honored for method parameters but not for constructor parameters
The PR description says @clientName(exact("api_key")) on a property makes "the generated C# parameter remain api_key", and IsExactName's doc comment says the name "must be used verbatim". That holds for method parameters — the RestClientProvider baseline emits CreateGetSomethingRequest(string exact_param, ...). It does not hold for model constructors. From BackCompat_ConstructorParameterExactNameNotRenamed.cs:
public MockInputModel(string unchanged, string newName)
{
...
new_name = newName;
}
public string new_name { get; }The property is verbatim; the parameter is camelCase-transformed to newName. Same in the model factory: BackCompatibility_ExactPropertyNameTakesPrecedence marks StringProp exact and the factory parameter is stringProp. Whichever behavior is intended, the two paths should agree and the doc comment plus PR description should match what actually ships.
5. Two near-identical helpers with divergent type comparison
ModelProvider.FindExactParameterNameMismatchTypes and BackCompatHelper.FindExactParameterNameMismatchIndices are the same loop, but the former compares types with a new private CSharpTypeNameComparer (namespace + name, ignoring generic arguments and nullability) and the latter with CSharpType.Equals. The last commit is titled "use CSharpType.Equals instead of AreNamesEqual for same-signature comparison" — only one of the two sites was changed. These should share one helper and one comparison. CSharpTypeNameComparer also duplicates the existing CSharpType.AreNamesEqual semantics.
6. Nits
GetBackCompatibleName(..., bool isExactName = false)— all three call sites pass the argument, so the optional default only creates a way to silently get the old behavior. Make it required.BuildMethodsForBackCompatibilityExactNameTakesPrecedencebuilds its input asInputFactory.QueryParameter("oldParam", ..., isExactName: true)followed byinputParameter.Update(name: "exact_param"), presumably to seedOriginalName. That's non-obvious; a one-line comment would help.- Model-factory
exactrenames are source-breaking by construction — you can't add a compatibility overload with identical parameter types. The PR accepts that silently; a diagnostic would make the tradeoff visible.
Overall this is heading the right way and the enum and ParameterProvider plumbing is clean. My main ask is #1/#2: replace the IsExactName-keyed guard with an actual reorder check ("a previous parameter name appears at a different index in the current signature"), which is both more correct and narrower than the current type-keyed suppression.
--generated by Copilot
…sions Positional parameter-name restoration was suppressed whenever any exact-name mismatch existed at a shared parameter type. Since `string` is the most common parameter type, a single exact rename silently disabled restoration for every other string parameter in the same constructor or method, shipping the source break the feature exists to prevent. Remove the type/index-keyed suppression from both `ModelProvider` and `BackCompatHelper` (along with `CSharpTypeNameComparer`) so non-exact parameters behave exactly as before. Exact parameters are still skipped, and a narrow guard now refuses only a restore that would collide with a retained exact name - which would otherwise emit a duplicate parameter - and logs why. Also make `EnumProvider.GetBackCompatibleName`'s `isExactName` required, correct the `ParameterProvider.IsExactName` doc comment, and document why the model factory overload scan has to be linear. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs:193
- This still allows a same-typed partial reorder to bind a published name to the wrong factory parameter. For example, with previous
(oldExact, other)and current(other, newExact),TryRestorePreviousParameterOrdercannot complete, positional fallback renamesothertooldExact, and this guard only skipsnewExact; calls usingoldExact:now populate theotherproperty. Preserve the survivingothermatch (or otherwise realign partially matched parameters) before applying positional fallback.
if (parameter.IsExactName)
{
continue;
}
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:953
- A constructor with previous
(oldExact, other)and current reordered(other, newExact)still reaches this branch with the current order because not every old name is present. The first parameter is then renamed tooldExactwhile the exact parameter is skipped, so the old named argument initializesOtherinstead of the exact property's slot. Partially realign parameters whose names still match (or use property identity where available) before restoring names positionally.
if (string.Equals(restoredParameters[i].Name, restoredName, StringComparison.Ordinal)
|| restoredParameters[i].IsExactName)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:952
- This has the same misbinding risk for reordered same-typed constructor properties. With previous
(oldExact, other)and current(other, newExact),currentByNamecannot form a complete permutation, so the loop keeps current order, renamesothertooldExact, and skips the exact slot here. A caller usingoldExact:then initializesotherrather than the renamed exact property. Match parameters through theirPropertyidentity, or skip restoration of same-typed non-exact slots when an exact-name mismatch makes positional correspondence ambiguous.
if (string.Equals(restoredParameters[i].Name, restoredName, StringComparison.Ordinal)
|| restoredParameters[i].IsExactName)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs:193
- For synthesized parameters this skip can still make positional fallback bind a previous name to the wrong value. For example, with previous
(oldExact: string, other: string)and current(other: string, newExact: string), order restoration cannot matcholdExact; model-factory parameters have noInputParameter, so line 206 then renamesothertooldExact, whilenewExactis skipped here. Existing calls usingoldExact:now initialize the wrong property. Match property-derived parameters by stable property identity, or suppress positional restoration where an unmatched exact parameter makes same-typed slots ambiguous.
if (parameter.IsExactName)
{
continue;
}
An exact parameter name rename on a model factory method cannot be shimmed with a compatibility overload, since the parameter types are unchanged. That tradeoff was accepted silently; log it so it is visible in the emitter output. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Thanks for the depth here — the empirical approach made these easy to act on. Closing out the three items that weren't covered by the inline threads, plus evidence on #1. #1 — confirmed pre-existing, with numbers. I reproduced your probe at HEAD and again at the merge-base ( So the mis-binding is real but is not introduced or widened by this PR. I didn't adopt the reorder check because it can't be distinguished from a chained rename: #2 / #5 — fixed. The type-keyed suppression and both mismatch helpers (plus #3 — acknowledged, not fixed here. Your diagnosis is correct and I verified it independently. We opted not to leave a comment in the end, since it documents a quirk in a different type rather than the code it sat on. Leaving the comparer alone deliberately: normalizing the hashing would make the loop #4 — doc and description corrected; behavior intentionally unchanged. Confirmed exactly as you described: RestClient emits #6a — done, #6b — done. Added a line explaining that #6c — done. The exact-name-compatible path now emits an Full suite is green at 2088/2088. 🤖 Generated by Jorge's Copilot |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs:192
- Skipping the exact slot still allows positional fallback to rename a different property-derived parameter onto the exact property's old slot. For example, previous
(oldExact, other)and current(other, newExact)(same types) fails the partial reorder; because factory parameters have noInputParameter,otheris then renamed positionally tooldExact, so the old named argument initializes the wrong property. Preserve a non-exact parameter that already matches another previous slot, or match property-derived parameters by stable identity before using positional fallback.
if (parameter.IsExactName)
{
continue;
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/ModelProvider.cs:952
- This has the same cross-slot misbinding for constructors: with previous
(oldExact, other)and current(other, newExact)of the same types, the partial name-based permutation fails, then the first parameter is renamed tooldExactand the exact parameter is skipped. Calls usingoldExact:now initializeOther, not the formerly first property. Match constructor parameters by property identity, or avoid positional restoration for a non-exact parameter that already matches a different previous slot.
if (string.Equals(restoredParameters[i].Name, restoredName, StringComparison.Ordinal)
|| restoredParameters[i].IsExactName)
…essage Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/BackCompatHelper.cs:193
- Skipping the exact slot still leaves positional fallback able to attach an old name to the wrong parameter after a partial reorder. With previous
Foo(oldExact, other)and currentFoo(other, newExact)(both strings, withnewExactexact),TryRestorePreviousParameterOrdercannot complete, thenotheris renamed tooldExactbefore the exact slot is skipped. A caller usingoldExact:now supplies the value forother. Please use the partial name matches to realign the unmatched parameter, or avoid positional restoration when correspondence is ambiguous, and cover this reordered case.
if (parameter.IsExactName)
{
continue;
}
… log Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Names explicitly configured with
exactcould be replaced by names restored from the previous contract. This made advanced client naming overrides ineffective when back compatibility was enabled.Naming precedence
Model factories
Coverage
The generated C# declaration remains
api_keyeven when the previous contract used another name.Note that
exactcontrols the declaration name. Parameters derived from a property — model constructor and model factory parameters — still go through the usualToVariableNametransformation, so the example above yields a propertyapi_keyinitialized from a parameter namedapiKey. Method parameters declared directly in the spec are emitted verbatim (api_key). This PR does not change that transformation; it only ensures the configured spelling is never replaced by a previously-published name.