False positives in Redundant check

[jhrcek]

Thanks for implementing the Redundant check in 2.0.0. It successfully identified many redundant instances of the operator in our codebase. Unfortunately it reports some false positives (cases where the left pizza is in fact NOT redundant).

Here are few noteworthy examples:

714|                 , Attrs.disabled <| not <| hasChangesToSave && nameIsNotEmpty
                                         ^^^^^^^^^^^^^^^^^^^^^^^
...
400|             Html.text <| "Initialization error " ++ Decode.errorToString err
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
..
640|                 [ Html.span [] [ Html.text <| "Week " ++ (String.fromInt <| index + 1) ]
                                                               ^^^^^^^^^^^^^^^^^^^^^^^
...
403|             [ Events.onInput <| config.msg << SearchTermChanged
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It seems that operator precedence is still not properly taken into account :cry:

[Janiczek]

@jfmengels is it the case that elm-review applies packages' operator precedence when parsing with elm-syntax?

[jfmengels]

elm-review is providing information about the operators from dependencies to elm-syntax. There was a bug fixed in elm-syntax v7.1.2 where the operator precedence was incorrect. I would check whether you are using a version of elm-syntax >= 7.1.2 in your elm.json.

If that is not the problem, try to check whether you get the correct AST in https://ellie-app.com/9btkt628226a1, which reproduces what elm-review has. If that is also not it, maybe try to debug the rule to see what is happening? Also, the fix for operators may not be perfect, so that might be it also.

[jhrcek]

I confirm we're using the latest version of elm-syntax as of today (7.1.3) in elm-review's elm.json. I tried couple of the problematic cases in ellie and in all cases I tried the AST looked correctly (i.e. the thing behind <| was not "simple expression" - for example s i = String.fromInt <| i + 1 parses the right hand side of <| as OperatorApplication, which by the implementation of the rule should not be considered simple and thus should not be reported, but it nevertheless IS reported. I'll investigate this some more on monday.

[jhrcek]

I created an isolated a small project demonstrating the issue: https://github.com/jhrcek/elm-review-noleftpizza-SSCCE making sure I was using the latest version of elm-review, elm-syntax and elm-review-noleftpizza.

[jhrcek]

I dove a bit deeper into this and fixed all true positives in our ~70k elm codebase. There are about 150 false positives left. All of them are of the following form:

NoLeftPizza: Redundant left pizza (<|) operator application

   someFunction <| x INFIX_OPERATOR y
   ^^^^^^^^^^^^^^^^^    

Where infix operator is one of : ||, &&, ++, <<, <?>,::,==,*,+,-,/`

I also pasted some of the test cases of this package (e.g. https://github.com/truqu/elm-review-noleftpizza/blob/c0696bd1d92068453fa8ffa3ccaac1df553976bc/tests/NoLeftPizzaTest.elm#L301-L303) into my SSCCE and they have the same false positives issue which is weird, given that tests for this package are actually passing.

[Janiczek]

So, Review.Test.run in the tests does some initialization differently than Review.Rule.review that's being run in the template Main.elm (when you run elm-review)?

[jhrcek]

Yes. It seems that Review.Test.run handles the elm code differently than the CLI tool, which seems not to be taking the operator precedence into account (cc @jfmengels)

[jfmengels]

It took me a while, but I found the issue.

The CLI elm-review contains a "parser application", which I spawn several times to parse files in parallel at startup (and don't use afterwards anymore).

The problem is that this application is compiled at publish time of the CLI (and not at runtime like the "review application"). This means that the elm-syntax version was fixed to an outdated one.

So I just fixed the issue here by bumping the version of elm-syntax to 7.1.3, but ultimately, this will have to be built at runtime using the same elm-syntax version as for your review application, to prevent any odd behavior like in fix or watch mode, where e parse using the elm-syntax version from your review application. I will soon create an issue for that (help would be appreciated, if you'd like to help :wink:).

The fix is available in v2.3.3

Thank you @jhrcek for setting up a nice SSCCE :)

[jhrcek]

@jfmengels This is great! Thank you for the fix :heart: It removed all the false positives.

help would be appreciated

Not sure what help you had in mind. Did you mean opening the issue in node-elm-review? I opened one here. But if what you had in mind was to rearchitect the code so that the "parser application" is compiled at compile time of review applications, I'm not sure if I'd be up for that task (given my lack of JS skills) :smiley:

[jhrcek]

Closing this as the issue was fixed on node-elm-review side: https://github.com/jfmengels/node-elm-review/issues/14

[jfmengels]

Did you mean opening the issue in node-elm-review? I opened one here. Yes I did. Thank you for opening it :blush:

But if what you had in mind was to rearchitect the code so that the "parser application" is compiled at compile time of review applications, I'm not sure if I'd be up for that task (given my lack of JS skills)

That is also what I meant. I would love to get help with the tool :smile: