microsoft / microsoft/typespec

[http-client-csharp] Positional constructor parameter restoration can silently mis-bind after a reorder

Open
#11,787 1 comment 1 reaction 0 assignees View on GitHub
bug emitter:client:csharp
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.