jfmengels / elm-review-simplify

Provides elm-review rules to simplify your Elm code
https://package.elm-lang.org/packages/jfmengels/elm-review-simplify/latest/
BSD 3-Clause "New" or "Revised" License
20 stars 9 forks source link

expectNaN behaviour in default for literal NaN #141

Closed gampleman closed 1 year ago

gampleman commented 1 year ago
Simplify: Dividing 0 always returns 0

133|             \_ ->
134|                 (0 / 0)
                      ^^^
135|                     |> Float.Extra.boundaryValuesAsUnicode (Float.Extra.toFixedSignificantDigits 2)

This can be fixed by enabling the expectNaN behavior. But I suggest there is an exception in the default config for this particular case, as its the most common and easiest way to explicitly make an NaN value.

lue-bird commented 1 year ago

Not adding expectNaN means that you do not expect to find NaN values running your code. Why should elm-review-simplify, configured as "NaN should not exist in this codebase", disable some checks if it sees an obvious NaN?

Adding expectNaN just seems right here because NaN is a possible (and therefore expected) input and a case you test for. Anything I've missed?

gampleman commented 1 year ago

The alternative is to just change the error message. Dividing 0 always returns 0 is true enough in most cases, but here it is obviously wrong.

jfmengels commented 1 year ago

I think the solution in this case would be to check whether both operands are the literals 0 (as an integer, hex or float), in which case we don't report anything, regardless of whether we're expecting NaN. As @gampleman says, it is wrong and it is in fact not simplifiable.

If we are not expecting NaN, if either value is not the literal 0, then we will do the expected simplification. Does that sound right?

jfmengels commented 1 year ago

I'm happy to hold back on this and discuss it further if you don't agree with this @lue-bird. What is bothering you about the proposal? Is it the fact the we should prevent people from doing 0 / 0 when we're not expecting NaN?

lue-bird commented 1 year ago

Exactly that. You're doing something in your code that simplify is not expecting because you configured it otherwise. The tool should at least tell you that you introduced a value that breaks your promise.

jfmengels commented 1 year ago

Yeah, that makes sense as well. Both approaches make sense to some extent. Just throwing an idea out there: We could have a configuration option to say "expect NaN, but only in these files" (|> expectNaN [ "tests/X.elm" ]). That way, users like @gampleman in this case would be less reticent to use that setting? Because I guess you mostly don't expect NaN, it's just in this test or something, right?

lue-bird commented 1 year ago

If you're testing for NaN, you're clearly expecting it to be a possible value in your code, right?

gampleman commented 1 year ago

Personally at this point I think the best solution is to just customise the error message in this case - it’s misleading and a better solution might be to point the user at the expectNaN configuration option (I’d guess many users might not know about it having used a template or just having forgotten about it).