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.

Sign up for GitHub

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

Prefer nullable over other conversions #14319

Merged
merged 2 commits into from
Nov 17, 2022
Merged

Prefer nullable over other conversions #14319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95d6069
Prefer nullable over other conversions, fixes #14302
NinoFloris Nov 14, 2022
d069694
Replace ROSpan for DateTimeOffset as op_Implicit target, ROSpan is no…
NinoFloris Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Compiler/Checking/CheckExpressions.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ let UnifyOverallType (cenv: cenv) (env: TcEnv) m overallTy actualTy =
| None -> ()

match usesTDC with
| TypeDirectedConversionUsed.Yes(warn, _) -> warning(warn env.DisplayEnv)
| TypeDirectedConversionUsed.Yes(warn, _, _) -> warning(warn env.DisplayEnv)
| TypeDirectedConversionUsed.No -> ()

if AddCxTypeMustSubsumeTypeUndoIfFailed env.DisplayEnv cenv.css m reqdTy2 actualTy then
Expand Down
14 changes: 9 additions & 5 deletions src/Compiler/Checking/ConstraintSolver.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2738,7 +2738,7 @@ and ArgsMustSubsumeOrConvert
msg csenv.DisplayEnv
| None -> ()
match usesTDC with
| TypeDirectedConversionUsed.Yes(warn, _) -> do! WarnD(warn csenv.DisplayEnv)
| TypeDirectedConversionUsed.Yes(warn, _, _) -> do! WarnD(warn csenv.DisplayEnv)
| TypeDirectedConversionUsed.No -> ()
do! SolveTypeSubsumesTypeWithReport csenv ndeep m trace cxsln (Some calledArg.CalledArgumentType) calledArgTy callerArg.CallerArgumentType
if calledArg.IsParamArray && isArray1DTy g calledArgTy && not (isArray1DTy g callerArg.CallerArgumentType) then
Expand Down Expand Up @@ -2769,7 +2769,7 @@ and ArgsMustSubsumeOrConvertWithContextualReport
msg csenv.DisplayEnv
| None -> ()
match usesTDC with
| TypeDirectedConversionUsed.Yes(warn, _) -> do! WarnD(warn csenv.DisplayEnv)
| TypeDirectedConversionUsed.Yes(warn, _, _) -> do! WarnD(warn csenv.DisplayEnv)
| TypeDirectedConversionUsed.No -> ()
do! SolveTypeSubsumesTypeWithWrappedContextualReport csenv ndeep m trace cxsln (Some calledArg.CalledArgumentType) calledArgTy callerArgTy (fun e -> ArgDoesNotMatchError(e :?> _, calledMeth, calledArg, callerArg))
return usesTDC
Expand All @@ -2796,7 +2796,7 @@ and ReturnTypesMustSubsumeOrConvert (csenv: ConstraintSolverEnv) ad ndeep trace
msg csenv.DisplayEnv
| None -> ()
match usesTDC with
| TypeDirectedConversionUsed.Yes(warn, _) -> do! WarnD(warn csenv.DisplayEnv)
| TypeDirectedConversionUsed.Yes(warn, _, _) -> do! WarnD(warn csenv.DisplayEnv)
| TypeDirectedConversionUsed.No -> ()
do! SolveTypeSubsumesTypeWithReport csenv ndeep m trace cxsln None reqdTy actualTy
return usesTDC
Expand All @@ -2813,7 +2813,7 @@ and ArgsEquivOrConvert (csenv: ConstraintSolverEnv) ad ndeep trace cxsln isConst
msg csenv.DisplayEnv
| None -> ()
match usesTDC with
| TypeDirectedConversionUsed.Yes(warn, _) -> do! WarnD(warn csenv.DisplayEnv)
| TypeDirectedConversionUsed.Yes(warn, _, _) -> do! WarnD(warn csenv.DisplayEnv)
| TypeDirectedConversionUsed.No -> ()
if not (typeEquiv csenv.g calledArgTy callerArgTy) then
return! ErrorD(Error(FSComp.SR.csArgumentTypesDoNotMatch(), m))
Expand Down Expand Up @@ -3225,7 +3225,11 @@ and GetMostApplicableOverload csenv ndeep candidates applicableMeths calledMethG
if c <> 0 then c else

// Prefer methods that need less type-directed conversion
let c = compare (match usesTDC1 with TypeDirectedConversionUsed.Yes(_, false) -> 1 | _ -> 0) (match usesTDC2 with TypeDirectedConversionUsed.Yes(_, false) -> 1 | _ -> 0)
let c = compare (match usesTDC1 with TypeDirectedConversionUsed.Yes(_, false, _) -> 1 | _ -> 0) (match usesTDC2 with TypeDirectedConversionUsed.Yes(_, false, _) -> 1 | _ -> 0)
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NinoFloris : I would be for merging this PR as is now, and doing a bigger change later (after next VS update/release)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if c <> 0 then c else

// Prefer methods that don't give "this code is less generic" warnings
Expand Down
22 changes: 13 additions & 9 deletions src/Compiler/Checking/MethodCalls.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,16 @@ type TypeDirectedConversion =

[<RequireQualifiedAccess>]
type TypeDirectedConversionUsed =
| Yes of (DisplayEnv -> exn) * isTwoStepConversion: bool
| Yes of (DisplayEnv -> exn) * isTwoStepConversion: bool * isNullable: bool
| No
static member Combine a b =
match a, b with
| Yes(_,true), _ -> a
| _, Yes(_,true) -> b
// We want to know which candidates have one or more nullable conversions exclusively
// If one of the values is false we flow false for both.
| Yes(_, true, false), _ -> a
| _, Yes(_, true, false) -> b
| Yes(_, true, _), _ -> a
| _, Yes(_, true, _) -> b
| Yes _, _ -> a
| _, Yes _ -> b
| No, No -> a
Expand Down Expand Up @@ -282,33 +286,33 @@ let rec AdjustRequiredTypeForTypeDirectedConversions (infoReader: InfoReader) ad

// Adhoc int32 --> int64
elif g.langVersion.SupportsFeature LanguageFeature.AdditionalTypeDirectedConversions && typeEquiv g g.int64_ty reqdTy && typeEquiv g g.int32_ty actualTy then
g.int32_ty, TypeDirectedConversionUsed.Yes(warn TypeDirectedConversion.BuiltIn, false), None
g.int32_ty, TypeDirectedConversionUsed.Yes(warn TypeDirectedConversion.BuiltIn, false, false), None

// Adhoc int32 --> nativeint
elif g.langVersion.SupportsFeature LanguageFeature.AdditionalTypeDirectedConversions && typeEquiv g g.nativeint_ty reqdTy && typeEquiv g g.int32_ty actualTy then
g.int32_ty, TypeDirectedConversionUsed.Yes(warn TypeDirectedConversion.BuiltIn, false), None
g.int32_ty, TypeDirectedConversionUsed.Yes(warn TypeDirectedConversion.BuiltIn, false, false), None

// Adhoc int32 --> float64
elif g.langVersion.SupportsFeature LanguageFeature.AdditionalTypeDirectedConversions && typeEquiv g g.float_ty reqdTy && typeEquiv g g.int32_ty actualTy then
g.int32_ty, TypeDirectedConversionUsed.Yes(warn TypeDirectedConversion.BuiltIn, false), None
g.int32_ty, TypeDirectedConversionUsed.Yes(warn TypeDirectedConversion.BuiltIn, false, false), None

elif g.langVersion.SupportsFeature LanguageFeature.NullableOptionalInterop && isMethodArg && isNullableTy g reqdTy && not (isNullableTy g actualTy) then
let underlyingTy = destNullableTy g reqdTy
// shortcut
if typeEquiv g underlyingTy actualTy then
actualTy, TypeDirectedConversionUsed.Yes(warn TypeDirectedConversion.BuiltIn, false), None
actualTy, TypeDirectedConversionUsed.Yes(warn TypeDirectedConversion.BuiltIn, false, true), None
else
let adjustedTy, _, _ = AdjustRequiredTypeForTypeDirectedConversions infoReader ad isMethodArg isConstraint underlyingTy actualTy m
if typeEquiv g adjustedTy actualTy then
actualTy, TypeDirectedConversionUsed.Yes(warn TypeDirectedConversion.BuiltIn, true), None
actualTy, TypeDirectedConversionUsed.Yes(warn TypeDirectedConversion.BuiltIn, true, true), None
else
reqdTy, TypeDirectedConversionUsed.No, None

// Adhoc based on op_Implicit, perhaps returing a new equational type constraint to
// eliminate articifical constrained type variables.
elif g.langVersion.SupportsFeature LanguageFeature.AdditionalTypeDirectedConversions then
match TryFindRelevantImplicitConversion infoReader ad reqdTy actualTy m with
| Some (minfo, _staticTy, eqn) -> actualTy, TypeDirectedConversionUsed.Yes(warn (TypeDirectedConversion.Implicit minfo), false), Some eqn
| Some (minfo, _staticTy, eqn) -> actualTy, TypeDirectedConversionUsed.Yes(warn (TypeDirectedConversion.Implicit minfo), false, false), Some eqn
| None -> reqdTy, TypeDirectedConversionUsed.No, None

else reqdTy, TypeDirectedConversionUsed.No, None
Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/Checking/MethodCalls.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ type CallerArgs<'T> =
/// has been used in F# code
[<RequireQualifiedAccess>]
type TypeDirectedConversionUsed =
| Yes of (DisplayEnv -> exn) * isTwoStepConversion: bool
| Yes of (DisplayEnv -> exn) * isTwoStepConversion: bool * isNullable: bool
| No

static member Combine: TypeDirectedConversionUsed -> TypeDirectedConversionUsed -> TypeDirectedConversionUsed
Expand Down
65 changes: 64 additions & 1 deletion tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ let test(x: 'T) =
(11, 5, 11, 11)
"""This construct causes code to be less generic than indicated by the type annotations. The type variable 'T has been constrained to be type 'int'."""

[<Test>]
[<Test(Description="https://github.com/dotnet/fsharp/issues/13731")>]
let ``Picking overload for typar fails when incompatible types are part of the candidate set``() =
CompilerAssert.TypeCheckWithErrors
"""
Expand Down Expand Up @@ -440,3 +440,66 @@ let test() =

if not (test().OtherArgs.Value.Name = "test") then failwith "Unexpected value was returned after setting Name"
""" []

[<Test>]
let ``Prefer nullable conversion only candidate when multiple candidates require conversions``() =
CompilerAssert.RunScript
"""
type M() =
static member A(size: System.DateTime, dtype: System.Nullable<int>) = 1
static member A(size: System.DateTimeOffset, dtype: System.Nullable<int>) = 2

let test() = M.A(System.DateTime.UtcNow, 1)

if test() <> 1 then failwith "Incorrect overload picked"
""" []

[<Test>]
let ``Prefer nullable conversion over numeric conversion``() =
CompilerAssert.RunScript
"""
type M() =
static member A(n: int64) = 1
static member A(n: System.Nullable<int>) = 2

let test() = M.A(0)

if test() <> 2 then failwith "Incorrect overload picked"
""" []

[<Test>]
let ``Prefer nullable conversion over op_Implicit conversion``() =

CompilerAssert.RunScript
"""
type M() =
static member A(n: System.DateTimeOffset) = 1
static member A(n: System.Nullable<System.DateTime>) = 2

let test() = M.A(System.DateTime.UtcNow)

if test() <> 2 then failwith "Incorrect overload picked"
""" []


[<Test(Description="https://github.com/dotnet/fsharp/issues/14318")>]
let ``Picking overload for TDC candidate set fails as ambiguous while one candidate requires more conversions``() =
CompilerAssert.TypeCheckSingleError
"""
type M() =
static member A(m: System.DateTime, n: int64) = 1
static member A(m: System.DateTimeOffset, n: int64) = 2

let test() = M.A(System.DateTime.UtcNow, 1)
"""
FSharpDiagnosticSeverity.Error
41
(6, 14, 6, 44)
"""A unique overload for method 'A' could not be determined based on type information prior to this program point. A type annotation may be needed.

Known types of arguments: System.DateTime * int

Candidates:
- static member M.A: m: System.DateTime * n: int64 -> int
- static member M.A: m: System.DateTimeOffset * n: int64 -> int
- static member M.A: m: System.DateTimeOffset * n: int64 -> int"""