microsoft / microsoft/typespec

[http-client-csharp] Model factory overload disambiguation should use C# overload-resolution analysis

Open
#11,805 1 comment 1 reaction 0 assignees View on GitHub
emitter:client:csharp feature
Dominant language
Java
Stars
5.9k
Forks
394
Avg merge
1d 23h
Merged PRs (30d)
104

Description

## Problem

`MethodSignatureHelper.GetMinimumRequiredParameterCount` decides how many leading parameters a generated model factory overload must require so it cannot collide with a published compatibility overload or a custom overload.

It reasons about **argument counts**, while C# overload resolution reasons about **argument types and forms**. Making the applicable argument counts disjoint is sufficient in the common case, but it cannot express several situations where two overloads stay ambiguous.

## Known gaps

### 1. Unordered implicit conversions

A positional type difference does not imply the position selects an overload:

```csharp
M(float count = default, string description = default);
M(decimal size, bool? enabled = default); // one-parameter prefix, as produced today

int value = 1;
M(value); // CS0121
```

`int` converts implicitly to both `float` and `decimal` and neither conversion is better.

Brute-forcing all 66 built-in numeric pairs against the compiler shows `decimal`↔`float` and `decimal`↔`double` are the only affected built-in pairs — every other pair is ordered either by an implicit conversion (`int`→`long`) or by the signed/unsigned better-conversion-target rules (`int` beats `uint`, `short` beats `ushort`). User-defined implicit conversions are unbounded and invisible to the current type-name comparison.

### 2. Equal-arity overloads

Two overloads with the same parameter count cannot be disambiguated by optionality at all, because no required-prefix length separates them:

```csharp
A(string id, string description, string text, bool? isRegex); // generated
A(string id = default, string description = default,
string text = default, string isRegex = default); // custom

A("a", "b", "c", null); // CS0121 regardless of the prefix required
```

### 3. Named arguments

Named arguments can keep both candidates applicable no matter which prefix is required, since they bypass positional matching:

```csharp
M(id: "i", name: "n", kind: "k"); // CS0121
```

#### Verified end-to-end example

This shape is reachable from the generator today. Given a last contract of

```csharp
public static CompatibilityModel CompatibilityModel(string id = default, int count = default);
```

and a current model exposing a required `Count` (`int`) plus an optional `Description` (`string`), the generator emits:

```csharp
public static CompatibilityModel CompatibilityModel(int count, string description = default)

[EditorBrowsable(EditorBrowsableState.Never)]
public static CompatibilityModel CompatibilityModel(string id = default, int count = 0)
```

Compiling a consumer against that pair:

| Call | Result |
| --- | --- |
| `CompatibilityModel(count: 1)` | **CS0121 ambiguous** |
| `CompatibilityModel(id: "x")` | ok |
| `CompatibilityModel("x", 1)` | ok |
| `CompatibilityModel(1)` | ok |

Only the named form breaks: each overload omits exactly one optional parameter, so neither wins the "fewer omitted optional parameters" tie-break.

The required-prefix computation is not what creates this. `count` is required because the property is required, and `GetMinimumRequiredParameterCount` returns the same value the natural signature would have had. The pre-PR shape (`M(string id = default, int count = default)` versus `M(int count = default, string description = default)`) is ambiguous for `M(count: 1)` as well.

Making `description` required too does resolve this particular call, but that direction -- requiring parameters that correspond to newly added optional properties -- regresses existing back-compat expectations, which is why it needs the conversion-aware analysis described below rather than a targeted tweak.

### 4. Parameter modifiers

`ref`/`out` and `params` affect applicability and are only partially accounted for.

## Proposed work

Replace the count-based heuristic with conversion-aware applicability and better-function-member analysis, ideally driven by Roslyn's `Compilation.ClassifyConversion` rather than a hand-maintained conversion table. Where an ambiguity cannot be resolved by adjusting optionality (gap 2), the generator should detect it and report a diagnostic instead of silently emitting overloads that do not compile for some call shapes.

Suggested acceptance criteria:

- Consumer-compilation regressions covering unordered numeric conversions, named arguments, `null` literals, equal-arity overloads, and parameter modifiers.
- A diagnostic when two emitted overloads cannot be made unambiguous.

## Context

Found while reviewing #11703, which introduced the required-prefix logic to preserve published model factory optionality. These gaps are **not regressions from that PR** — before it, the competing overloads were fully optional and the same calls were already ambiguous. #11703 narrows the problem but does not close it.

Relevant code: `packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Shared/MethodSignatureHelper.cs` (`GetMinimumRequiredParameterCount`, `RequireMinimumParameterPrefix`).

Contributor guide

Open the contributing guide

Research direction

Start in packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Shared/MethodSignatureHelper.cs, reading GetMinimumRequiredParameterCount and RequireMinimumParameterPrefix. Trace the generated model-factory overloads and consumer-compilation coverage for numeric conversions, named arguments, null literals, equal arity, and parameter modifiers. Done means ambiguous emitted overloads are detected with a diagnostic and the listed regressions are covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend-api-design, compilers, testing-qa
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.