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

ignore keyword giving unexpected results #15880

Open
MarcDHall opened this issue Aug 28, 2023 · 8 comments
Open

ignore keyword giving unexpected results #15880

MarcDHall opened this issue Aug 28, 2023 · 8 comments
Milestone

Comments

@MarcDHall
Copy link

Repro steps

open System.Threading.Channels
open FSharp.Control

let channel = Channel.CreateUnbounded<int>();

let puts = task {
    return! 0 |> channel.Writer.WriteAsync
    return! 1 |> channel.Writer.WriteAsync  
} 

puts |> Async.AwaitTask |> Async.RunSynchronously |> ignore

channel.Writer.Complete |> ignore 

channel.Reader.ReadAllAsync()
|> AsyncSeq.ofAsyncEnum
|> AsyncSeq.iter (fun x -> printfn "%i" x)
|> Async.RunSynchronously
|> ignore

"Done" |> System.Console.WriteLine |> ignore;

Expected behavior
The IAsyncEnumerable returned by ReadAllAsync() should behave like a finite set of 2 elements and come to an end.

Actual behavior
The IAsyncEnumerable never ends. "Done" is never written to the stdout. The Channel isn't marked as closed.

Workaround
Swapping to channel.Writer.Complete() will cause this code to terminate.

I am new to F#, so this might be my ignorance, but my understanding is that Complete() and Complete |> ignore should be equivalent in this case. The only odd thing about channel.Writer.Complete, that I can see, is that it takes an optional parameter. Have I missed something obvious here? I couldn't find any mention of this type of behaviour while everything I've read suggests that |> ignore is the correct, idiomatic syntax.

@github-actions github-actions bot added this to the Backlog milestone Aug 28, 2023
@kerams
Copy link
Contributor

kerams commented Aug 28, 2023

ignore is used to throw away a value ("convert" it to a unit). If you drop the parentheses, you never call the method. Instead, you're passing it, not the return value, to ignore. channel.Writer.Complete |> ignore is a no-op, it literally doesn't do anything or result in any IL instructions.

@vzarytovskii
Copy link
Member

vzarytovskii commented Aug 28, 2023

As @kerams mentioned, since ignore is just normal function, which takes anything and returns a unit (()), X |> ignore is an equivalent of ignore X or ignore (X), so when you're doing channel.Writer.Complete |> ignore you're essentially taking a method group and passing it to ignore, like ignore(channel.Writer.Complete) without calling it.

You probably want channel.Writer.Complete() |> ignore in this case.

@MarcDHall
Copy link
Author

MarcDHall commented Aug 28, 2023

Thanks for the replies.

That's interesting. So why does this work?

"Done" |> System.Console.WriteLine |> ignore

@vzarytovskii
Copy link
Member

vzarytovskii commented Aug 28, 2023

Thanks for the replies.

That's interesting. So why does this work?

"Done" |> System.Console.WriteLine |> ignore

Due to piping, first you pass string "Done" to System.Console.WriteLine as argument via |>, then you pass its result (since you made a call) to the ignore, so this essentially will transform into ignore(System.Console.WriteLine("Done")).

@Happypig375
Copy link
Member

Should we warn against ignoring a function?

@vzarytovskii
Copy link
Member

I would say not compiler itself, since, well, ignore is just a function. Would need special treatment from the compiler in this case.
Analyzer would work though.

@Happypig375
Copy link
Member

@vzarytovskii I remember someone proposed a [<NotAFunction>] attribute somewhere for printing functions but cannot find it currently. It can be a good way to do this.

@0101
Copy link
Contributor

0101 commented Sep 4, 2023

This works as expected. But it would be a good use case for an analyzer that could warn users that this code probably won't do what they expect. Or that it doesn't make sense to ignore functions/methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: New
Development

No branches or pull requests

5 participants