Add analyzer for ignoring results in do!.

[fkj]

This pattern is dangerous because it makes it easy to write very subtle bugs that ignore error handling.

[mickhansen]

I have a reasonable use case for Result.ignore, it's nice to use with traverseResultA which can yield a unit list

[fkj]

I have a reasonable use case for Result.ignore, it's nice to use with traverseResultA which can yield a unit list

Sure, it can be useful in general. This is just to disallow the pattern do! ... |> Result.ignore and variants.

[mickhansen]

Sure, it can be useful in general. This is just to disallow the pattern do! ... |> Result.ignore and variants.

do! List.traverseResultA (fun -> ...) |>Result.ignore is a pattern I used recently.

[fkj]

Sure, it can be useful in general. This is just to disallow the pattern do! ... |> Result.ignore and variants.

do! List.traverseResultA (fun -> ...) |>Result.ignore is a pattern I used recently.

Can you send me a link to the code where this was necessary?

[mickhansen]

Can you send me a link to the code where this was necessary?

Guess I used Result.map ignore

https://github.com/criipto/signatures-api/blob/068e61dead95871bd1a2dec7bde17f1dea477ab7/GraphQL/Schema/SignMutation.fs#L154

https://github.com/criipto/signatures-api/blob/068e61dead95871bd1a2dec7bde17f1dea477ab7/GraphQL/Schema/SignMutation.fs#L713

[fkj]

Can you send me a link to the code where this was necessary?

Guess I used Result.map ignore

https://github.com/criipto/signatures-api/blob/068e61dead95871bd1a2dec7bde17f1dea477ab7/GraphQL/Schema/SignMutation.fs#L154

https://github.com/criipto/signatures-api/blob/068e61dead95871bd1a2dec7bde17f1dea477ab7/GraphQL/Schema/SignMutation.fs#L713

Same, same - the analyzer will find both patterns.

In the first case, the analyzer will not be triggered because the Result.map ignore call is not the final thing in the pipeline. So if the Result is accidentally double-wrapped, the types are unlikely to work out, which would cause a type error.

The second case could be written with Result.map (List.reduce (fun () () -> ())) instead of Result.ignore to make it safer. Maybe it would be useful to define our own function for the overall "traverse a list and stop on the first error, but don't give me the result" use case?

[fkj]

Results of running on the Verify solution

7 occurrences detected, none of which were false positives.
All occurrences were validation code which is the target of the analyzer.
Almost of the occurrences look like they could be fixed with minor rewrites.

[kasperhj]

LGTM. Should we consider ObservedTaskResult.ignore?

[fkj]

LGTM. Should we consider ObservedTaskResult.ignore?

Sure, though I think it might belong in a separate analyzer suite. It's common to package analyzers with a library, so might make sense to maintain it with the implementation (which I think might make sense to put into a separate library anyway).