github · semmle-qlci · Oct 30, 2018 · Oct 18, 2018 · Oct 12, 2018 · Oct 15, 2018
diff --git a/javascript/ql/src/Declarations/DeadStoreOfLocal.ql b/javascript/ql/src/Declarations/DeadStoreOfLocal.ql
@@ -25,25 +25,6 @@ predicate deadStoreOfLocal(VarDef vd, PurelyLocalVariable v) {
   not exists (SsaExplicitDefinition ssa | ssa.defines(vd, v))
 }
 
-/**
- * Holds if `e` is an expression evaluating to `null` or `undefined`.
- *
- * This includes not only direct references to `null` and `undefined`, but
- * also `void` expressions and assignments of the form `x = rhs`, where `rhs`
- * is itself an expression evaluating to `null` or `undefined`.
- */
-predicate isNullOrUndef(Expr e) {
-  exists (Expr inner |
-    inner = e.stripParens() |
-    // `null` or `undefined`
-    inner instanceof NullLiteral or
-    inner.(VarAccess).getName() = "undefined" or
-    inner instanceof VoidExpr or
-    // recursive case to catch multi-assignments of the form `x = y = null`
-    isNullOrUndef(inner.(AssignExpr).getRhs())
-  )
-}
-
 /**
  * Holds if `e` is an expression that may be used as a default initial value,
  * such as `0` or `-1`, or an empty object or array literal.
@@ -57,9 +38,7 @@ predicate isDefaultInit(Expr e) {
   // initialising to an empty array or object literal, even if unnecessary,
   // can convey useful type information to the reader
   e.(ArrayExpr).getSize() = 0 or
-  e.(ObjectExpr).getNumProperty() = 0 or
-  // recursive case
-  isDefaultInit(e.(AssignExpr).getRhs())
+  e.(ObjectExpr).getNumProperty() = 0
 }
 
 from VarDef dead, PurelyLocalVariable v  // captured variables may be read by closures, so don't flag them
@@ -77,9 +56,9 @@ where deadStoreOfLocal(dead, v) and
         not fd = outer.getBody().(BlockStmt).getAStmt()
       ) and
       // don't flag overwrites with `null` or `undefined`
-      not isNullOrUndef(dead.getSource()) and
+      not SyntacticConstants::isNullOrUndefined(dead.getSource()) and
       // don't flag default inits that are later overwritten
-      not (isDefaultInit(dead.getSource()) and dead.isOverwritten(v)) and
+      not (isDefaultInit(dead.getSource().(Expr).getUnderlyingValue()) and dead.isOverwritten(v)) and
       // don't flag assignments in externs
       not dead.(ASTNode).inExternsFile() and
       // don't flag exported variables

diff --git a/javascript/ql/src/semmle/javascript/Constants.qll b/javascript/ql/src/semmle/javascript/Constants.qll
@@ -127,14 +127,32 @@ module SyntacticConstants {
   class WrappedConstant extends SyntacticConstant {
 
     WrappedConstant() {
-      stripParens() instanceof SyntacticConstant or
-      this.(SeqExpr).getLastOperand() instanceof SyntacticConstant or
-      this.(TypeAssertion).getExpression() instanceof SyntacticConstant or
-      this.(Assignment).getRhs() instanceof SyntacticConstant
+      getUnderlyingValue() instanceof SyntacticConstant
     }
 
   }
 
+  /**
+   * Holds if `c` evaluates to `undefined`.
+   */
+  predicate isUndefined(SyntacticConstant c) {
+    c.getUnderlyingValue() instanceof UndefinedConstant
+  }
+
+  /**
+   * Holds if `c` evaluates to `null`.
+   */
+  predicate isNull(SyntacticConstant c) {
+    c.getUnderlyingValue() instanceof NullConstant
+  }
+
+  /**
+   * Holds if `c` evaluates to `null` or `undefined`.
+   */
+  predicate isNullOrUndefined(SyntacticConstant c) {
+    isUndefined(c) or isNull(c)
+  }
+
 }
 
 /**

diff --git a/javascript/ql/src/semmle/javascript/Expr.qll b/javascript/ql/src/semmle/javascript/Expr.qll
@@ -45,8 +45,45 @@ class ExprOrType extends @exprortype, Documentable {
     )
   }
 
-  /** Gets this expression or type, with any surrounding parentheses removed. */
+  /**
+   * Gets this expression or type, with any surrounding parentheses removed.
+   *
+   * Also see `getUnderlyingValue` and `getUnderlyingReference`.
+   */
   ExprOrType stripParens() { result = this }
+
+  /**
+   * Gets the innermost reference that this expression evaluates to, if any.
+   *
+   * Examples:
+   *
+   * - a variable or property access: the access itself.
+   * - a parenthesized expression `(e)`: the underlying reference of `e`.
+   * - a TypeScript type assertion `e as T`: the underlying reference of `e`.
+   *
+   * Also see `getUnderlyingValue` and `stripParens`.
+   */
+  Expr getUnderlyingReference() {
+    none()
+  }
+
+  /**
+   * Gets the innermost expression that this expression evaluates to.
+   *
+   * Examples:
+   *
+   * - a parenthesised expression `(e)`: the underlying value of `e`.
+   * - a sequence expression `e1, e2`: the underlying value of `e2`.
+   * - an assignment expression `v = e`: the underlying value of `e`.
+   * - a TypeScript type assertion `e as T`: the underlying value of `e`.
+   * - any other expression: the expression itself.
+   *
+   * Also see `getUnderlyingReference` and `stripParens`.
+   */
+  Expr getUnderlyingValue() {
+    result = this
+  }
+
 }
 
 /** An expression. */
@@ -210,6 +247,15 @@ class ParExpr extends @parexpr, Expr {
   override predicate isImpure() {
     getExpression().isImpure()
   }
+
+  override Expr getUnderlyingValue() {
+    result = getExpression().getUnderlyingValue()
+  }
+
+  override Expr getUnderlyingReference() {
+    result = getExpression().getUnderlyingReference()
+  }
+
 }
 
 /** A `null` literal. */
@@ -617,6 +663,11 @@ class SeqExpr extends @seqexpr, Expr {
   override string getStringValue() {
     result = getLastOperand().getStringValue()
   }
+
+  override Expr getUnderlyingValue() {
+    result = getLastOperand().getUnderlyingValue()
+  }
+
 }
 
 /** A conditional expression. */
@@ -862,6 +913,11 @@ class PropAccess extends @propaccess, Expr {
   override ControlFlowNode getFirstControlFlowNode() {
     result = getBase().getFirstControlFlowNode()
   }
+
+  override Expr getUnderlyingReference() {
+    result = this
+  }
+
 }
 
 /** A dot expression. */
@@ -1284,10 +1340,17 @@ class Assignment extends @assignment, Expr {
   override ControlFlowNode getFirstControlFlowNode() {
     result = getLhs().getFirstControlFlowNode()
   }
+
 }
 
 /** A simple assignment expression. */
-class AssignExpr extends @assignexpr, Assignment {}
+class AssignExpr extends @assignexpr, Assignment {
+
+  override Expr getUnderlyingValue() {
+    result = getRhs().getUnderlyingValue()
+  }
+
+}
 
 /** A compound assign expression. */
 abstract class CompoundAssignExpr extends Assignment {}

diff --git a/javascript/ql/src/semmle/javascript/TypeScript.qll b/javascript/ql/src/semmle/javascript/TypeScript.qll
@@ -1315,6 +1315,15 @@ class TypeAssertion extends Expr, @typeassertion {
   override ControlFlowNode getFirstControlFlowNode() {
     result = getExpression().getFirstControlFlowNode()
   }
+
+  override Expr getUnderlyingValue() {
+    result = getExpression().getUnderlyingValue()
+  }
+
+  override Expr getUnderlyingReference() {
+    result = getExpression().getUnderlyingReference()
+  }
+
 }
 
 /**

diff --git a/javascript/ql/src/semmle/javascript/Variables.qll b/javascript/ql/src/semmle/javascript/Variables.qll
@@ -298,6 +298,11 @@ class VarRef extends @varref, Identifier, BindingPattern, LexicalRef {
   override VarRef getABindingVarRef() { result = this }
 
   override predicate isImpure() { none() }
+
+  override Expr getUnderlyingReference() {
+    result = this
+  }
+
 }
 
 /** An identifier that refers to a variable in a non-declaring position. */

diff --git a/javascript/ql/test/library-tests/Constants/Constants.expected b/javascript/ql/test/library-tests/Constants/Constants.expected
@@ -58,7 +58,6 @@
 | tst.js:47:5:47:5 | 1 |
 | tst.js:48:1:48:7 | x.p = 1 |
 | tst.js:48:7:48:7 | 1 |
-| tst.js:49:1:49:6 | x += 1 |
 | tst.js:49:6:49:6 | 1 |
 | tst.ts:1:13:1:21 | <number>1 |
 | tst.ts:1:21:1:21 | 1 |
diff --git a/javascript/ql/test/query-tests/Declarations/ArgumentsRedefined/ArgumentsRedefined.expected b/javascript/ql/test/query-tests/Declarations/ArgumentsRedefined/ArgumentsRedefined.expected
@@ -1,2 +1,4 @@
 | tst.js:3:5:3:13 | arguments | Redefinition of arguments. |
 | tst.js:7:7:7:15 | arguments | Redefinition of arguments. |
+| tst.js:11:11:11:19 | arguments | Redefinition of arguments. |
+| tst.js:12:11:12:19 | arguments | Redefinition of arguments. |
diff --git a/javascript/ql/test/query-tests/Declarations/ArgumentsRedefined/tst.js b/javascript/ql/test/query-tests/Declarations/ArgumentsRedefined/tst.js
@@ -6,3 +6,8 @@ function f() {
 function g(x, y) {
   var arguments = [y, x]; // NOT OK
 }
+
+(function (){
+    for ([arguments] of o);
+    for ([arguments = 4] of o);
+});
diff --git a/javascript/ql/test/query-tests/Declarations/AssignmentToConst/AssignmentToConst.expected b/javascript/ql/test/query-tests/Declarations/AssignmentToConst/AssignmentToConst.expected
@@ -4,3 +4,4 @@
 | tst.js:7:1:7:6 | y = 23 | Assignment to variable y, which is $@ constant. | tst.js:1:1:1:21 | const x ... y = 42; | declared |
 | tst.js:10:5:10:10 | y = -1 | Assignment to variable y, which is $@ constant. | tst.js:1:1:1:21 | const x ... y = 42; | declared |
 | tst.js:13:1:13:3 | ++x | Assignment to variable x, which is $@ constant. | tst.js:1:1:1:21 | const x ... y = 42; | declared |
+| tst.js:25:10:25:14 | [ c ] | Assignment to variable c, which is $@ constant. | tst.js:24:5:24:19 | const c = null; | declared |
diff --git a/javascript/ql/test/query-tests/Declarations/AssignmentToConst/tst.js b/javascript/ql/test/query-tests/Declarations/AssignmentToConst/tst.js
@@ -18,4 +18,9 @@ var z = 56;
 z = 72;
 
 // OK
-const s = "hi";
+const s = "hi";
+
+(function (){
+    const c = null;
+    for ([ c ] of o);
+});
diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst.js b/javascript/ql/test/query-tests/Declarations/DeadStoreOfLocal/tst.js
@@ -153,3 +153,7 @@ function v() {
 	z2 = 42;
 	return x + y + z1 + z2;
 });
+
+(function() {
+	for (var a = (x, -1) in v = a, o);
+});
diff --git a/javascript/ql/test/query-tests/Declarations/MissingVarDecl/MissingVarDecl.expected b/javascript/ql/test/query-tests/Declarations/MissingVarDecl/MissingVarDecl.expected
@@ -4,5 +4,6 @@
 | test.js:54:10:54:10 | z | Variable z is used like a local variable, but is missing a declaration. |
 | test.js:60:6:60:6 | y | Variable y is used like a local variable, but is missing a declaration. |
 | test.js:66:2:66:2 | z | Variable z is used like a local variable, but is missing a declaration. |
+| test.js:72:9:72:20 | unresolvable | Variable unresolvable is used like a local variable, but is missing a declaration. |
 | tst3.js:7:10:7:10 | x | Variable x is used like a local variable, but is missing a declaration. |
 | tst3.js:7:16:7:19 | rest | Variable rest is used like a local variable, but is missing a declaration. |
diff --git a/javascript/ql/test/query-tests/Declarations/MissingVarDecl/test.js b/javascript/ql/test/query-tests/Declarations/MissingVarDecl/test.js
@@ -66,4 +66,10 @@ function r() {
 	z = {};
 	for (var p in z)
 		;
-}
+}
+
+(function() {
+	for ([ unresolvable ] of o) {
+		unresolvable;
+	}
+});
diff --git a/javascript/ql/test/query-tests/Expressions/ImplicitOperandConversion/tst.js b/javascript/ql/test/query-tests/Expressions/ImplicitOperandConversion/tst.js
@@ -87,4 +87,11 @@ function l() {
 
 1n + 1; // NOT OK, but not currently flagged
 
+(function(){
+    let sum = 0;
+    for ({value} of async(o)) {
+        sum += value;
+    }
+});
+
 // semmle-extractor-options: --experimental
diff --git a/javascript/ql/test/query-tests/Expressions/SuspiciousPropAccess/tst.js b/javascript/ql/test/query-tests/Expressions/SuspiciousPropAccess/tst.js
@@ -15,3 +15,15 @@
   if (x !== undefined)
     x.p;
 })();
+
+(function() {
+    var v0
+    for({ v0 } of o) {
+        v0.p;
+    }
+
+    var v1;
+    for({ v1 = x } of o) {
+        v1.p;
+    }
+});
diff --git a/javascript/ql/test/query-tests/Statements/SuspiciousUnusedLoopIterationVariable/tst.js b/javascript/ql/test/query-tests/Statements/SuspiciousUnusedLoopIterationVariable/tst.js
@@ -127,3 +127,9 @@ function countOccurrencesDead(xs, p) {
 			++count;
 	return count;
 }
+
+(function(a) {
+	for([a] of o) {
+		a;
+	}
+});
diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected
@@ -15,6 +15,7 @@
 | UselessConditional.js:65:5:65:5 | x | Variable 'x' always evaluates to true here. |
 | UselessConditional.js:76:13:76:13 | x | Variable 'x' always evaluates to true here. |
 | UselessConditional.js:82:13:82:13 | x | Variable 'x' always evaluates to true here. |
+| UselessConditional.js:89:10:89:16 | x, true | This expression always evaluates to true. |
 | UselessConditionalGood.js:58:12:58:13 | x2 | Variable 'x2' always evaluates to false here. |
 | UselessConditionalGood.js:69:12:69:13 | xy | Variable 'xy' always evaluates to false here. |
 | UselessConditionalGood.js:85:12:85:13 | xy | Variable 'xy' always evaluates to false here. |

diff --git a/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js b/javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.js
@@ -84,4 +84,9 @@ async function awaitFlow(){
     }
     f3(true);
 });
+
+(function() {
+    if ((x, true));
+});
+
 // semmle-extractor-options: --experimental