F# 6.0 -> 7.0: A unique overload for method could not be determined

[pkese]

Please provide a succinct description of the issue.

I had the following TorchSharp code working fine in F# 6.0:

let xs = torch.zeros([|10L|], dtype=torch.float32)

After upgrading to F# 7.0 the compiler started complaining:

*A unique overload for method 'zeros' could not be determined based on type information prior to this program point. A type annotation may be needed.Known types of arguments: int64 array * dtype: torch.ScalarType * device: torch.Device * requiresGrad: bool

Candidates:
- torch.zeros(size: ReadOnlySpan<int64>, ?dtype: Nullable<torch.ScalarType>, ?device: torch.Device, ?requiresGrad: bool) : torch.Tensor 
- torch.zeros(size: ReadOnlySpan<int64>, ?dtype: Nullable<torch.ScalarType>, ?device: torch.Device, ?requiresGrad: bool) : torch.Tensor 
- torch.zeros(size: int64 array, ?dtype: Nullable<torch.ScalarType>, ?device: torch.Device, ?requiresGrad: bool) : torch.Tensor*

It does work in F# 7.0 if I change the code to:

let xs = torch.zeros([|10L|], ?dtype=Some torch.float32)

... but having to sprinkle this all over the code (in this way) is clumsy and hinders readability.

Yes, there's a workaround,
but from a usability perspective, I'd call this a regression.

[pkese]

A sample reproduction code:

#!/usr/bin/env -S dotnet fsi

#r "nuget: TorchSharp-cpu, 0.99.0"

open TorchSharp

let xs = torch.zeros([|10L|], dtype=torch.float32)

[T-Gro]

Thank you for filing it, this has definitely high impact.
Is the behavior the same for all arguments you encountered, or just the Nullable<> ones?

[edgarfgp]

Might it be similar to #13731 ?

[vzarytovskii]

@pkese out of curiosity, does it fix it when you're setting the language version to 6?

<LangVersion>6</LangVersion>

or --langversion:6

Update: it seems, it's not.

[vzarytovskii]

I suspect it might've been caused by the changes in #13673

[NinoFloris]

I did some digging and this is a fun one, very much related to #13673 indeed.

In that PR Nullable conversions became integrated into TDC (Type-Directed Conversion) proper, meaning every Nullable conversion is now also a TDC. This was done to fix bug #12860 but in doing so we've exposed a flaw in the way the compiler picks a candidate between two overloads that both have conversions.

For the following overload set (minimized from TorchSharp):

type ScalarType =
    | Float32 = 0

type Tensor =
    static member zeros(size: int64 array, dtype: System.Nullable<ScalarType>) =
        Unchecked.defaultof<Tensor>
    static member zeros(size: System.ReadOnlySpan<int64>, dtype: System.Nullable<ScalarType>) =
        Unchecked.defaultof<Tensor>

Given the call that reproduces the issue:

Tensor.zeros([|10L|], dtype=ScalarType.Float32)

Whether the first or second overload would match always had to rely on a conversion of some form for the second argument (from ScalarType to Nullable<ScalarType>) starting from F# 5 when the nullable interop feature was released.

In F# 6 TDC was added and we narrowly avoided hitting this bug because overload 1 had no 'true' TDCs while the second overload would (as ROSpan has an implicit conversion for any array).

Since F# 7 where Nullable conversions are now TDC conversions we get into the problematic part. For this call both applicable methods become marked as type directed under the new logic, the nullable argument causes the first one to be so and the ROSpan one will have two conversions applied, one for each argument.

After this point our overload rules are not granular enough to pick a better candidate, the TDC status in the resolution is only granular up to a candidate, not per argument.

I just want to clarify that before #13673 this was already a problem. The issue is just expanded now that Nullable is included, causing it to show up more frequently. Take the following example:

type Tensor =
    static member zeros(size: int64 array, dtype: int64) =
        Unchecked.defaultof<Tensor>
    static member zeros(size: System.ReadOnlySpan<int64>, dtype: int64) =
        Unchecked.defaultof<Tensor>

let test() = Tensor.zeros([|10L|], 1)

And try this on sharplab or an F# 6 compiler (the repro still compiles over there which must mean it's not yet on F# 7)
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AbEAzAzgHwgAcYA7AAgGUBPXAFxgFsBYAKDbupPIBUzdo5ALxtyY8vQCGdAJZhyjJsBhRyALxURcAClwyNIcjNJ1U5SVCiTqacgBNOJQwDkArhgyTgGGAB5jdAB8AJTCouIRAKqkYAAWMGAA1jB2AHR2MNiS7nQQ2L58pAJQgeFiUrLyiozKqhpQWrr6MIY09EypAEowknYA8qQY1JREkqT+JqiBtg5cLeRuHl4+E0GhIqwRUTHxSSnpmdkYufmFxaWbW2JsPnTkDPTa67z80Kn1jQDa+ACMAAwAGXwAF1bD9gmwOHMXkVoAAmMKXcp0aRyBRKFTqTQ6PQGIyTFDmSzWGaOeYBVDrMpbaJxBLJNIZLI5PIFV5QOEXCIVNHVWpYho45qtWgMRhdHr9QbDUbjCkoab2MmGeVUpE0nb0/ZMo4nNmwjlcq7kG4wO4POhwp7CGHFOHvbHab7/IGg8jgoA==

You'll notice that calls like these also produce the same issue on F# 6, it's just problematic that we've expanded the issue to cover Nullable, which likely sees quite a bit more use of this feature than numeric and op_implicit conversions. There are also fairly big verbosity benefits to using it.

We have a few options, from least to more involved/risky:

1. Bring back the Nullable special case by backing out the fix for Overload resolution for Result cannot be determined when a method for Nullable exists #12860.
2. Count all type directed conversions per candidate.
3. Special casing Nullable in the context of TDC.
4. Per argument TDC checks.

My thoughts on each option:

1. May be best if the first 7.0 servicing release is too soon to land and validate a decent fix.
2. Counting all TDC uses, we already do something similar on the argument level with isTwoStepConversion and we already have a rule for that one. It would be a sufficient tiebreaker for this particular bug but it still would not catch disjoint overload sets where two candidates both need to convert a different argument totaling to the same TDC count.
3. This approach brings back more-or-less the same overload behavior as F# 6 displayed. We would do so by adding a new rule that prefers candidates that only have nullable type directed conversions over candidates that need other conversions as well. It suffers from the same issues as 3 and doesn't really make the overall situation simpler in my view.
4. This would solve the issue in the right way, but needs quite a bit of rework. It is quite risky to push into a 7.0 patch and I would need to collaborate with @dsyme so we can convince one another that we won't regress anything new.

@dsyme / @vzarytovskii I'm available on slack or we can hop on a call if that helps.

[vzarytovskii]

1. May be best if the first 7.0 servicing release is too soon to land and validate a decent fix.

Yeah, won't likely be an option for us, we would like to properly test such fix before we service it.

[NinoFloris]

Yeah, won't likely be an option for us, we would like to properly test such fix before we service it.

What do you mean by that? It would mostly mean reverting this specific commit da5d49b

[vzarytovskii]

Yeah, won't likely be an option for us, we would like to properly test such fix before we service it.

What do you mean by that? It would mostly mean reverting this specific commit da5d49b

Oh, I misunderstood the statement. Yeah, reverting it might me an option, or just hiding new logic under language preview flag.

[dsyme]

My instinct is we should indeed prefer overloads involving some kinds of TDC conversions over others.

Roughly in preference order:

• Anything where the Nullable conversion relates to an optional arg
• Anything else involving Nullable
• Anything else involving numeric or op_Implicit conversions

@NinoFloris What's your judgment - would that resolve this and is it implementable? Thanks! :)

[NinoFloris]

After discussing privately with @dsyme we concluded that for a fix here we should prefer nullable TDCs. I've implemented it in #14319. I have also opened #14318 to track the previously mentioned example which reproduces in F# 6 of an ambiguous set that clearly shouldn't be.

[T-Gro]

Can I mark as closed by #14319 now ?