Merge pull request #440 from asger-semmle/range-analysis

JS: Range analysis for dead code detection

Author: Max Schaefer

change-notes/1.20/analysis-javascript.md

@@ -4,9 +4,10 @@
 
 ## New queries
 
-| **Query** | **Tags** | **Purpose** |
-|-----------|----------|-------------|
-|           |          |             |
+| **Query**                                     | **Tags**                                             | **Purpose**                                                                                                                                                                 |
+|-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| Useless comparison test | correctness | Highlights code that is unreachable due to a numeric comparison that is always true or always false. |
+
 
 ## Changes to existing queries
 

javascript/config/suites/javascript/correctness-more

@@ -15,3 +15,4 @@
 + semmlecode-javascript-queries/Statements/ReturnOutsideFunction.ql: /Correctness/Statements
 + semmlecode-javascript-queries/Statements/SuspiciousUnusedLoopIterationVariable.ql: /Correctness/Statements
 + semmlecode-javascript-queries/Statements/UselessConditional.ql: /Correctness/Statements
++ semmlecode-javascript-queries/Statements/UselessComparisonTest.ql: /Correctness/Statements

javascript/ql/src/Statements/UselessComparisonTest.qhelp

@@ -0,0 +1,47 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+If a condition always evaluates to true or always evaluates to false, this often indicates
+incomplete code or a latent bug, and it should be examined carefully.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Examine the surrounding code to determine why the condition is redundant. If it is no
+longer needed, remove it.
+</p>
+
+<p>
+If the check is needed to guard against <code>NaN</code> values, insert a comment explaning the possibility of <code>NaN</code>.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+The following example finds the index of an element in a given slice of the array:
+</p>
+
+<sample src="examples/UselessComparisonTest.js" />
+
+<p>
+The condition <code>i &lt; end</code> at the end is always false, however. The code can be clarified if the
+redundant condition is removed:
+</p>
+
+<sample src="examples/UselessComparisonTestGood.js" />
+
+</example>
+<references>
+
+
+<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Glossary/Truthy">Truthy</a>,
+<a href="https://developer.mozilla.org/en-US/docs/Glossary/Falsy">Falsy</a>.</li>
+
+</references>
+</qhelp>

javascript/ql/src/Statements/UselessComparisonTest.ql

@@ -0,0 +1,60 @@
+/**
+ * @name Useless comparison test
+ * @description A comparison that always evaluates to true or always evaluates to false may
+ *              indicate faulty logic and dead code.
+ * @kind problem
+ * @problem.severity warning
+ * @id js/useless-comparison-test
+ * @tags correctness
+ * @precision high
+ */
+
+import javascript
+
+/**
+ * Holds if there are any contradictory guard nodes in `container`.
+ *
+ * We use this to restrict reachability analysis to a small set of containers.
+ */
+predicate hasContradictoryGuardNodes(StmtContainer container) {
+  exists (ConditionGuardNode guard |
+    RangeAnalysis::isContradictoryGuardNode(guard) and
+    container = guard.getContainer())
+}
+
+/**
+ * Holds if `block` is reachable and is in a container with contradictory guard nodes.
+ */
+predicate isReachable(BasicBlock block) {
+  exists (StmtContainer container |
+    hasContradictoryGuardNodes(container) and
+    block = container.getEntryBB())
+  or
+  isReachable(block.getAPredecessor()) and
+  not RangeAnalysis::isContradictoryGuardNode(block.getANode())
+}
+
+/**
+ * Holds if `block` is unreachable, but could be reached if `guard` was not contradictory.
+ */
+predicate isBlockedByContradictoryGuardNodes(BasicBlock block, ConditionGuardNode guard) {
+  RangeAnalysis::isContradictoryGuardNode(guard) and
+  isReachable(block.getAPredecessor()) and // the guard itself is reachable
+  block = guard.getBasicBlock()
+  or
+  isBlockedByContradictoryGuardNodes(block.getAPredecessor(), guard) and
+  not isReachable(block)
+}
+
+/**
+ * Holds if the given guard node is contradictory and causes an expression or statement to be unreachable.
+ */
+predicate isGuardNodeWithDeadCode(ConditionGuardNode guard) {
+  exists (BasicBlock block |
+    isBlockedByContradictoryGuardNodes(block, guard) and
+    block.getANode() instanceof ExprOrStmt)
+}
+
+from ConditionGuardNode guard
+where isGuardNodeWithDeadCode(guard)
+select guard.getTest(), "The condition '" + guard.getTest() + "' is always " + guard.getOutcome().booleanNot() + "."

javascript/ql/src/Statements/examples/UselessComparisonTest.js

@@ -0,0 +1,12 @@
+function findValue(values, x, start, end) {
+  let i;
+  for (i = start; i < end; ++i) {
+    if (values[i] === x) {
+        return i;
+    }
+  }
+  if (i < end) {
+    return i;
+  }
+  return -1;
+}

javascript/ql/src/Statements/examples/UselessComparisonTestGood.js

@@ -0,0 +1,8 @@
+function findValue(values, x, start, end) {
+  for (let i = start; i < end; ++i) {
+    if (values[i] === x) {
+        return i;
+    }
+  }
+  return -1;
+}

javascript/ql/src/javascript.qll

@@ -36,6 +36,7 @@ import semmle.javascript.NPM
 import semmle.javascript.Paths
 import semmle.javascript.Promises
 import semmle.javascript.CanonicalNames
+import semmle.javascript.RangeAnalysis
 import semmle.javascript.Regexp
 import semmle.javascript.SSA
 import semmle.javascript.StandardLibrary