[API Proposal]: An analyzer to warn for ambiguous overloads
- 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
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