Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overload resolution fails in complex scenario with Delegate overloads and extension methods #55691

Closed
davidfowl opened this issue Aug 18, 2021 · 4 comments · Fixed by #56341
Closed

Comments

@davidfowl
Copy link
Member

davidfowl commented Aug 18, 2021

Version Used:
Version 17.0.0 Preview 4.0 [31616.459.main]

Steps to Reproduce:

var app = new WebApp();

app.Map("/sub", builder =>
{
    builder.UseAuth();
});


class WebApp : IAppBuilder, IRouteBuilder
{
    public void UseAuth()
    {
        
    }
}

public interface IAppBuilder
{
    void UseAuth();
}

public interface IRouteBuilder
{

}

static class AppBuilderExtensions
{
    public static IAppBuilder Map(this IAppBuilder app, PathSring path, Action<IAppBuilder> callback) => app;
}

public static class RouteBuilderExtensions
{
    public static IRouteBuilder Map(this IRouteBuilder routes, string path, Delegate callback) => routes;
}

struct PathSring
{
    public PathSring(string? path)
    {
        Path = path;
    }

    public string? Path { get; }

    public static implicit operator PathSring(string? s) => new PathSring(s);

    public static implicit operator string?(PathSring path) => path.Path;
}

Expected Behavior:

AppBuilderExtensions.Map would be chosen by the overload resolution algorithm.

Actual Behavior:

The call is ambiguous between the following methods or properties: 'AppBuilderExtensions.Map(IAppBuilder, PathSring, Action)' and 'RouteBuilderExtensions.Map(IRouteBuilder, string, Delegate)' C:\Users\david\source\repos\ConsoleApp133\WebApplication1\Program.cs

It seems like the implicit conversion on PathString throws off the overload resolution completely. If PathString is replaced with string, it works as expected.

cc @cston

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 18, 2021
@cston cston self-assigned this Aug 18, 2021
@cston
Copy link
Member

cston commented Aug 18, 2021

Fixed in #55357.

@cston cston closed this as completed Aug 18, 2021
@halter73
Copy link
Member

halter73 commented Aug 18, 2021

@cston I checked that the above sample is indeed fixed using sharplab, but I noticed that a small change of specifying the parameter type brings back the same error.

app.Map("/sub", (IAppBuilder builder) =>
{
    builder.UseAuth();
});

This makes some sense given the PR was titled "Treat overload with Delegate parameter as not applicable if lambda delegate type cannot be inferred" and the delegate type can be inferred once we specify the parameter. But this does still seem like a bug to me. I would expect Roslyn to still prefer AppBuilderExtensions.Map(IAppBuilder, PathSring, Action<IAppBuilder>) in this case similarly to how it prefers RequestDelegate to Delegate in similar cases.

It seems like the implicit conversion on PathString is still throwing off the overload resolution because if I change AppBuilderExtensions.Map(IAppBuilder, PathSring, Action<IAppBuilder>) to AppBuilderExtensions.Map(IAppBuilder, string, Action<IAppBuilder>) everything works as expected even when I do specify (IAppBuilder builder) in the lambda args list.

@halter73 halter73 reopened this Aug 18, 2021
@cston
Copy link
Member

cston commented Aug 18, 2021

I noticed that a small change of specifying the parameter type brings back the same error.

Thanks for catching this @halter73, I'll investigate.

@cston
Copy link
Member

cston commented Sep 2, 2021

The ambiguity error for the second example (with explicit parameter type)

app.Map("/sub", (IAppBuilder builder) =>
{
    builder.UseAuth();
});

was resolved as "by design" in C# LDM-2021-08-23.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment