G-Research / fsharp-analyzers

Analyzers for F#
https://g-research.github.io/fsharp-analyzers/
Apache License 2.0
14 stars 1 forks source link

Disposable will be disposed before async is run #54

Open nojaf opened 5 months ago

nojaf commented 5 months ago

Original request by @Smaug123:

let foo () =
    use blah = thing
    async {
        return "hi"
    }

We should warn that use blah will be disposed of before the async is run. This should happen when a function returns an Async/Task.

An additional request is that this check can be disabled by a code comment. Something like:

let foo () =
    // Note: disposed before returned async is run
    use blah = thing
    async {
        return "hi"
    }
dawedawe commented 5 months ago

Hey @Smaug123 ,

to be sure I get the requirements right. A use in a local func should not trigger a warning, right? Only top level use expressions in the async/task returning function. Right?

let foo () =
    let localFunc () =
        use t = new DisposableThing() // warning here?
        ()
    async {
        return "hi"
    }
Smaug123 commented 5 months ago

That's correct, because I think most readers would correctly expect the use to be disposed when leaving the scope of the local function. The analyser is specifically to catch the unintended interaction between implicit-disposal (from use) and deferred execution (from Async/Task); I think there's much less likely to be a bug in code of the creation-and-disposal-within-local-function pattern you quoted there.


In principle we could catch a related bug, I guess, where we return anything that closes over the disposed object:

let foo () =
    use blah = new DisposableThing ()
    fun () -> blah

This isn't quite the same as the async bug (though it is very related), but I think it's probably much less common and it's also got many more edge cases to consider, so we should probably just ignore that one.

dawedawe commented 5 months ago

Alright, thanks!