Skip to content

Commit

Permalink
SONARJS-899 Improve InfiniteLoopCheck: account for simple value ranges (
Browse files Browse the repository at this point in the history 
#487)
  • Loading branch information
@vilchik-elena @inverno
vilchik-elena authored and inverno committed Feb 20, 2017
1 parent 92b3429 commit e06415a
Show file tree
Hide file tree
Showing 2 changed files with 134 additions and 18 deletions.
90 changes: 73 additions & 17 deletions ...cript-checks/src/main/java/org/sonar/javascript/checks/LoopsShouldNotBeInfiniteCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
*/
package org.sonar.javascript.checks;

import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
Expand All @@ -31,48 +33,101 @@
import org.sonar.javascript.cfg.CfgBranchingBlock;
import org.sonar.javascript.cfg.ControlFlowGraph;
import org.sonar.javascript.checks.utils.CheckUtils;
import org.sonar.javascript.se.Constraint;
import org.sonar.javascript.se.SeCheck;
import org.sonar.javascript.tree.impl.JavaScriptTree;
import org.sonar.plugins.javascript.api.symbols.Symbol;
import org.sonar.plugins.javascript.api.symbols.Usage;
import org.sonar.plugins.javascript.api.tree.ScriptTree;
import org.sonar.plugins.javascript.api.tree.Tree;
import org.sonar.plugins.javascript.api.tree.expression.ExpressionTree;
import org.sonar.plugins.javascript.api.tree.expression.IdentifierTree;
import org.sonar.plugins.javascript.api.tree.expression.LiteralTree;
import org.sonar.plugins.javascript.api.tree.statement.DoWhileStatementTree;
import org.sonar.plugins.javascript.api.tree.statement.ForStatementTree;
import org.sonar.plugins.javascript.api.tree.statement.IterationStatementTree;
import org.sonar.plugins.javascript.api.tree.statement.WhileStatementTree;
import org.sonar.plugins.javascript.api.visitors.DoubleDispatchVisitor;
import org.sonar.plugins.javascript.api.visitors.DoubleDispatchVisitorCheck;
import org.sonar.plugins.javascript.api.visitors.IssueLocation;

@Rule(key = "S2189")
public class LoopsShouldNotBeInfiniteCheck extends DoubleDispatchVisitorCheck {
public class LoopsShouldNotBeInfiniteCheck extends SeCheck {


private FileLoops fileLoops;
private Set<Tree> alwaysTrueConditions = new HashSet<>();

@Override
public void visitForStatement(ForStatementTree tree) {
visitLoop(tree, tree.condition());
super.visitForStatement(tree);
public void startOfFile(ScriptTree scriptTree) {
alwaysTrueConditions.clear();
fileLoops = FileLoops.create(scriptTree);
}

@Override
public void visitWhileStatement(WhileStatementTree tree) {
visitLoop(tree, tree.condition());
super.visitWhileStatement(tree);
public void checkConditions(Map<Tree, Collection<Constraint>> conditions) {
alwaysTrueConditions.addAll(conditions.entrySet().stream()
.filter(treeCollectionEntry -> alwaysTrue(treeCollectionEntry.getValue()))
.map(Map.Entry::getKey)
.collect(Collectors.toSet()));
}

@Override
public void visitDoWhileStatement(DoWhileStatementTree tree) {
visitLoop(tree, tree.condition());
super.visitDoWhileStatement(tree);
public void endOfFile(ScriptTree scriptTree) {
fileLoops.loopsAndConditions.entrySet().stream()
.filter(entry -> isInfiniteLoop(entry.getKey(), (JavaScriptTree) entry.getValue()))
.forEach(entry -> addIssue(entry.getKey()));
}

private void addIssue(IterationStatementTree loop) {
loop.accept(new LoopIssueCreator());
}

/**
* FileLoops is a tree visitor that collects a map of loops and corresponding conditions
*/
private static class FileLoops extends DoubleDispatchVisitor {

private Map<IterationStatementTree, ExpressionTree> loopsAndConditions = new HashMap<>();

static FileLoops create(ScriptTree scriptTree) {
FileLoops fileLoops = new FileLoops();
scriptTree.accept(fileLoops);
return fileLoops;
}

@Override
public void visitForStatement(ForStatementTree tree) {
visitLoop(tree, tree.condition());
super.visitForStatement(tree);
}

@Override
public void visitWhileStatement(WhileStatementTree tree) {
visitLoop(tree, tree.condition());
super.visitWhileStatement(tree);
}

@Override
public void visitDoWhileStatement(DoWhileStatementTree tree) {
visitLoop(tree, tree.condition());
super.visitDoWhileStatement(tree);
}

private void visitLoop(IterationStatementTree tree, @Nullable ExpressionTree condition) {
loopsAndConditions.put(tree, condition);
}
}

private void visitLoop(IterationStatementTree tree, @Nullable Tree condition) {
if (isInfiniteLoop(tree, (JavaScriptTree) condition)) {
tree.accept(new LoopIssueCreator());
private static boolean alwaysTrue(Collection<Constraint> results) {
if (results.size() == 1) {
Constraint constraint = results.iterator().next();
return Constraint.TRUTHY.equals(constraint);
}

return false;
}

private static boolean isInfiniteLoop(IterationStatementTree tree, @Nullable JavaScriptTree condition) {
private boolean isInfiniteLoop(IterationStatementTree tree, @Nullable JavaScriptTree condition) {
if (isNeverExecutedLoop(condition)) {
return false;
}
Expand All @@ -81,7 +136,7 @@ private static boolean isInfiniteLoop(IterationStatementTree tree, @Nullable Jav
if (isBrokenLoop(condition, tree, flowGraph)) {
return false;
}
return condition == null || !conditionIsUpdated(condition, (JavaScriptTree) tree, treesOfFlowGraph);
return condition == null || !conditionIsUpdated(condition, (JavaScriptTree) tree, treesOfFlowGraph) || alwaysTrueConditions.contains(condition);
}

private static boolean isBrokenLoop(@Nullable JavaScriptTree condition, IterationStatementTree loopTree, ControlFlowGraph flowGraph) {
Expand Down Expand Up @@ -218,10 +273,11 @@ public void visitForStatement(ForStatementTree tree) {
}
}

private void createIssue(Tree keyword, Tree condition) {
private void createIssue(Tree keyword, ExpressionTree condition) {
LoopsShouldNotBeInfiniteCheck.this.addIssue(keyword, "Correct this loop's end condition as to not be invariant.")
.secondary(new IssueLocation(condition, null));
}

}

}
62 changes: 61 additions & 1 deletion javascript-checks/src/test/resources/checks/loopsShouldNotBeInfinite.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ function condition_is_static() {

function outer_loop_condition_changes_but_inner_loop_has_no_condition() {
var i = -2;
while(i < 0) {
while(i < 0) { // Noncompliant?
for(;;) { // Noncompliant
i++;
}
Expand Down Expand Up @@ -205,3 +205,63 @@ function * generator() {
yield;
}
}


function always_true() {

var trueValue = true;

while(trueValue) { // Noncompliant
trueValue = 42;
}
}


function do_while() {

var trueValue = true;

do { // Noncompliant
trueValue = true;
} while (trueValue);
}


function loop_broken() {

var trueValue = true;

while(trueValue) { // OK
trueValue = true;
if (condition) {
break;
}
}
}


function value_is_updated_to_false() {

var trueValue = true;

while(trueValue) { // OK
trueValue = false;
}
}


function numeric_comparison() {

var i = 0;
while (i < 10) { // OK
i--;
if (condition) {
break;
}
}

var i = 0;
while (i < 10) { // Noncompliant
i--;
}
}

0 comments on commit e06415a

Please sign in to comment.