Merge pull request #14333 from dotnet/backport/pr-14319-to-release/dev17.4

[release/dev17.4] Prefer nullable over other conversions

Author: vzarytovskii

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

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

src/Compiler/Checking/MethodCalls.fs

@@ -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
@@ -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

src/Compiler/Checking/MethodCalls.fsi

@@ -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

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'."""
 
-    [<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
             """
@@ -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"""