feat: detect useless parens in binops

[leana8959]

Related to #14

Currently it only checks for functions in binops, other kinds of detection (e.g., associativity) are to be discussed.

Would it be reasonable to contribute to upstream and implement Ord or PartialOrd so that we could compare the precedence of different syntax nodes?

Related CPPNix documentation: https://nix.dev/manual/nix/2.29/language/operators

[mightyiam]

Hey, if you rebase I'll review.

[leana8959]

What do you think about incorporating this function from nil? https://github.com/oxalica/nil/blob/f80fe365cb441624d1608235e6e793e5dce47fb0/crates/syntax/src/ast.rs#L268-L332

I think it would be nice to be able to handle more than just function applications in binops.

Thank you for your time :)

[mightyiam]

Sorry, I think #138 is the priority so we've been working on that.

[mightyiam]

Now that we have #138 would you mind rebasing (again, sorry)?

[leana8959]

Sure, I'll do it when I have time, maybe this weekend. Thanks :)

[leana8959]

@mightyiam I noticed that there are quite some tests all test on the manual_inherit rule instead of the rule the filename indicates, maybe this is a mistake? useless_parens is one of them. Namely:

$ rg -F "rule: manual_inherit" -c
bin/tests/useless_parens.rs:1
bin/tests/unquoted_uri.rs:1
bin/tests/empty_inherit.rs:1
bin/tests/deprecated_to_path.rs:1
bin/tests/collapsible_let_in.rs:1
bin/tests/manual_inherit.rs:1
bin/tests/eta_reduction.rs:1
bin/tests/empty_pattern.rs:1
bin/tests/redundant_pattern_bind.rs:1
bin/tests/legacy_let_syntax.rs:1
bin/tests/manual_inherit_from.rs:1

For useless_parens I tried to change it to rule: useless_parens but there was no change in the test output (still success). Is the testsuite reflecting the behaviour of the rules correctly?

I'll need to debug a bit since the rnix crate has been updated and the rules I wrote are no longer matching. I tried to run cargo test -p statix --test useless_parens -- --nocapture to get dbg! macro output, but nothing but insta output was shown. Could you share how you debug your rules?

Thank you for your time!

[mightyiam]

@mightyiam I noticed that there are quite some tests all test on the manual_inherit rule instead of the rule the filename indicates, maybe this is a mistake?

Yes, sorry. Corrected.

For useless_parens I tried to change it to rule: useless_parens but there was no change in the test output (still success). Is the testsuite reflecting the behaviour of the rules correctly?

I think the only thing this has affect on is the identifier of the test fn. That, in turn, rarely affects anything else.

I'll need to debug a bit since the rnix crate has been updated and the rules I wrote are no longer matching. I tried to run cargo test -p statix --test useless_parens -- --nocapture to get dbg! macro output, but nothing but insta output was shown. Could you share how you debug your rules?

The tests are CLI tests, so those dbg! invocations would result in the tested process writing to stderr which goes nowhere. I haven't thought of that.

It should be possible to use dbg! invocations outside of those tests, by working against some temporary fixture Nix files. So that's one option.

Another thing we could do is add some explicit "sanity check" CLI tests that have to do with no rule in particular and then convert the implementation of the rule-specific tests to not be end-to-end (testing the behavior of the process) but to be unit tests, testing a private function. Of course, this raises some questions. Let me know whether you'd like to get into that.