Expand warning when expression result is ignored

[taylorwood]

These changes attempt to address #1108. I ended up using a combination of the suggestions from the issue discussion, but am happy to change per feedback.

[forki]

This looks good. Could you please add a new test case under tests\fsharpqa\Source\Warnings that contains your sample from the screenshot? Thanks

[isaacabraham]

Great stuff! 2 questions: -

1. Why "expr" rather than "expression?
2. I'm putting myself in the shoes of a C# or Python dev who has never seen this before - is the message clear as to why it's happening i.e. this is an expression, it has a result that is being ignored and F# warns you about this because it's trying to guide you towards writing pure functions etc. etc.?

[forki]

I think "Use the ignore function" is what we would want to put into the "consider" part ("Consider to use the ignore function...").
Before that part we should give a short explanation why implicit ignore might be unwanted.

[isaacabraham]

@forki yeah, exactly - there should be some description of why "throwing away" a result is considered "bad form" in F# and why this is potentially a code smell.

This will probably lead to a longer warning message than @KevinRansom would like though :-)

[taylorwood]

@forki @isaacabraham I'm having trouble coming up with an explanation that isn't made redundant by the "consider" suggestions. Also, there are interop scenarios where ignore isn't bad form (collection mutators that return an item), but explaining pure functions and side effects succinctly is hard! I swapped the order to suggest binding first, since that's the more common F# idiom.

This expression has a value of type 'int' that is implicitly discarded. You probably mean to use the value or discard it explicitly. Consider binding to a name, e.g. 'let result = expression', or using the 'ignore' function, e.g. 'expression |> ignore'.

You probably mean to use the value, or explicitly discard it if you're evaluating the expression only for its side effects.

[forki]

No we don't think ignore is bad.
We think implicit ignore is bad.

[taylorwood]

@forki agreed. If we can settle on some reasonable message, I'll push another commit.

[KevinRansom]

Lol bikeshedding.

I think this probably works okay:

The result of this expression is implicitly ignored. Consider using ignore to discard this value explicitly, e.g. 'expr' |> ignore', or 'let' to bind the result to a name., e.g. 'let result = expr'.

[taylorwood]

Updated message from @KevinRansom's suggestion and added test re: @forki

[KevinRansom]

@taylorwood, Hi mate, did you push your latest updates to the messages?

Thanks
Kevin

[taylorwood]

@KevinRansom yes, I used your suggestion for the message in the last commit

[cartermp]

This LGTM.

[KevinRansom]

@taylorwood This looks great,

Thanks for taking care of this.

Kevin

[cartermp]

👍