Skip to content

invariant_booleans false positive with while #57535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zoechi opened this issue Apr 10, 2017 · 4 comments
Closed

invariant_booleans false positive with while #57535

zoechi opened this issue Apr 10, 2017 · 4 comments
Assignees
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-false-positive P4 type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@zoechi
Copy link
Contributor

zoechi commented Apr 10, 2017

image

  Future<Null> pageChanged(int newPage) async {
    if (newPage - 1 == currentPage) {
      return;
    }
    if (newPage - 1 > currentPage) {
      while (newPage - 1 > currentPage) {
        await nextPage();
      }
    } else {
      while (newPage - 1 < currentPage) {
        await previousPage();
      }
    }
  }

await nextPage() updates currentPage, therefore this will eventually evaluate to true

@alexeieleusis
Copy link
Contributor

alexeieleusis commented Apr 10, 2017

This is a known limitation of the rule. We thought it was too expensive to look for all places that modify involved variables.

If currentPage is a getter, then we could probably change the rule to ignore them. Thoughts? It might be the right thing to do since is like a method without arguments, there is a convention to no mutate state in them, but still allowed.

@bwilkerson
Copy link
Member

It seems to me (based on research that's been done in this area) that it's better to have no false positives and a weak rule, then work toward making the rule catch more and more issues, than to have a rule that catches 150% of the real problems. False positives cause people to disable lints, and nothing triggers them to re-enable them later.

I think that if a condition includes a reference to any non-local variable or any function (including getters and local functions) that we should assume that we don't know what the truth value of the expression will be. Also note that state mutation can be difficult to detect because the state might be far removed from the object causing it to change.

@alexeieleusis alexeieleusis self-assigned this Apr 10, 2017
@leonsenft
Copy link

Here's a related case that should be simple to detect, as the variable in the "invariant" condition is directly mutated inside the if block.

screen shot 2017-05-18 at 10 05 06 pm

I did a little investigating and realized the lint correctly detects that the variable changes if it's mutated within the if block, but before the while loop. The issue occurs however if the variable is mutated within the loop. Perhaps the reasoning here is that the loop condition is guaranteed to pass the first iteration for for and while loops, so it's initially a wasted check, but for do-while this is not the case as the variable will have been mutated before the condition is checked.

@pq pq added linter-false-positive type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Jun 27, 2018
@srawlins srawlins changed the title should not unconditionally evaluate to "TRUE" or "FALSE" invariant_booleans false positive with while Sep 3, 2022
@srawlins srawlins added the P4 label Sep 21, 2022
@srawlins
Copy link
Member

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

@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 18, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-false-positive P4 type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

7 participants