Get more explicit help in choosing the right overload

[aklefdal]

Is your feature request related to a problem? Please describe.
When a .NET library written in C# has several overloads, it is hard for the compiler to choose right, as seen in this response to an issue: dotnet/runtime#45035 (comment)

"It seems that F# treats `Action<T>` as `Func<T,unit>`, but it still should be able to tell between two overloads. We will investigate this further."

Describe the solution you'd like
When both Action<T>, treated as Func<T,unit>, and another Func<T1,T2>, maybe have warnings or errors if types are not given explicitly.

Describe alternatives you've considered
It might be that the problem is the developer, overusing |> ignore, then we just have to live with it.

[smoothdeveloper]

@aklefdal, do you think the behaviour should be specific to Action<_> / Func<_> or work for overload resolutions that involve fitting lambdas literals into delegates in general?

@halter73 dotnet/runtime#45035 (comment) says

t's too bad F# prefers the overload expecting Func<T, TResult> over the overload that takes Action when the lambda signature is T -> unit. Maybe that can be fixed in the F# compiler.

To me, it feels a bit odd if the compiler was changed to treat the two specific delegates (just because they happen to be used in lots of place) rather than have it deal with all delegate types the same for the resolution. I'm not sure if there are already special casing for those.

It is also too bad that C# codebases use overloads on those two delegate types (I'm culprit of this myself, having authored such overloads...) just because there is no concept of returning unit (and still compiling to the same).

I like the suggestion of a warning, taking @cartermp sample:

open System

type C() =
    member _.M(a: Action<int>) = ()
    member _.M(f: Func<int, unit>) = ()
    
let c = C()
c.M(fun _ -> ())

once either of an overload making lambda literal ambiguous is added, the compiler could output something akin to

Potential ambiguous overload resolution:

• M(a: Action< int>)
• M(f: Func<int,unit>)

'M(f: Func<int,unit>)' was picked; use a type annotation or explicit delegate constructor to fix the warning.

I don't think going the warning route requires to do fslang-suggestion, I'm not clear if the logic to emit the warning would have noticeable cost in overload resolution itself, but it could be shortcircuited, I assume there is already infrastructure to determine if a given warning is active in the type checker environment.

[aklefdal]

I like that the compiler asks me to be explicit, instead of default silently choosing Func<T,TResult> over Action<T>. I think @smoothdeveloper gets it right:

Potential ambiguous overload resolution:

M(a: Action)
M(f: Func<int,unit>)

'M(f: Func<int,unit>)' was picked; use a type annotation or explicit delegate constructor to fix the warning.

Adding a warning should not be a breaking change for anyone. In order to get rid of the warning, everyone would have to be explicit in which overload is wanted. Explicit is better than implicit.

[smoothdeveloper]

Relevant code:

fsharp/src/fsharp/ConstraintSolver.fs

Lines 2652 to 2673 in cbfd0f6

	let c =
	(calledArg1.CalledArgumentType, calledArg2.CalledArgumentType) ||> compareCond (fun ty1 ty2 ->
	// Func<_> is always considered better than any other delegate type
	match tryTcrefOfAppTy csenv.g ty1 with
	| ValueSome tcref1 when
	tcref1.DisplayName = "Func" &&
	(match tcref1.PublicPath with Some p -> p.EnclosingPath = [| "System" |] | _ -> false) &&
	isDelegateTy g ty1 &&
	isDelegateTy g ty2 -> true
	// T is always better than inref<T>
	| _ when isInByrefTy csenv.g ty2 && typeEquiv csenv.g ty1 (destByrefTy csenv.g ty2) ->
	true
	// T is always better than Nullable<T> from F# 5.0 onwards
	| _ when g.langVersion.SupportsFeature(Features.LanguageFeature.NullableOptionalInterop) &&
	isNullableTy csenv.g ty2 &&
	typeEquiv csenv.g ty1 (destNullableTy csenv.g ty2) ->
	true
	| _ -> false)

I'll look some more for cases where they are arbitrary delegates rather than System.Action & System.Func, and try to see if there is an obvious way to add "postmortem warning in case of candidates having ambiguous delegate types" but this doesn't sound like an easy journey.

Right now, to me, it feels there should be a "refineable context" of the picked overload being threaded through that code, rather than having to do a further check post mortem, which as is, is starting a bit from scratch.

In this sense, the warning is going to be an extra cost in the method overload resolution logic.

It could also be just adding a mutable in ResolveOverloading to capture we passed through this "picked one delegate as better" and that being the "refineable context" would be good enough and the extra cost would only be added when this check passed.

Looking for guidance from the compiler maintainers as to "add a couple of mutables to track the overloads for purpose of this warning" being OK route or not.

[smoothdeveloper]

type A<'a> = delegate of 'a -> unit
type F<'a,'b> = delegate of 'a -> 'b

type CD() =
  member _.M(a: A<int>) = printfn "aa"
  member _.M(f: F<int, unit>) = printfn "ff"

let cd = CD()
cd.M(fun _ -> ())

error FS0041: A unique overload for method 'M' could not be determined based on type information prior to this program point. A type annotation may be needed.

Known type of argument: (int -> unit)

Candidates:

• member CD.M : a:A< int> -> unit
• member CD.M : f:F<int,unit> -> unit

It seems that the special casing for Func<_> makes the behaviour inconsistent, there is no ambiguity possible when it doesn't come into play.

It would be nice to understand better the motivation for the special casing to come in the first place.

It's too bad F# prefers the overload expecting Func<T, TResult> over the overload that takes Action when the lambda signature is T -> unit. Maybe that can be fixed in the F# compiler.

since there is already special casing for Func<_> and Func/Action overload seems to be common place, we should consider if, when the return type is unit, current special casing would say "Action is better", it may be better option than warning, and if the need for a warning still holds.

[smoothdeveloper]

The solution which would be easiest (and the best in not breaking principle of least surprise) would probably be the breaking change of removing the favouring of Func<_> and rely on the good error reporting around overloads as the "soother" to those that such change would upset.

The special rule feels odd, and doubling down like I propose in previous comment feels the same.

Warning is the middle ground, but if enabled by default, it is a soft breaking change already, also @cartermp said in the original issue:

There's a general problem of F# type inference combined with overloads that is unavoidable - an added overload can be a breaking change for F# callers that rely on type inference - and this is what hit. This is an example of that. Using explicit types or named parameters can help.

My personal choice would hence be removing the favouring, unless I understand better the implications of the original choice that brought it, and better understanding of negative impact.

[baronfel]

An alternative would be to do like what was done with the new XML validation work: establish a warning in the current SDK, and in a future SDK/compiler release elevate the warning as an error to allow for a gradual tightening over time. This gives users a bit of runway to pin their overloads ahead of time.

[smoothdeveloper]

@aklefdal / @baronfel I have some news (from #11538):

It is of course, not clear if this just works yet, and the test coverage on overload resolution doesn't seem to assert a lot of stuff (on the "identify expected overload" front, on the "fails resolution" front, it is probably a bit better).

[jkone27]

This would help so much adoption, many times e.g. when working with dotnet/aspnetcore, overloads suggestion doesnt show up, or the list of overloads is not available in a dropdown/selection.

[smoothdeveloper]

Is this in F# 9.0 plan which has F# method resolution and conversion improvements around delegates
entry.

I also want to point out that such critical BCL API TaskFactory.StartNew is still problematic as it was :

open System
open System.Threading.Tasks
let makeTask () = new Task<_>(fun () -> 1)
let task = makeTask()
//let runningTask = Task.Run(fun () -> task) // can't find overload
//let runningTask = Task.Run(Func<_>(fun () -> task)) // can't find overload
//let runningTask = Task.Run<_>(Func<_>(fun () -> task)) // can't find overload
//let runningTask : Task<_> = Task.Run<_>(Func<_>(fun () -> task)) // can't find overload
//let runningTask = Task.Run(Func<Task<_>>(fun () -> task)) // can't find overload
//let runningTask = Task.Run(Func<Task<int>>(fun () -> task)) // can't find overload
//let runningTask = Task.Run<int>(Func<Task<int>>(fun () -> task)) // can find overload!
//let runningTask = Task.Run<int>(Func<Task<_>>(fun () -> task)) // can find overload!
//let runningTask = Task.Run<int>(Func<_>(fun () -> task)) // can find overload!
let runningTask = Task.Run<int>(fun () -> task) // can find overload!

I think it impacts lots of dotnet API as other languages than F# need to rely on distinction between Func and Action.

It would be great to have the C# and F# team collaborate on how this should be nudged, and to do similar work on C# side, when it comes to use FSharp.Core types (which are pervasive in F# code) (@jaredpar).