Fix issues around type directed conversion #13673

NinoFloris · 2022-08-11T12:56:10Z

This PR is split into a few parts:

Tests, some fail because of Error for extra implicit conversion is needed #12994 while another test fails expecting a ctor call while getting op_Implicit due to the code falling back to FS-1093.
Fix for Error for extra implicit conversion is needed #12994, copying what @dsyme did in Fixes 12414: int32 --> Nullable Int64 additional implicit conversion #12423.
Improved diagnostics by keeping the known parameter type around instead of reporting the underlying type. This results in a message going from The type 'bool' is not compatible with type 'float' to The type 'bool' is not compatible with type 'Nullable<float>' which is what users would expect to see.
A bit more contentious, it integrates the fix from 2 into the type directed conversions code of FS-1093, completing a nullable TODO comment and removing the old superseded code.

Why integrate with the code for FS-1093? De-facto it already is; op_Implicit would trigger if FS-1075 would not exist in most cases (except the double conversion ones).

This is most clear when looking at Error for extra implicit conversion is needed #12994 which was missed in the implementation of FS-1075; Passing a setter argument float to Nullable<float> results in op_Implicit without a warning being emitted today.
Additionally having FS-1075 overlap FS-1093 means it obscures the diagnostics introduced in the latter due to it not being seen as a type directed conversion (which means it does not produce consistent warnings for System.Nullable, see FS-1093 op_Implicit 3395 warnings missing for constructors and potentially inconsistent #13437). It now produces a builtin diag when it's a method arg (something to discuss) and an implicit conversion diag for the others.

@dsyme Maybe this needs a new RFC or change to FS-1093? Adding the diag seemed so small to me that it would fit under tidying up things.

In neither implementation nothing beyond the diag has been widened past what was specified in FS-1093 and FS-1075, notably:

Double conversions for System.Nullable are still restricted to method arguments only.
Type directed conversions for System.Nullable outside method args still fall through to op_Implicit, meaning that we still error on let x: Nullable<float> = 1: int as expected.
Setter arguments produce a constructor call instead of an op_Implicit as an expected change to fix Error for extra implicit conversion is needed #12994.

@dsyme related, looking at line

fsharp/src/Compiler/Checking/MethodCalls.fs

Line 1306 in 9f65a3b

| None -> mkCoerceIfNeeded g reqdTy actualTy expr

if the compiler runs into a conversion that it cannot properly do it just spews box unbox.any due to the coerce. I assume this only happened because the type checks were more lenient than the expr adjustments?
Also, none of the nullable interop conversions in AdjustCallerArgForOptional were guarded by the language feature flag, it is now guarded in AdjustExprForTypeDirectedConversions. Maybe it was ok because it was already checked under AdjustCalledArgTypeForOptionals?

NinoFloris · 2022-08-11T13:05:50Z

Maybe this should be combined with similar work in #13063? I had similar thoughts that it may need a richer model to capture the different tdc forms, though ideally it should just be one big thing IMO.

NinoFloris · 2022-08-11T15:10:16Z

Fun test failure, the dumped IL is different on my machine by one dot, probably some ildasm culture output difference.

CI error:

 FSharp.Compiler.UnitTests.TypeDirectedConversionTests.float converts to System.Nullable<float> in method call parameter

==
Name: '.method public static void  test() cil managed'

Expected:	 IL_0000:  ldc.r8     100
Actual:		 IL_0000:  ldc.r8     100.
==

Changing it locally to include the . produces the inverse error. Moved these tests to use an int.

dsyme · 2022-08-11T16:48:04Z

@vzarytovskii @NinoFloris I'll need to review this very carefully. I'll be back at work properly next week and suggest we do it then

src/Compiler/Checking/MethodCalls.fs

NinoFloris · 2022-08-16T20:36:00Z

I included the fix for #12946 as well (could easily be split out if preferred). I've written about the source of the current broken behavior here #12946 (comment).

tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs

dsyme · 2022-08-17T22:52:48Z

if the compiler runs into a conversion that it cannot properly do it just spews box unbox.any due to the coerce. I assume this only happened because the type checks were more lenient than the expr adjustments

I think this case should match coercion type-drected conversions - which are originate in CheckExpressions.fs via MustCoerce and so on.

Also, none of the nullable interop conversions in AdjustCallerArgForOptional were guarded by the language feature flag, it is now guarded in AdjustExprForTypeDirectedConversions. Maybe it was ok because it was already checked under AdjustCalledArgTypeForOptionals?

It might be (I need to double check) that these specific coercions were present in F# 5.0 for "nullable interop" i.e. https://github.com/fsharp/fslang-design/blob/main/FSharp-5.0/FS-1075-nullable-interop.md. Or else yes, the checking in AdjustCalledArgTypeForOptionals should be sufficient - there is no specific need to guard again.

src/Compiler/Checking/MethodCalls.fs

tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs

Fixes test failures for cases where separate overloads have args like Nullable<int> and Nullable<int64> in the same position

dsyme · 2022-08-19T13:01:19Z

tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs

+        """ []
+
+    [<Test>]
+    let ``Picking overload for typar does not favor any form of System.Nullable nor produce ambiguity warnings``() =


Is the name of this test still correct?

Also could you add the cases

With the Nullable<float> overload

With the Nullable<float> overload and without Nullable<'T> overload

As far as it's compatible with the current definition of 'producing ambiguity warnings' I'd say yes. I also opened an issue that tracks the rest.

Added a failing test for #13731

dsyme · 2022-08-19T13:03:18Z

tests/fsharp/Compiler/Language/OptionalInteropTests.fs

-    [<Test>]
-    let ``C# method with an optional parameter and called with an option type should compile`` () =
+    [<TestCase("5.0")>]
+    [<TestCase("latest")>]


Should preview be added here?

I thought about it, that'd be fine. The commit that introduces this change specifically mentions it's to test the compiler pre and post FS-1093 additional conversions but preview would be compatible with that wording.

Also the baseline test that this test was copied from is running on preview and 4.7 (that's fsfromfsviacs)

Changed to preview here too

dsyme · 2022-08-19T13:05:11Z

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)
+    | Yes of (DisplayEnv -> exn) * isDoubleConversion: bool


The name DoubleConversion is problematic - does it mean "int to double" or "there are two conversions".

Also could this better be an integer indicating the number of conversion rules applied?

Fair point, something like isTwoStepConversion would be a worse sounding but unambiguous name.

Regardless of naming this is a kludge to fix the immediate issue. I would suggest that the PR that fixes #13731 and fully resolves #13437 would introduce richer types like you did in #13063. that would be where we could capture potentially as a linked list (or just an integer indeed) all the conversions done.

Yes isTwoStepConversion please

dsyme · 2022-08-19T13:10:08Z

src/Compiler/Checking/CheckExpressions.fs

@@ -5385,7 +5385,7 @@ and TcAdjustExprForTypeDirectedConversions (cenv: cenv) (overallTy: OverallTy) a
    let g = cenv.g

    match overallTy with
-    | MustConvertTo (_, reqdTy) when g.langVersion.SupportsFeature LanguageFeature.AdditionalTypeDirectedConversions ->
+    | MustConvertTo (isMethodArg, reqdTy) when g.langVersion.SupportsFeature LanguageFeature.AdditionalTypeDirectedConversions || (g.langVersion.SupportsFeature LanguageFeature.NullableOptionalInterop && isMethodArg) ->


Thinking aloud: The conditional here is now quite complex - maybe it would have been better originally to just always call AdjustExprForTypeDirectedConversions and have the conditions on the specific rules in that method.

No need to change anything here though.

dsyme · 2022-08-19T13:10:50Z

This is looking good - a couple of minor nits

NinoFloris · 2022-08-22T12:08:13Z

Ok so as mentioned offline, the only thing that's left to decide on is whether we should actually begin to emit the 'builtin conversion' warning under test here for Nullable<'T> 88ae3f8

The strongest argument for this change is that it brings those conversions in line with the other supported conversions.

The downside for this implementation in particular is that it will only emit a warning when full type info is available, basically until this problem #13673 (comment) is tackled (captured in #13731) we'll have cases like 'T being converted to Nullable<'T> where the compiler won't warn yet. I think that's acceptable but maybe we want to do it all in one go.

@dsyme please let me know, I'd like to get this off my list :)

dsyme · 2022-08-22T12:38:59Z

The downside for this implementation in particular is ...

I think we won't do this as part of this PR, partly because emitting the warning (even optional) would take it from being a fix to a possible RFC.

So I'll review now, and accept as is. Thank you sooooo much for your detailed work here.

vzarytovskii · 2022-08-22T13:27:53Z

@NinoFloris thanks a lot for this!

dsyme · 2022-08-22T13:32:43Z

@NinoFloris pointed out privately that this does emit the FS3389 off-by-default warning in an additional case, seen here https://github.com/dotnet/fsharp/pull/13673/files#diff-0020979b647768e28019703c22e81771d97b9e9b26eb7c3cb518c692e6596beaR264
2:25

This is ok and I consider this a bug fix as it's definitely a type-directed conversion outside the scope of optional-arg interop (the argument is not labelled optional in metadata, it just has explicit type Nullable).

Additionally, the presence of the warning doesn't affect overload resolution because this rule would already take this into account in overload resolution

In short, all OK, I'm just noting this in case it comes up later

Add notes summarizing my knowledge of the current state of this RFC after writing dotnet/fsharp#13673

NinoFloris commented Aug 11, 2022

View reviewed changes

src/Compiler/Checking/MethodCalls.fs Outdated Show resolved Hide resolved

NinoFloris commented Aug 11, 2022

View reviewed changes

src/Compiler/Checking/MethodCalls.fs Outdated Show resolved Hide resolved

NinoFloris force-pushed the fix-nullable-tdc-setter-args branch from 5f8d5fc to b06451f Compare August 11, 2022 17:37

NinoFloris closed this Aug 16, 2022

NinoFloris reopened this Aug 16, 2022

NinoFloris mentioned this pull request Aug 16, 2022

Default use of implicit conversion creates non-working code (using Pulumi libraries) #12946

Closed

NinoFloris changed the title ~~Fix #12994 and parts of #13437~~ Fix issues around type directed conversion Aug 16, 2022

NinoFloris force-pushed the fix-nullable-tdc-setter-args branch from 47cbab8 to 69d109a Compare August 17, 2022 13:44

vzarytovskii reviewed Aug 17, 2022

View reviewed changes

tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs Show resolved Hide resolved

NinoFloris force-pushed the fix-nullable-tdc-setter-args branch 2 times, most recently from a81a9ce to ffa2e0d Compare August 17, 2022 20:05

NinoFloris commented Aug 17, 2022

View reviewed changes

tests/fsharp/Compiler/Language/TypeDirectedConversionTests.fs Show resolved Hide resolved