From 95d60694bf91c198a6181dcc9de5ca7a7e35b9a9 Mon Sep 17 00:00:00 2001 From: Nino Floris Date: Mon, 14 Nov 2022 21:06:31 +0100 Subject: [PATCH 1/2] Prefer nullable over other conversions, fixes #14302 --- src/Compiler/Checking/CheckExpressions.fs | 2 +- src/Compiler/Checking/ConstraintSolver.fs | 14 ++-- src/Compiler/Checking/MethodCalls.fs | 22 ++++--- src/Compiler/Checking/MethodCalls.fsi | 2 +- .../Language/TypeDirectedConversionTests.fs | 65 ++++++++++++++++++- 5 files changed, 88 insertions(+), 17 deletions(-) diff --git a/src/Compiler/Checking/CheckExpressions.fs b/src/Compiler/Checking/CheckExpressions.fs index 043502372c2..c25bbfa92ab 100644 --- a/src/Compiler/Checking/CheckExpressions.fs +++ b/src/Compiler/Checking/CheckExpressions.fs @@ -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 diff --git a/src/Compiler/Checking/ConstraintSolver.fs b/src/Compiler/Checking/ConstraintSolver.fs index 751e71bff5c..6b0ef85d5e8 100644 --- a/src/Compiler/Checking/ConstraintSolver.fs +++ b/src/Compiler/Checking/ConstraintSolver.fs @@ -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 @@ -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 @@ -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 @@ -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)) @@ -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) if c <> 0 then c else // Prefer methods that don't give "this code is less generic" warnings diff --git a/src/Compiler/Checking/MethodCalls.fs b/src/Compiler/Checking/MethodCalls.fs index 1536277c506..c7523e7ba50 100644 --- a/src/Compiler/Checking/MethodCalls.fs +++ b/src/Compiler/Checking/MethodCalls.fs @@ -236,12 +236,16 @@ type TypeDirectedConversion = [] 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 @@ -282,25 +286,25 @@ 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 @@ -308,7 +312,7 @@ let rec AdjustRequiredTypeForTypeDirectedConversions (infoReader: InfoReader) ad // 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 diff --git a/src/Compiler/Checking/MethodCalls.fsi b/src/Compiler/Checking/MethodCalls.fsi index a70827d8fec..60a5ace7201 100644 --- a/src/Compiler/Checking/MethodCalls.fsi +++ b/src/Compiler/Checking/MethodCalls.fsi @@ -119,7 +119,7 @@ type CallerArgs<'T> = /// has been used in F# code [] type TypeDirectedConversionUsed = - | Yes of (DisplayEnv -> exn) * isTwoStepConversion: bool + | Yes of (DisplayEnv -> exn) * isTwoStepConversion: bool * isNullable: bool | No static member Combine: TypeDirectedConversionUsed -> TypeDirectedConversionUsed -> TypeDirectedConversionUsed diff --git a/tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs b/tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs index be31e72712e..9055740f2a1 100644 --- a/tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs +++ b/tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs @@ -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'.""" - [] + [] let ``Picking overload for typar fails when incompatible types are part of the candidate set``() = CompilerAssert.TypeCheckWithErrors """ @@ -440,3 +440,66 @@ let test() = if not (test().OtherArgs.Value.Name = "test") then failwith "Unexpected value was returned after setting Name" """ [] + + [] + let ``Prefer nullable conversion only candidate when multiple candidates require conversions``() = + CompilerAssert.RunScript + """ +type M() = + static member A(size: int64 array, dtype: System.Nullable) = 1 + static member A(size: System.ReadOnlySpan, dtype: System.Nullable) = 2 + +let test() = M.A([|10L|], 1) + +if test() <> 1 then failwith "Incorrect overload picked" + """ [] + + [] + let ``Prefer nullable conversion over numeric conversion``() = + CompilerAssert.RunScript + """ +type M() = + static member A(n: int64) = 1 + static member A(n: System.Nullable) = 2 + +let test() = M.A(0) + +if test() <> 2 then failwith "Incorrect overload picked" + """ [] + + [] + 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) = 2 + +let test() = M.A(System.DateTime.UtcNow) + +if test() <> 2 then failwith "Incorrect overload picked" + """ [] + + + [] + let ``Picking overload for TDC candidate set fails as ambiguous while one candidate requires more conversions``() = + CompilerAssert.TypeCheckSingleError + """ +type M() = + static member A(m: int64 array, n: int64) = 1 + static member A(m: System.ReadOnlySpan, n: int64) = 2 + +let test() = M.A([|10L|], 1) + """ + FSharpDiagnosticSeverity.Error + 41 + (6, 14, 6, 29) + """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: int64[] * int + +Candidates: + - static member M.A: m: System.ReadOnlySpan * n: int64 -> int + - static member M.A: m: System.ReadOnlySpan * n: int64 -> int + - static member M.A: m: int64 array * n: int64 -> int""" From d0696941e5c836237b8d01d1cc2a0b66c43d81b2 Mon Sep 17 00:00:00 2001 From: Nino Floris Date: Tue, 15 Nov 2022 14:48:53 +0100 Subject: [PATCH 2/2] Replace ROSpan for DateTimeOffset as op_Implicit target, ROSpan is not defined on all test TFMs --- .../Language/TypeDirectedConversionTests.fs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs b/tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs index 9055740f2a1..054664224c9 100644 --- a/tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs +++ b/tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs @@ -446,10 +446,10 @@ if not (test().OtherArgs.Value.Name = "test") then failwith "Unexpected value wa CompilerAssert.RunScript """ type M() = - static member A(size: int64 array, dtype: System.Nullable) = 1 - static member A(size: System.ReadOnlySpan, dtype: System.Nullable) = 2 + static member A(size: System.DateTime, dtype: System.Nullable) = 1 + static member A(size: System.DateTimeOffset, dtype: System.Nullable) = 2 -let test() = M.A([|10L|], 1) +let test() = M.A(System.DateTime.UtcNow, 1) if test() <> 1 then failwith "Incorrect overload picked" """ [] @@ -487,19 +487,19 @@ if test() <> 2 then failwith "Incorrect overload picked" CompilerAssert.TypeCheckSingleError """ type M() = - static member A(m: int64 array, n: int64) = 1 - static member A(m: System.ReadOnlySpan, n: int64) = 2 + static member A(m: System.DateTime, n: int64) = 1 + static member A(m: System.DateTimeOffset, n: int64) = 2 -let test() = M.A([|10L|], 1) +let test() = M.A(System.DateTime.UtcNow, 1) """ FSharpDiagnosticSeverity.Error 41 - (6, 14, 6, 29) + (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: int64[] * int +Known types of arguments: System.DateTime * int Candidates: - - static member M.A: m: System.ReadOnlySpan * n: int64 -> int - - static member M.A: m: System.ReadOnlySpan * n: int64 -> int - - static member M.A: m: int64 array * n: int64 -> int""" + - 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"""