Prefer nullable over other conversions #14319
Conversation
| if c <> 0 then c else | ||
|
|
||
| // Prefer methods that only have nullable type-directed conversions | ||
| let c = compare (match usesTDC1 with TypeDirectedConversionUsed.Yes(_, _, true) -> 1 | _ -> 0) (match usesTDC2 with TypeDirectedConversionUsed.Yes(_, _, true) -> 1 | _ -> 0) |
There was a problem hiding this comment.
Looking at the ordering of these preference checks, and their global action across the overload, I'm concerned that they're too strong - preferring "methods that need less type-directed conversion" and "methods that only have nullable type-directed conversions" over all else in all situations, regardless of what else is happening (assuming there is some TDC in play in both overloads at all). This would affect method overloads where there's some TDC, and some other things in play too.
For the original addition of the "no TDC" check that's reasonable, as we didn't want that feature to affect any existing overloading. But I'm wondering whether these two further checks should go at the end of the preference list, or should be done argument by argument in compareArg
There was a problem hiding this comment.
That would be option 4 in #14302, because I agree, this is absolutely a kludge. It isn't really low risk to push such a refactor into a patch though.
There was a problem hiding this comment.
@NinoFloris : I would be for merging this PR as is now, and doing a bigger change later (after next VS update/release)
There was a problem hiding this comment.
The only downside is that code created at preview version might behave differently now.
But in general I agree with considering this a bugfix and not preferring nullable overloads was a bug ( = decreases strictness of backwards compatibility view)
There was a problem hiding this comment.
To balance the situation a bit @dsyme, we are not making more overload sets ambiguous than we were doing in F# 6 (correct me if I'm wrong). Overload sets where per argument checks will start to resolve to some other overload seem uncommon as they require a high amount of caller participation (passing arguments in the right nullable/numeric mix, all needing various conversions).
Subsequently we should be able to move from whole signature to per argument later in 7.0 or 8.0 without changing any but the most theoretic overload resolutions, strictly expanding the set of overloads that will resolve without error.
Additionally, if a user encounters an overload set with a high average arity - and where the caller arguments are (almost) all subject to conversions - they always have the option to do as many conversions by hand as necessary to disambiguate. For any other overload ambiguity issues a user might be able to use a parameter name difference to disambiguate (if any exists) but beyond that has no other options.
BC and risk wise this is a big difference, this PR can unblock the affected users and at the same time we don't realistically dig ourselves deeper into a BC hole.
…t defined on all test TFMs
NinoFloris
force-pushed
the
fix/resolution-regression-14302
branch
from
dd3750f to
d069694
Compare
|
I will go ahead and pre-created a backport, so we can merge it faster if approved by Don |
|
/backport to release/dev17.4 |
|
Started backporting to release/dev17.4: https://github.com/dotnet/fsharp/actions/runs/3479980049 |
Prefer nullable over other conversions, fixes #14302