Skip too complex guards #570
Conversation
Since too complex guards lead to skipping a function altogether now, this condition is redundant. Dropping it allows exhaustiveness checking to benefit from the simple refinements from guards that are supported.
There was a problem hiding this comment.
Hi again! I am also for lowering the number of false positives in cases our code is not smart enough yet. To be honest, I am not really sure of the scope/implications of the changes in this PR though. Do I understand it correctly that all functions that contain any guard other than a single is_TYPE/1 (such as is_integer/1) guard are now skipped (as refine_vars_by_mismatching_clause/3 throws the {skip, too_complex_guards} tuple?
| @@ -0,0 +1,19 @@ | |||
| -module(type_refinement_should_fail). | |||
|
|
|||
| -export([pattern_prevents_refinement/2]). | |||
There was a problem hiding this comment.
Shouldn't we export all three functions?
| -spec guard_prevents_refinement2(erlang:timestamp()) -> ok. | ||
| guard_prevents_refinement2(X) when is_integer(X), X rem 7 == 0 -> ok; | ||
| guard_prevents_refinement2(infinity) -> ok. % can still be an integer |
There was a problem hiding this comment.
I know that you just moved this function, but still, the comment can still be an integer seems a bit weird to me. The type erlang:timestamp() is defined as the tuple {MegaSecs :: non_neg_integer(), Secs :: non_neg_integer(), MicroSecs :: non_neg_integer()}, so it can never be an integer (nor infinity). Maybe @zuiderkwast as the author of this function understands it better. 🙂
| solve_constraints = false :: boolean(), | ||
| %% Skip functions whose guards are too complex to be handled yet, | ||
| %% which might result in false positives when checking rest of the functions | ||
| no_skip_complex_guards = false :: boolean() |
There was a problem hiding this comment.
I appreciate adding the option (as it enables us to test Gradualizer's behaviour with/without it easily). I would also add the CLI option for it (to src/gradualizer_cli.erl and to the README.md).
This PR skips processing functions which use complex guards in order to make Gradualizer more convenient for everyday use, based on the fact that such complex guards aren't handled properly yet. It's important to underline that this PR doesn't lead to fewer or inferior warnings, since the warnings that are skipped could only be false positives anyway, due to incorrect/missing parameter refinement.
This enables Gradualizer users to more conveniently use the tool on "random", i.e. Gradualizer-unaware, projects, thanks to fewer false positives being raised. The PR also contains examples of refactoring (some) unhandled Erlang constructs into equivalent ones that are already handled completely by the tool.
Gradualizer developers can see which Erlang constructs and what guards would be worth handling next. We can also see that with this change some assertions can be dropped from Gradualizer code, yet maintaining that the tool can self-check.