Conditions should not unconditionally evaluate to "TRUE" or to "FALSE"

[alexeieleusis]

I would like to think of this as the first pass implementation (or even first draft) since is quite complex to figure out false things and I am not an expert in either parsing, deducting, logic programming or whatsoever. I am open to discuss the feature and approach and I am aware that might be too complex to be included in the linter. Still I think adds a lot of value since it is very common to break this rule as you can see in the metrics here https://nemo.sonarqube.org/

Ran this rule in the following projects dart-lang/sdk, dart-lang/markdown and dart-lang/reflectable and these were the only occurrences reported, there are some debatable false positives that rely on side effects. SDK at commit 8caa5a6dcb9f35a101516460d6b0abf12affeaf5

/Users/.../development/dart/sdk/sdk/lib/_internal/js_runtime/lib/js_mirrors.dart 1967:11 [lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE"
/Users/.../development/dart/sdk/sdk/lib/_internal/js_runtime/lib/js_number.dart 249:9 [lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE"
/Users/.../development/dart/sdk/sdk/lib/_internal/js_runtime/lib/js_number.dart 288:9 [lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE"
/Users/.../development/dart/sdk/sdk/lib/core/duration.dart 247:11 [lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE"
/Users/.../development/dart/sdk/sdk/lib/core/uri.dart 2322:15 [lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE"
/Users/.../development/dart/sdk/sdk/lib/html/dartium/html_dartium.dart 3108:9 [lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE"
/Users/.../development/dart/sdk/sdk/lib/io/secure_socket.dart 868:17 [lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE"

[pq]

cc @bwilkerson

[bwilkerson]

I looked over the code briefly, but haven't had a chance to look at it in depth. But I'll make a couple of comments.

First, you've stepped on a land-mine :-), by which I mean that this kind of rule is really hard to define without causing lots of discussions / disagreements. We once (foolishly) tried to introduce a much less powerful version of this rule in which we checked for expressions that conformed to the specification of a constant expression, and flagged those cases. The very first case we heard back about was

const bool debugging = false;

  if (debugging) {
  }

After a few more conversations about why nearly every expression we could think of had some valid use case (and hence shouldn't be flagged), we backed out the rule. Of course, we we foolish enough to try doing this as a hint (we didn't have lints at that point), which at the time couldn't be opted out of, so maybe a lint will have better success.

That said, if you still want to pursue this, I think there's a better approach: write a visitor class that maintains the state you need to track as you traverse the AST structure (such as what you know to be true because you're in the 'then' branch of an 'if' statement). You might also want a separate visitor to visit boolean expressions and return an indication of whether the condition has a known value and what the 'true' and 'false' implications are. You can see an example of this approach in both ExitDetector and ConstantVisitor (used by ConstantEvaluator). It's a little harder to see, but we also do something similar in the resolver when computing the propagated type of variables.

Let me know if you want more detailed feedback at this point in time.

[alexeieleusis]

Thanks for the feedback, I'll look in the classes you recommend and come back with questions. In the meantime I am surprised that you recommend writing a visitor class that maintains state, I thought it was a requirement or at least a convention to make the visitor implementors stateless, what I did is build such state with a function and use it to keep track of the things exactly as you mention, the ones I know have to be true or false in order to get to the node under analysis.

The concrete question is, Is there a place (besides the code) where I can read about the design decisions, requirements conventions or similar to write new rules?

[mezoni]

I think that it is not so hard to implement the following resolvers:

• Expression evaluators
• Variable assignment trackers

They can be used (at least) for tracking the (re)assignment of the primitive types such as the num, bool and String, and of course, the value null for all rest of types.
Also they can be useful for the recognition the results of the expressions with type test (is and is!).
The same approach I used in my PEG parser generator.
This is not the same but a similar approach to solving the problem.

[mezoni]

Very very simple example of that how it can works and how it can be implemented.
binary_declarations/lib/src/expression_evaluator/expression_evaluator.dart

Even if it a much more complext then this does not means that is not possible.

[bwilkerson]

I thought it was a requirement or at least a convention to make the visitor implementors stateless ...

It is. I guess I wasn't very clear. What I'm suggesting is that your lint visitor look for top-level functions and methods, then create a separate visitor that can maintain the state as it visits the function body. (You'll have to do some extra work if you want to visit closures in initialization expressions.) The second visitor might even use a third visitor when it gets to the conditions that you're actually testing, but that's mostly for clarity in the code. (You actually can't do what I was suggesting with the current implementation of lint rules because you don't control when the children are visited and hence can't unwind as you leave a node.)

[bwilkerson]

The other thing I'll suggest is that you think about whether some of these checks could be smaller and faster when separated out into their own lints. Consider for example the request in dart-lang/sdk#26311, which could be folded into this check, but which also makes a perfectly good stand-alone lint.

Also, having separate lints makes it easier to produce good feedback to the user by being more specific. One of the other lessons I've learned in this area is that at some level of complexity it becomes necessary to explain how you arrived at your conclusion. Seeing code like

if (x == y) ...

and being told that 'Conditions should not unconditionally evaluate to "TRUE" or to "FALSE" ' doesn't explain why we thought that the condition had a single outcome. Is it because of the types of x and y? Because we previously tested that x != y? Just something else to consider. :-)

[alexeieleusis]

Wow! thanks for the valuable feedback, I'll look at all the recommended code and consider splitting this into smaller lints preparing to have this in a better shape :)

[pq]

Smaller would be better for sure. All in all, great stuff!

[alexeieleusis]

A comments on the feedback received:

I think there's a better approach: write a visitor class that maintains the state you need to track as you traverse the AST structure (such as what you know to be true because you're in the 'then' branch of an 'if' statement)

This rule keeps track of that state up to the closest containing function, though in the same visitor, certainly having a more robust engine to evaluate expressions seems to be required.

Seeing code like if (x == y) ... and being told that 'Conditions should not unconditionally evaluate to "TRUE" or to "FALSE" ' doesn't explain why we thought that the condition had a single outcome...

I remember you (@bwilkerson) mentioned in a different issue that making use of an explanatory message would be useful for some other rule, we could use a similar approach here, actually we know what nodes have contradictory values, is just that there is no easy way to communicate back, even linting both nodes would be confusing unless they are on the same line.

They can be used (at least) for tracking the (re)assignment of the primitive types such as the num, bool and String, and of course, the value null for all rest of types.

The rule keeps track of reassignments and discards such variables like in the test 317d455#diff-39221ee11f2f70a30d529b2c35683473R84 though certainly detecting is and is! should be performed by this kind of rule.

[bwilkerson]

... making use of an explanatory message ...

I suspect I was referring to the "correction" message, which is intended to explain how to fix the problem, not to provide more information about what's wrong. From the documentation for ErrorCode:

---

Generally, we want to provide messages that consist of three sentences. From the user's perspective these sentences should explain:

1. what is wrong,
2. why is it wrong, and
3. how do I fix it.
   However, we combine the first two in the [message] and the last in the [correction].

---

What I'm talking about here is the second part: explaining why it's wrong.

... we know what nodes have contradictory values, is just that there is no easy way to communicate back ...

True. What I really want is IDE support for hovering over the highlighted text and having an overlay appear that consists of an arrow from the highlighted text to the earlier expression that contradicts it, with a label on the arrow explaining that this is why we know that this condition has a constant value. Unfortunately, such an interface does not yet exist.

In the meantime, we could potentially craft a message, such as:

This condition will always be false because the condition on line 3 indicates that the opposite condition will be true here.

[alexeieleusis]

Finally addressed all comments and worked on performance, currently I detect 3 performance groups in the set of implemented rules, this rule is in the middle group, current implementation is almost an order of magnitude faster than previous implementation.

Ran the rule in the SDK codebase and the only false positives are the ones relying on side effects, which are not in the scope of this rule.

Below some snippets of the addressed comments.

write a visitor class that maintains the state you need to track as you traverse the AST structure (such as what you know to be true because you're in the 'then' branch of an 'if' statement).

Done

You might also want a separate visitor to visit boolean expressions and return an indication of whether the condition has a known value and what the 'true' and 'false' implications are.

Done

They can be used (at least) for tracking the (re)assignment

Done

What I'm suggesting is that your lint visitor look for top-level functions and methods, then create a separate visitor that can maintain the state as it visits the function body.

Done

The other thing I'll suggest is that you think about whether some of these checks could be smaller and faster when separated out into their own lints.

Done

One of the other lessons I've learned in this area is that at some level of complexity it becomes necessary to explain how you arrived at your conclusion.

Done, the rule now prints the conflicting node, which is always before the current node, e.g:

void nestedOk4() {
  if (foo <= bar) {
    return;
  }

  if (foo == bar) {} // LINT
}


~/development/dart/linter/test/rules/invariant_boolean.dart 92:7 [lint] Conditions should not unconditionally evaluate to "TRUE" or to "FALSE" verify: foo <= bar.
  if (foo == bar) {} // LINT
      ^^^^^^^^^^

[bwilkerson]

It would be more efficient to only visit binary expressions when they are children of an if statement than to test the parent of every binary expression. (That is, don't use a visit method for binary expressions, but invoke a helper method, or maybe just inline the code, when the condition is a binary expression.)

[alexeieleusis]

Done.

[bwilkerson]

We probably also want to visit the condition in a DoStatement.

[alexeieleusis]

Done.

[alexeieleusis]

I ran the travis script locally and it passed, also the error is before the tests ran. I don't know how to force it to run again though.

[alexeieleusis]

Friendly ping.

[pq]

Nit: in keeping with naming elsewhere, let's rename this to: InvariantBooleans.

[alexeieleusis]

done

[pq]

Just a bunch of naming nits from me... After that, LGTM!

[matanlurey]

So this is neat, but just wondering, why isn't this caught by the analyzer's own "unreachable code"-analysis, or what would it take to promote this?

[bwilkerson]

@matanlurey We don't want any hints with false positives. Previous experience with this kind of lint suggests that it will have a fair number of false positives. Perhaps we can refine it to the point where it no longer does and then promote it.

[matanlurey]

Cool, just checking. SGTM 👍

(I'd be interesting in dry running this internally and see what comes up)

[pq]

Thanks for all the fixes @alexeieleusis !

[alexeieleusis]

Thanks for the review and the patience :)