microsoft / microsoft/typespec
[http-client-csharp] Positional constructor parameter restoration can silently mis-bind after a reorder
- Dominant language
- Java
- Stars
- 5.9k
- Forks
- 394
- Avg merge
- 1d 23h
- Merged PRs (30d)
- 104
Description
## Summary
`ModelProvider.RestorePreviousConstructorParameterNames` restores previously-published parameter names **positionally** when it cannot realign them by name. When the current signature is a reorder of the previous one, this binds a caller's named argument to the wrong property — and it still compiles, so the failure is silent.
This is long-standing behavior, not a regression. It was surfaced during review of #11663 but confirmed present on `main`.
## Reproduction
Last contract:
```csharp
public MockInputModel(string oldName, string other)
```
Current properties (both ordinary, non-exact renames): `other` and `newName`. Generated:
```csharp
public MockInputModel(string oldName, string other)
{
Other = oldName;
NewName = other;
}
```
A caller written against the previous contract:
```csharp
new MockInputModel(oldName: a, other: b);
```
still compiles, but `a` and `b` are now silently swapped into the wrong properties.
## Verification
Reproduced with a throwaway probe at `210f84c0d` (merge-base of #11663) and at that PR's head. Both emit identical output:
```
params=String oldName, String other
props=Other, NewName
```
The same shape reproduces when one property uses `exact` naming, again identically on both revisions:
```
merge-base: oldExact->Other | other->new_exact
PR head: oldExact->Other | newExact->new_exact
```
Position 0 — the mis-binding — is unchanged in every case.
## Why this is not trivially fixable
The obvious rule is *"a previous parameter name appears in the current signature at a different index"*. That does detect this case, but it also matches a **chained rename**, which is currently supported and asserted by `BackCompat_ConstructorParameterChainedRenameRestoredBySignatureMatch`:
- previous `(name, vmName)`, current `(skuName, name)` — previous `name` reappears at index 1
- that test asserts restoration **should** happen, because `Name`→`SkuName` and `VmName`→`Name` is a chain and position is the correct mapping
Nothing in the model distinguishes the two: the last contract carries only C# names, with no stable identity (property/spec identity) to match on. So a naive reorder check would trade this silent bug for a regression in supported behavior.
## Possible directions
- Match previous parameters to current ones by a stable identity (serialized/spec name or property identity) rather than by C# name plus position, which would disambiguate reorder from chained rename.
- Alternatively, decline positional restoration when the mapping is genuinely ambiguous and emit a diagnostic, accepting a loud compile break instead of a silent mis-binding.
- Either way, keep `BackCompat_ConstructorParameterChainedRenameRestoredBySignatureMatch` green, and add regressions for the reorder cases above (both the all-non-exact and the exact-name variants).
## Related
- Raised during review of #11663 (review [`5036785893`](https://github.com/microsoft/typespec/pull/11663#pullrequestreview-5036785893), item 1, and [`r3881736068`](https://github.com/microsoft/typespec/pull/11663#discussion_r3881736068)). That PR deliberately left this behavior unchanged for non-exact parameters.
Contributor guide
Research direction
Start with ModelProvider.RestorePreviousConstructorParameterNames and the existing BackCompat_ConstructorParameterChainedRenameRestoredBySignatureMatch test. Reproduce the positional reorder cases described in the issue, including the exact-name variant, then compare the mapping with the chained-rename behavior. Done means the reorder no longer silently mis-binds, the supported chained rename remains green, and regression coverage documents the chosen behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend-api-design
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100