-
Notifications
You must be signed in to change notification settings - Fork 166
invariant_boolean: When searching for references reassignment include PostfixExpression and similar, e.g. variable++. #412
invariant_boolean: When searching for references reassignment include PostfixExpression and similar, e.g. variable++. #412
Conversation
.length == | ||
0; | ||
return (AstNode statement) { | ||
bool isMutation(AstNode n) { |
There was a problem hiding this comment.
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 ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bwilkerson: take another look?
return; | ||
} | ||
|
||
offset++; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Fixes dart-lang/sdk#28967 |
.length == | ||
0; | ||
return (AstNode statement) { | ||
bool isMutation(AstNode n) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bwilkerson: take another look?
.takeWhile((n) => n != node) | ||
.where(isMutation) | ||
.length == | ||
0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isEmpty
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha! Irony, I should be using the linter :)
return; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a newline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
… PostfixExpression and similar, e.g. variable++. Fixes https://github.com/dart-lang/linter/issues/337
…ression invariant_boolean: When searching for references reassignment include PostfixExpression and similar, e.g. variable++.
…ression invariant_boolean: When searching for references reassignment include PostfixExpression and similar, e.g. variable++.
…ression invariant_boolean: When searching for references reassignment include PostfixExpression and similar, e.g. variable++.
Fixes dart-lang/sdk#57399