Fix missing TailCall warnings in TryWith/TryFinally

[dawedawe]

Fixes #16322 by adding the missing checks for TOp.TryWith and TOp.TryFinally.
Add a good amount of test cases.

Thanks again to @jwosty for raising the issue. I really had a blind spot there while I worked on this.

[psfinaki]

Thanks!

[jwosty]

Nice; that was fast!

Original comment

These could some good test cases, too. They already work in the release but it might be good to have them too to protect against regressions. Nevermind, I see that you already did this. Disregard this comment

// should warn
[<TailCall>]
let rec retryAsyncNonTc nRetries (work: unit -> 'a) : Result<'a, Exception list> = 
    try
        Ok (work ())
    with e ->
        match retryAsyncNonTc (nRetries - 1) work with
        | Ok _ as result -> result
        | Error subExns ->
            raise (AggregateException(e::subExns))

// should not warn
[<TailCall>]
let rec retryAsync prevExns nRetries (work: unit -> 'a) : 'a = 
    try
        work ()
    with e ->
        let exns = e::prevExns
        if nRetries < 1 then
            raise (AggregateException(List.rev exns))
        else
            retryAsync exns (nRetries - 1) work

[jwosty]

I do actually wonder about Task though. Shouldn't a warning be emitted for those as well, since Task tailcalls aren't supported? Correct me if I'm wrong.

// should emit warning, I think?
[<TailCall>] 
let rec f x = task {
    let y = x - 1
    let z = y - 1
    return! f (z - 1)
}

[dawedawe]

I do actually wonder about Task though. Shouldn't a warning be emitted for those as well, since Task tailcalls aren't supported? Correct me if I'm wrong.

You're right, the function you showed crashes with a StackOverflow.
We do already warn about it in the v8 release. I added the test to catch regressions.

[jwosty]

Great work!

While we're still here, a test for use bindings might be good too, for more regression protection:

[<TailCall>]
let rec f () =
    use x = makeDisposable()
    f ()

[dawedawe]

While we're still here, a test for use bindings might be good too, for more regression protection:

Oh yeah, good idead. Pushed it.

[Martin521]

@dawedawe
Not related to this PR, but I think the namespace should be "ErrorMessages", not "FSharp.Compiler.ComponentTests.ErrorMessages", else the tests appear in test explorers in a weird place.

[dawedawe]

@dawedawe Not related to this PR, but I think the namespace should be "ErrorMessages", not "FSharp.Compiler.ComponentTests.ErrorMessages", else the tests appear in test explorers in a weird place.

Oh good catch, thanks! I changed that while here.