Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent error and warning messaging in VS while using [<TailCall>] plus unexpected error while using [<TailCall>] #16330

Closed
MiroslavHustak opened this issue Nov 22, 2023 · 11 comments · Fixed by #16347
Labels
Milestone

Comments

@MiroslavHustak
Copy link

MiroslavHustak commented Nov 22, 2023

  1. Inconsistent showing of warning messages in VS during my testing of [<TailCall>] in my app:
[<TailCall>]
let rec internal bind f program =
    match program with
    | Free x -> Free (mapI (bind f) x)
    | Pure x -> f x

[<TailCall>]
let rec internal nXor operands =
    match operands with
    | []    -> false  
    | x::xs -> (x && not (nXor xs)) || ((not x) && (nXor xs))

After a build, the aforementioned code sometimes triggers warning messages (see below) in the Error List, other times not.
After a build, the aforementioned code sometimes triggers warning messages (see below) in the Output, other times not.

warning FS3569: The member or function 'bind' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way.
warning FS3569: The member or function 'nXor' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way.

To reproduce, you may use my entire VS solution (the functions in question are in the files Helpers.fs and FreeMonads.fs). 54 MB.

[<TailCall>] in the following code invokes an error message. I do not see any reason why. UPDATE: A warning is expected instead.

[<TailCall>]
let rec private interpret config io = 

    let source = config.source
    let destination = config.destination

    let msg = sprintf "Chyba %s při čtení cesty " 
    
    let result path1 path2 = 
        match path1 with
        | Ok path1  -> 
                    path1
        | Error err -> 
                    printf "%s%s" err path2 
                    Console.ReadKey() |> ignore 
                    System.Environment.Exit(1) 
                    String.Empty

    let f = 
        match io with
        | Copy -> fun p1 p2 -> File.Copy(p1, p2, true) //(fun _ _ -> ())           
        | Move -> fun p1 p2 -> File.Move(p1, p2, true) //(fun _ _ -> ())
  
    function
    | Pure x -> x
    | Free (SourceFilepath next) ->
                                  let sourceFilepath source =                                        
                                      pyramidOfDoom
                                         {
                                             let! value = Path.GetFullPath(source) |> Option.ofNull, Error <| msg "č.2"   
                                             let! value = 
                                                 (
                                                     let fInfodat: FileInfo = new FileInfo(value)   
                                                     Option.fromBool value fInfodat.Exists
                                                 ), Error <| msg "č.1"
                                             return Ok value
                                         }      
                                  next (result (sourceFilepath source) source) |> interpret config io
    | Free (DestinFilepath next) ->
                                  let destinFilepath destination =                                        
                                      pyramidOfDoom
                                         {
                                             let! value = Path.GetFullPath(destination) |> Option.ofNull, Error <| msg "č.4"   
                                             let! value = 
                                                 (
                                                     let dInfodat: DirectoryInfo = new DirectoryInfo(value)   
                                                     Option.fromBool value dInfodat.Exists
                                                 ), Error <| msg "č.3"
                                             return Ok value
                                         }                                        
                                  next (result (destinFilepath destination) destination) |> interpret config io
    | Free (CopyOrMove (s, _))   -> 
                                  let sourceFilepath = fst s
                                  let destinFilepath = snd s  
                                  f sourceFilepath destinFilepath 
                                  //next |> interpret config `

Again, the messaging is inconsistent. After a build, the aforementioned code sometimes triggers an error message (see below) both in the Output and Error List, other times only in the Output.

error FS0251: Invalid member signature encountered because of an earlier error

To reproduce, you may use my entire VS solution (the function in question is in the file Helpers.fs). 54 MB.

Windows 10 Pro, version 22H2
.NET 8
VS, version 17.8.0

@vzarytovskii
Copy link
Member

The first case can be because compiler is newer than tooling in VS (vs lags behind in other words).

So, when full build happens, diagnostics from compiler are shown, and then when typecheck happens it rewrites output.

But it's only theory.

Diagnostics in VS also may differ due to error recovery difference.

Not sure what's happening in second case though

@brianrourkeboll
Copy link
Contributor

brianrourkeboll commented Nov 22, 2023

The second part, where you get a warning for … |> interpret …, seems potentially related to #6984.

@MiroslavHustak
Copy link
Author

MiroslavHustak commented Nov 22, 2023

The second part, where you get a warning for … |> interpret …, seems potentially related to #6984.

Yes, I know about that, but why an error is raised, not just a warning? I apologise for not suggesting the expected result in the text of the raised issue.
And yes, it compiles (with a warning) when refactored in this way interpret config io (next (result (sourceFilepath source) source))

@dawedawe
Copy link
Contributor

dawedawe commented Nov 23, 2023

Hey @MiroslavHustak ,
first of all, thanks for giving the new feature a spin!

[<TailCall>]
let rec internal bind f program =
    match program with
    | Free x -> Free (mapI (bind f) x)
    | Pure x -> f x

[<TailCall>]
let rec internal nXor operands =
    match operands with
    | []    -> false  
    | x::xs -> (x && not (nXor xs)) || ((not x) && (nXor xs))

These are not tail-recursive definitions, so it's okay to get a warning for them.

Regarding the warning for your interpret function:
I think the issue is the usage of the match lambda (aka function). By that, you are giving back a function.
Refactor your interpret into

let rec interpret config io xxx = 
...

and later do a

match xxx with
| Pure x -> x
...

That should help turning it into a tailrec definition.
And yes, keep your refactoring into interpret config io (next (result (sourceFilepath source) source))

@brianrourkeboll
Copy link
Contributor

brianrourkeboll commented Nov 23, 2023

I think the issue is the usage of the match lambda (aka function). By that, you are giving back a function.

I don't think it's the function on its own, but rather the fact that there are intermediate definitions that need to be hoisted (like the local result function) and/or are closed over in the returned function.

But yes, @dawedawe is right that making the last parameter explicit in the outer let rec should work—compare this versus this.

One could argue that the compiler should treat the two forms the same (like it does when there are no intermediate definitions in something like let rec f x = fun y -> …), but it doesn't currently.

@MiroslavHustak
Copy link
Author

MiroslavHustak commented Nov 23, 2023

Thanx all for trying to help :-).

Actually, my primary aim was not to make the recursive functions tail-recursive (although I appreciate your help), but to test the new fantastic feature (IMHO extremely useful for everyone) and see whether the warnings do appear.

The inconsistency of the warning messages (they sometimes appeared, other times not) was the first issue, the second one was that the interpret non-tail recursive function containing issue 6984 generated an error rather than a warning.

If it should stay like that, perhaps it will be fine to mention it in the documentation. Also, perhaps it might be useful to mention that local functions cannot be annotated with [<TailCall>] and should be tested separately.

I wish I could help you somehow, but I am not a very experienced programmer :-(.

@dawedawe
Copy link
Contributor

If it should stay like that, perhaps it will be fine to mention it in the documentation. Also, perhaps it might be useful to mention that local functions cannot be annotated with [<TailCall>] and should be tested separately.

Well, local functions can just never have any attributes. It's a weak spot of the language, I agree.

Regarding the error: I was able to reproduce FS0251 locally with the piped implementation and will look into it.

@dawedawe
Copy link
Contributor

@MiroslavHustak
I have a fix to not trigger FS0251 during the analysis but before pushing it I want to do some historical digging to see how things were when the first attempt for the [<TailCall>] attribute was made some years ago.

@MiroslavHustak
Copy link
Author

MiroslavHustak commented Nov 26, 2023

@dawedawe @brianrourkeboll
Making the last parameter explicit in the outer let rec does work. Thanx for that. No warnings/errors appear now.

Anyway, I was curious what would happen when I made the recursive call "not optimised" (#6984) for the now tail-recursive function - applying the piping operator |>. And I received neither the FS0251 error, nor the FS3569 warning. I am baffled.

Let me summarise the behaviour of the non-tail recursive and tail-recursive interpret functions. Actual and expected results are in the comments in the code:

[<TailCall>] //non-tail-recursive function
let rec private interpret config io = //for testing purposes
    //some code
    function //CommandLineProgram<unit> -> unit
    | Pure x -> x
    | Free (SourceFilepath next) ->
                                    //Some code
                                    let param = next (result (sourceFilepath source) source) 
                                    interpret config io param 
                                    //A build ends with a warning FS3569 (as expected)
    | Free (DestinFilepath next) ->
                                    //Some code 
                                    next (result (destinFilepath destination) destination) |> interpret config io 
                                    //A build ends with an error FS0251 (hopefully a warning in future)
    | Free (CopyOrMove (s, _))   -> //Some code
                                    

[<TailCall>] //tail-recursive function
let rec private interpret config io clp = //for testing purposes
    //some code
    match clp with 
    | Pure x -> x
    | Free (SourceFilepath next) ->
                                    //Some code
                                    let param = next (result (sourceFilepath source) source) 
                                    interpret config io param 
                                    //A build succeeds without a warning (as expected)
    | Free (DestinFilepath next) ->
                                    //Some code
                                    next (result (destinFilepath destination) destination) |> interpret config io 
                                    //A build succeeds without neither a warning nor an error,
                                    //but an error (a warning in future) is expected 
    | Free (CopyOrMove (s, _))   -> //Some code`

@dawedawe
Copy link
Contributor

@MiroslavHustak
There's a bug in the TailCall analysis causing the FS0251 error. The usage of function and piping in the recursive call triggers it.
I have a fix for it. I just need to clean it up and will raise a PR for it soon.

As your second interpret function is not using function the bug is not triggered.
So as far as I understand the situation, what you see makes sense.

Anyway, I was curious what would happen when I made the recursive call "not optimised" (#6984) for the now tail-recursive function - applying the piping operator |>. And I received neither the FS0251 error, nor the FS3569 warning. I am baffled.

By using match instead of function, the compiler is actually able to optimize that into a loop, even though you're using the pipe.
See here
That's Brian's minimal repro using the pipe. As you can see, it's still optimized to a loop.

@MiroslavHustak
Copy link
Author

By using match instead of function, the compiler is actually able to optimize that into a loop, even though you're using the pipe.

Yeah, now I got the point. I've got the big picture now. Thanx very much for your explanation. This information is IMHO important and worth including in some textbook :-).

dawedawe added a commit to dawedawe/fsharp that referenced this issue Nov 27, 2023
- use GetValReprTypeInFSharpForm instead of GetTopTauTypeInFSharpForm
- fixes dotnet#16330
- add regression tests
@0101 0101 removed the Needs-Triage label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants