Fix #12994 and parts of #13437

[NinoFloris]

This PR is split into a few parts:

1. Tests, some fail because of #12994 while another test fails expecting a ctor call while getting op_Implicit due to the code falling back to FS-1093.
2. Fix for #12994, copying what @dsyme did in #12423.
3. 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.
4. 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 #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 https://github.com/dotnet/fsharp/issues/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 #12994.

@dsyme related, looking at line https://github.com/dotnet/fsharp/blob/9f65a3b6d3c6f6171e297fe55dbec1c42b235009/src/Compiler/Checking/MethodCalls.fs#L1306 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]

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]

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]

@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

[NinoFloris]

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 https://github.com/dotnet/fsharp/issues/12946#issuecomment-1217121581.

[dsyme]

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.

[dsyme]

This is looking good - a couple of minor nits

[NinoFloris]

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> https://github.com/dotnet/fsharp/pull/13673/commits/88ae3f8fafe2840db1d9125af4ababe199901cd8

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 https://github.com/dotnet/fsharp/pull/13673#discussion_r949358350 is tackled (captured in https://github.com/dotnet/fsharp/issues/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]

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]

@NinoFloris thanks a lot for this!

[dsyme]

@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