Skip to content
This repository was archived by the owner on Nov 20, 2024. It is now read-only.

invariant_boolean: When searching for references reassignment include PostfixExpression and similar, e.g. variable++. #412

Merged
merged 1 commit into from
Mar 7, 2017
Merged

invariant_boolean: When searching for references reassignment include PostfixExpression and similar, e.g. variable++. #412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions lib/src/rules/invariant_booleans.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,15 +216,29 @@ AstNodePredicate _isConditionalStatementWithReturn(
AstNodePredicate _noFurtherAssignmentInvalidatingCondition(
Expression node, Iterable<AstNode> nodesInDFS) {
Set<Identifier> identifiers = _findStatementIdentifiers(node.parent);
return (AstNode statement) =>
nodesInDFS
.skipWhile((n) => n != statement)
.takeWhile((n) => n != node)
.where((n) =>
n is AssignmentExpression &&
!identifiers.contains(n.leftHandSide))
.length ==
0;
return (AstNode statement) {
bool isMutation(AstNode n) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fairly inefficient because you have to perform multiple is checks for every node, even when you've already determined that it isn't necessary. Perhaps re-structure slightly:

if (n is AssignmentExpression) {
  return !identifiers.contains(n.leftHandSide);
} else if (n is PostfixExpression) {
  TokenType type = n.operator.type;
  return (type == TokenType.PLUS_PLUS || type == TokenType.MINUS_MINUS) && !identifiers.contains(n.operand);
} else ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bwilkerson: take another look?

if (n is AssignmentExpression) {
return !identifiers.contains(n.leftHandSide);
} else if (n is PostfixExpression) {
TokenType type = n.operator.type;
return (type == TokenType.PLUS_PLUS || type == TokenType.MINUS_MINUS) &&
!identifiers.contains(n.operand);
} else if (n is PrefixExpression) {
TokenType type = n.operator.type;
return (type == TokenType.PLUS_PLUS || type == TokenType.MINUS_MINUS) &&
!identifiers.contains(n.operand);
}

return false;
}

return nodesInDFS
.skipWhile((n) => n != statement)
.takeWhile((n) => n != node)
.where(isMutation)
.isEmpty;
};
}

List<Expression> _splitConjunctions(Expression expression) {
Expand Down
53 changes: 53 additions & 0 deletions test/rules/invariant_booleans.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,3 +237,56 @@ void bug372(bool foo) {
// doSomethingElse();
}
}

void bug337_1(int offset, int length) {
if (offset >= length) {
return;
}

offset++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to introduce other variables so that you can check other operators.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good idea, but I am not sure which ones to check, can you please provide a few examples?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bwilkerson : thoughts on what @alexeieleusis should check?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks the postfix increment operator, but the rule checks for all four combinations of (prefix | postfix) (increment | decrement) operators. There are three operators that are not being tested, and I think they should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I was confused, somehow I thought this was the rule's code. Will add those cases to the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

if (offset >= length) { // OK
}
}

void bug337_2(int offset, int length) {
if (offset >= length) {
return;
}

offset--;
if (offset >= length) { // OK
}
}

void bug337_3(int offset, int length) {
if (offset >= length) {
return;
}

++offset;
if (offset >= length) { // OK
}
}

void bug337_4(int offset, int length) {
if (offset >= length) {
return;
}

--offset;
if (offset >= length) { // OK
}
}

void test337_5() {
int b = 2;
if (b > 0) {
b--;
if (b == 0) {
return;
}
if (b > 0) {
return;
}
}
}