dotnet / dotnet/runtime

[API Proposal]: An analyzer to warn for ambiguous overloads

Open
#133,323 1 comment 0 reactions 0 assignees View on GitHub
api-suggestion area-Meta code-analyzer
Dominant language
C#
Stars
18.3k
Forks
5.6k
PR merge metrics
PR metrics pending

Description

### Background and motivation

There are some APIs with multiple 'ambiguous' overloads. One example is `MLDsa.SignData`, with these given two methods:
`void SignData(ReadOnlySpan data, Span destination, ReadOnlySpan context = default)`
and
`byte[] SignData(byte[] data, byte[]? context = default)`.

Let's take this code snippet for using the `SignData` method:

```csharp
using var mldsa = MLDsa.GenerateKey(MLDsaAlgorithm.MLDsa44);
Span message = new byte[1];
var signature = new byte[MLDsaAlgorithm.MLDsa44.SignatureSizeInBytes];
mldsa.SignData(message, signature);
```

This will use the ROS overload.

However, when changing the types declaration from `Span` to `var` (e.g. `var` obsession):

```csharp
using var mldsa = MLDsa.GenerateKey(MLDsaAlgorithm.MLDsa44);
var message = new byte[1];
var signature = new byte[MLDsaAlgorithm.MLDsa44.SignatureSizeInBytes];
mldsa.SignData(message, signature);
```

will result that the overload with the `context` parameter is invoked, resulting a runtime exception only (because a context object is expected as opposed a differently sized array).

However, spotting this error by reading the code or on a code review is very difficult.

### API Proposal

Comparing the two examples does not reveal the issue even for trained eyes.

Hence the proposal is to create an analyzer rule that can help to catch these issues. This is discussed in https://github.com/dotnet/runtime/issues/133129#issuecomment-5545251248

Instead of an `MLDsa` specific analyzer, I also think a general purpose analyzer should be feasible. I would consider the following rule: emit a diagnostics (info/warning), for call-sites where a semantically non-equivalent overload is available and can be matched by the compiler (because being type equivalent).

I think the *semantically non-equivalent* means type parameters that are

- named differently (e.g.: `destination` vs. `context`),
- and one is optional, where the parameter in the same position in the other overload is not.

I think the BCL APIs are well-designed in the sense that such issues mostly occurs for overloads with

- `Span`, `byte[]`, `ReadOnlySpan` (the closed generic types),
- return value is ignored at the call-site

So an analyzer could also consider these two additional conditions.

### API Usage

API usage is not relevant for the proposal.

### Alternative Designs

I understand that there are many less constrained examples like the one below, also facing a similar issue:

```csharp
var/Base o = new Derived();
Console.WriteLine(Foo.Method(o));

public static class Foo
{
public static string Method(Base o) => "Overload1";
public static string Method(Derived o) => "Overload2";
}

public class Base { }
public class Derived : Base { }
```

But I am not convinced that the proposed analyzer rule should catch all these cases - I don't think there are many cases like this in the BCL anyway.

---
However, I could imagine a *second* analyzer that emits a warning/error at the declaration of the overloads in type `Foo`, so that the author `Foo` is warned: "your overloads may cause ambiguity".

### Risks

_No response_

Contributor guide

Open the contributing guide

Research direction

No implementation files, tests, or entry points are named. Start by reading the linked discussion in dotnet/runtime#133129 and the analyzer proposal, then define a focused rule scope and its diagnostic behavior; done means an agreed analyzer design with coverage for the described ambiguous-overload call pattern.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
devtools
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.