FS3388 (implicit conversion warning) rarely gets triggered when it should, using `--warnon: 3388`

[abelbraaksma]

Part of F# 6.0 was more leniency towards implicit conversions.

While there have been arguments both in favor and against it, the workaround for people wanting the "old behavior", warning FS3388, along with a few others, was introduced. Explicitly, it states:

FS3388: Type-directed conversion by subtyping (e.g. string --> obj). This warning is OFF by default.

However, even after adding warnon: 3388 to the compiler options, this warning is rarely raised, among which some of the most critical scenarios.

Repro steps

Example 1: boxing should always trigger this warning

The following code does not trigger the warning, whether on or off:

let a: obj list = [1; 2]      // boxing, should always warn, or raise, at the very least trigger FS3388
let b: obj list = ["a"; "b"]  // cast, should trigger FS3388

Example 2: previously failing-to-compile code should now warn

This code comes from this StackOverflow question, which, at the time, did not compile, because an upcast was necessary.

Code that didn't compile previously, should, imo, always raise this warning currently.

type A() =
    member x.A = "A"

type B() =
    inherit A()
    member x.B = "B"

let f (g: _ -> A) = g ()

let a = f (fun () -> A()) // works
let b = f (fun () -> B()) // should raise FS3388, but doesn't. Used to fail on older compilers

Example 3: most notorious scenarios with Task

It has happened to @natalie-o-perret recently, and last week to myself as well: accidentally returning a nested task, where the implementation expects a Task, not a Task<'T>, and a nested task then never gets executed. This can lead to very subtle bugs, and F# can, and I believe, should help with this.

This now gets auto-upcast, and does not trigger FS3388 when it's enabled:

type IFoo =
    abstract member Execute: unit -> Task

type Foo() =
    interface IFoo with
        member x.Execute() =
            let doSomething () = task { return () }
            task { return doSomething () }  // should trigger FS3388, as Task<Task<unit>> gets upcast to Task

Expected behavior

We introduced the new warning, so I'd expect the warning to be triggered. I should note that I tested this with different warning levels, and all with --warnon: 3388.

Actual behavior

In many cases the warning is not raised.

Known workarounds

None. Unless you code differently. For instance, take the last example. If you code it with an extra assignment, the warning does trigger. However, the whole point of the warning is to warn against such inadvertent uses.

This triggers the warning correctly:

type IFoo =
    abstract member Execute: unit -> Task

type Foo() =
    interface IFoo with
        member x.Execute() =
            let doSomething () = task { return () }
            let x = task { return doSomething () }
            x // due to the extra assignment, warning FS3388 is triggered

This also triggers the warning correctly:

let b: obj = "test"  // correctly triggers FS3388

Related information

Tested both on F# 6.0 and latest F# 7.0 Preview (thanks @gusty!).

[dsyme]

@abelbraaksma Strictly speaking, that warning only applies when the new-type directed ways of getting subtyping conversions. That's because these have more potential to be troublesome under code refactorings than the ones you mention above.

Because of this, this is by design - or a feature request for additional linting by the compiler.

[abelbraaksma]

@dsyme, you say “the new type directed conversions”. But that’s my point here. That doesn’t happen.

What did not compile before, now compiles without warning. So I’d still say it’s a bug, and quite a dangerous one at that, not a request for a new feature, as these cases where clearly listed in the RFC to trigger the warning.

That's because these have more potential to be troublesome

Exactly. But they don’t happen, no warning trigger in most cases.

[dsyme]

@abelbraaksma OK, so you mean examples 2 and 3, sorry, I didn't read past example 1, my bad . Can you remove example 1 from the report? (though note I can also see the case for a warning for all such upcasts).

It's curious that examples 2 and 3 don't trigger. The emit of the warning was fairly straight-forward. I wonder what's up.

Also example 3 is a spectacularly good example of how bad the Task<_> --> Task subtyping is, and how wrong it is that Task is widely used as a substitute for Task<void> and Task<unit>.

[abelbraaksma]

Also example 3 is a spectacularly good example of how bad the Task<_> --> Task subtyping is

Indeed it is, @gusty will be delighted to hear that! (We’ve both been hit by this and chased our tails, when XUnit runners just crashed because of out of band running orphaned tasks).

Glad you read the rest now too. This is currently one of my most worrying bugs, as its effects are so unpredictable. I’ll update the text.

Btw, @dsyme you consider my first example still “not a bug”, but why then:

let b: obj = "test"  // correctly triggers FS3388

But not:

let b: obj list = [ "test" ]  // does not  trigger warning FS3388

Is that a covariance relationship treated differently?

[charlesroddie]

Can we add false positives to this issue?

type X() =
    static member op_Implicit(_: bool) = X()
    static member AcceptX(_: X) = ()

module TestX =
    X.AcceptX(true) // FS3388 This expression implicitly converts type 'bool' to type 'X'

This should trigger FS3395: Type-directed conversion by an op_Implicit conversion at method-argument position. not FS3388: Type-directed conversion by subtyping (e.g. string --> obj) according to https://github.com/fsharp/fslang-design/blob/main/FSharp-6.0/FS-1093-additional-conversions.md .