FSharp.Control.TaskSeq icon indicating copy to clipboard operation
FSharp.Control.TaskSeq copied to clipboard

Unintended object inference

Open JohSand opened this issue 1 year ago • 3 comments

Binding task-likes in the taskSeq-builder will trigger the FS3559-warning. This seems to be due to the existence of the 'TOverall parameter here:

        [<NoEagerConstraintApplication>]
        member inline Bind< ^TaskLike, 'T, 'U, ^Awaiter, 'TOverall> :
            task: ^TaskLike * continuation: ('T -> ResumableTSC<'U>) -> ResumableTSC<'U>
                when ^TaskLike: (member GetAwaiter: unit -> ^Awaiter)
                and ^Awaiter :> ICriticalNotifyCompletion
                and ^Awaiter: (member get_IsCompleted: unit -> bool)
                and ^Awaiter: (member GetResult: unit -> 'T)

This parameter cannot be inferred, since it is not used in the method, and is probably only present due to copy-paste.

JohSand avatar Jun 07 '24 14:06 JohSand

Hi @JohSand, thanks for reporting this.

and is probably only present due to copy-paste.

~The 'TOverall cannot be removed here (at least, I remember it wasn't removable, but I may be wrong, this part was done 2 years ago and I don't recall why it's there). Do you happen to have a small repo for this that I can add as test?~

EDIT: I was wrong. Looks like it works just fine after removing it. @JohSand If you can test the linked PR to verify it removes the warning, please do (and if you have a repo, even better!).

To be fair, when designing this, I didn't expect task-like types to be used very often and I don't think I have proper coverage in the tests, which ought to have uncovered this issue.

I'm glad there are use-cases "in the wild" :) and I hope there's a straightforward fix :).

abelbraaksma avatar Jun 12 '24 15:06 abelbraaksma

Hi. For a repro, the easiest way is to bind Task.Yield(), which does in fact not return a Task, but a YieldAwatable.

So with <WarnOn>FS3559</WarnOn>, I can repro it with code like

    taskSeq {
        do! Task.Yield()
        yield 1
    }

I can also confirm that remove 'TOverall removes the warning.

JohSand avatar Jun 12 '24 17:06 JohSand

Awesome. I'll add a few tests tomorrow and merge the PR. Thank you very much for reporting this!

abelbraaksma avatar Jun 12 '24 22:06 abelbraaksma