search

wartremover / wartremover

Flexible Scala code linting tool
https://www.wartremover.org/
Apache License 2.0
1.07k stars 113 forks source link

Disable Future#Filter and Future#withFilter? #185

Open jimmydivvy opened 9 years ago

jimmydivvy commented 9 years ago

The filter and withFilter methods on scala.concurrent.Future simply cause the entire future to fail with an unhelpful error message and no meaningful stack trace.

There doesn't appear to be any valid implementation of filter possible for this type - should this be added as a rule in WartRemover?

puffnfresh commented 9 years ago

Generally I want to disable withFilter. Is that enough for this? https://github.com/puffnfresh/wartremover/issues/55

jimmydivvy commented 9 years ago

My understanding is that withFilter is just a non-strict version of filter, and in this context they'd both be valid or invalid together. (Although it sounds like there may be separate issues with withFilter in #55?)

In Haskell terms filter/withFilter is only relevant on a type with a MonadPlus instance. There is no valid mzero possible for a Future, so Future#filter is (in my opinion) always incorrect and should be a compile failure.

Many other types (List, Vector, Seq, etc) do have valid mzero instances so filter and withFilter are valid.

The current behaviour of Future#filter is to return a failed Future containing NoSuchElementException: Future.filter predicate is not satisfied. Due to trampolining the resulting stack trace has no meaningful information whatsoever.

This has actually tripped me up pretty severely in production, and the only way to track down the offending line was to write a custom rule for wartremover (Due to the lack of useful stack traces). I can throw this up in a PR if you'd like, although it was pretty much just a copy-paste-tweak on the rule for Option#get

tl;dr Not only is Future#filter inherently unsafe and always incorrect, but it fails in a way that's virtually impossible to figure out where in the codebase it actually happened, is very difficult to grep for, and looks completely innocuous on the surface, especially when de-sugared from if in a for-comprehension.

puffnfresh commented 9 years ago

@jimmydivvy sounds like filter is broken, :+1: for a PR!

fommil commented 7 years ago

lot's of :+1: for this... my book covers all the horror of for in chapter two https://leanpub.com/fp-scala-mortals