invariant_booleans: False positives and negatives with repeated conditions.

[rkirsling]

In 2.0.0-dev.4.0, invariant_booleans behaves erratically when the same condition occurs multiple times in a function.

First, checking the value of a field before and after updating it can cause a false positive:

class Counter {
  int count = 0;
  void increment() { count++; }
}

void test1() {
  final counter = new Counter();
  if (counter.count > 0) return;

  counter.increment();
  if (counter.count > 0) return; // false positive here

  print('hello');
}

The choices of > 0 and ++ for comparison and assignment don't matter, and int can be replaced with a different numerical type. Even String with += 's' will repro.

Now, if we modify our test function to use a ternary, the problem goes away...

int test2() {
  final counter = new Counter();
  if (counter.count > 0) return 1;

  counter.increment();
  return counter.count > 0 ? 1 : 0; // true negative here
}

...but don't rejoice just yet, because if we keep going we hit a false negative!

class FixedCounter {
  final int count = 0;
}

int test3() {
  final counter = new FixedCounter();
  if (counter.count > 0) return 1;
  return counter.count > 0 ? 1 : 0; // false negative here
}

And even without the ternary, if we use a boolean field, invariant_booleans ironically doesn't care how many times we check it. 😆

class A {
  final bool foo = false;
}

void test4() {
  final a = new A();
  if (a.foo) return;
  if (a.foo) return; // false negative here

  print('hello');
}

---

Follow-up: It didn't occur to me earlier, but the false positive seems to hinge on the first conditional being an early return.

[pq]

cc @alexeieleusis

[lexaknyazev]

Same issue here (also 2.0.0-dev.4.0):

void main() {
  final bar = 0;
  final foo = 10;

  for (var i = 0; i < foo; ++i) {}

  for (var i = 0; i < foo; ++i) {} // invariant_booleans: i < foo 

  if (bar == 10) {}
}

[kevmoo]

Hitting this as well. @alexeieleusis ? @pq ?

[pq]

Sorry, no update just yet. Hoping to set aside some time to look in the coming week.

cc @bwilkerson

[alexeieleusis]

I have a fix for the bug itself, some other tests broke though. I don't know yet if the problem are the tests themselves, will upload the progress I have if I can't finish it before Tuesday (2/20).

[pq]

Awesome. Thanks @alexeieleusis !

[rkirsling]

While @alexeieleusis' PR targets the case @lexaknyazev demonstrated above, it doesn't address a single one of the cases in the issue description (none of which include a for loop).

Please reopen this issue (as I evidently don't have the permissions to).

[alexeieleusis]

We decided when the rule was implemented that false negatives like the one produced by invoking counter.increment() in the first examples would exists and the cost of detecting those was not worth the effort of implementing them (if possible at all, sounds dangerously close to the halting problem) neither the cost of computing it.

The rule detects early returns and does not use those conditions, as @rkirsling points out.

As for ternary operators, sounds like a missing implementation.

I am unsure about adding it, the reason being that I haven't found a single true positive in code in the real world. The idea behind this rule was to catch debugging leftovers, which in my case, are always caught by literal_only_boolean_expressions.

Thoughts?

[pq]

cc @bwilkerson

[rkirsling]

That makes sense—I mentioned the false negatives so as to give the most complete description of the problem that I could, but it's really just that the false positive is plaguing me in actual code. 😄

[alexeieleusis]

Looks like you are writing an interpreter, I wonder how useful this lint is for you. I have the impression that performance constraints require changes undetectable for the lint to be common place.

[rkirsling]

As a policy, I've been keeping as many lint rules enabled as possible. If I disagree with a rule, I'll turn it off, but I don't disagree with invariant_booleans, I've just noticed that it's reporting a problem where there isn't one. It's okay that it can't detect every real problem. 😅

[pingbird]

Reproduced:

void main() {
  Foo()
    ..a() // prints 'true'
    ..a()
    ..a(); // prints 'false'
}

class Foo {
  int x = 1;

  void a() {
    if (x + 2 > 2) {
      b();
      // Produces an invariant_booleans warning
      if (x + 2 <= 2) {
        print('true');
        return;
      } else {
        print('false');
      }
    }
    x += 2;
  }

  void b() {
    x--;
  }
}

[alexeieleusis]

I think the lint to flag a case like the one above is too ambitious, the most common use case I have (in any language) is forcing a condition to be true or false for debugging purposes, in such cases I use constants (with a FIXME comment) to avoid committing that change. Catching a case like the one in the example above can become too expensive rendering the lint unusable (think of the code changing a property inside an object and the code is performed in a different library).

[srawlins]

We've deprecated invariant_booleans and will not be fixing correctness bugs going forward.