Skip to content

Commit

Permalink
Fix: no-unmodified-loop-condition false positive (fixes #5445)
Browse files Browse the repository at this point in the history
Previously, this rule has been checking whether every BinaryExpression
was modified or not. But this way has been reporting wrong warnings in
chaining binary expression (e.g. `foo + bar < 1`).
Now, this rule came to check whether the top of BinaryExpressions was
modified or not.
  • Loading branch information
mysticatea committed Mar 5, 2016
1 parent 2225616 commit 9592c45
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 14 deletions.
25 changes: 12 additions & 13 deletions lib/rules/no-unmodified-loop-condition.js
Expand Up @@ -30,8 +30,8 @@ var DYNAMIC_PATTERN = /^(?:Call|Member|New|TaggedTemplate|Yield)Expression$/;
/**
* @typedef {object} LoopConditionInfo
* @property {escope.Reference} reference - The reference.
* @property {ASTNode[]} groups - BinaryExpression or ConditionalExpression
* nodes that the reference is belonging to.
* @property {ASTNode} group - BinaryExpression or ConditionalExpression nodes
* that the reference is belonging to.
* @property {function} isInLoop - The predicate which checks a given reference
* is in this loop.
* @property {boolean} modified - The flag that the reference is modified in
Expand Down Expand Up @@ -67,13 +67,13 @@ function isUnmodified(condition) {

/**
* Checks whether or not a given loop condition info does not have the modified
* flag and does not have any groups that this condition is belonging to.
* flag and does not have the group this condition belongs to.
*
* @param {LoopConditionInfo} condition - A loop condition info to check.
* @returns {boolean} `true` if the loop condition info is "unmodified".
*/
function isUnmodifiedAndNotBelongToGroup(condition) {
return !condition.modified && condition.groups.length === 0;
return !(condition.modified || condition.group);
}

/**
Expand Down Expand Up @@ -144,7 +144,7 @@ function toLoopCondition(reference) {
return null;
}

var groups = [];
var group = null;
var child = reference.identifier;
var node = child.parent;
while (node) {
Expand All @@ -153,7 +153,7 @@ function toLoopCondition(reference) {
// This reference is inside of a loop condition.
return {
reference: reference,
groups: groups,
group: group,
isInLoop: isInLoop[node.type].bind(null, node),
modified: false
};
Expand All @@ -163,13 +163,13 @@ function toLoopCondition(reference) {
}

// If it's inside of a group, OK if either operand is modified.
// So stores the group that the reference is belonging to.
// So stores the group this reference belongs to.
if (GROUP_PATTERN.test(node.type)) {
if (hasDynamicExpressions(node)) {
// This expression is dynamic, so don't check.
break;
} else {
groups.push(node);
group = node;
}
}

Expand Down Expand Up @@ -251,7 +251,7 @@ module.exports = function(context) {
}

/**
* Registers given conditions to groups that the condition is belonging to.
* Registers given conditions to the group the condition belongs to.
*
* @param {LoopConditionInfo[]} conditions - A loop condition info to
* register.
Expand All @@ -261,12 +261,11 @@ module.exports = function(context) {
for (var i = 0; i < conditions.length; ++i) {
var condition = conditions[i];

for (var j = 0; j < condition.groups.length; ++j) {
var be = condition.groups[j];
var group = groupMap.get(be);
if (condition.group) {
var group = groupMap.get(condition.group);
if (!group) {
group = [];
groupMap.set(be, group);
groupMap.set(condition.group, group);
}
group.push(condition);
}
Expand Down
3 changes: 2 additions & 1 deletion tests/lib/rules/no-unmodified-loop-condition.js
Expand Up @@ -41,7 +41,8 @@ ruleTester.run("no-unmodified-loop-condition", rule, {
"for (var foo = 0; foo; ++foo) { }",
"for (var foo = 0; foo;) { ++foo }",
"var foo = 0, bar = 0; for (bar; foo;) { ++foo }",
"var foo; if (foo) { }"
"var foo; if (foo) { }",
"var a = [1, 2, 3]; var len = a.length; for (var i = 0; i < len - 1; i++) {}"
],
invalid: [
{code: "var foo = 0; while (foo) { } foo = 1;", errors: ["'foo' is not modified in this loop."]},
Expand Down

0 comments on commit 9592c45

Please sign in to comment.