Merge pull request #396 from esben-semmle/js/unconditional-property-override

JS: add query: js/unconditional-property-override

javascript/ql/src/Declarations/DeadStore.qhelp

@@ -0,0 +1,49 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+A value is assigned to a variable or property, but either that location is never read
+later on, or its value is always overwritten before being read. This means
+that the original assignment has no effect, and could indicate a logic error or
+incomplete code.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Ensure that you check the control and data flow in the method carefully.
+If a value is really not needed, consider omitting the assignment. Be careful,
+though: if the right-hand side has a side-effect (like performing a method call),
+it is important to keep this to preserve the overall behavior.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+In the following example, the return value of the call to <code>send</code> on line 2
+is assigned to the local variable <code>result</code>, but then never used.
+</p>
+
+<sample src="examples/DeadStoreOfLocal.js" />
+
+<p>
+Assuming that <code>send</code> returns a status code indicating whether the operation
+succeeded or not, the value of <code>result</code> should be checked, perhaps like this:
+</p>
+
+<sample src="examples/DeadStoreOfLocalGood.js" />
+
+</example>
+<references>
+
+
+<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Dead_store">Dead store</a>.</li>
+
+
+
+</references>
+</qhelp>

javascript/ql/src/Declarations/DeadStore.qll

@@ -0,0 +1,22 @@
+/**
+ * Provides classes and predicates for reasoning about dead stores.
+ */
+
+import javascript
+
+/**
+ * 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.
+ */
+predicate isDefaultInit(Expr e) {
+  // primitive default values: zero, false, empty string, and (integer) -1
+  e.(NumberLiteral).getValue().toFloat() = 0.0 or
+  e.(NegExpr).getOperand().(NumberLiteral).getValue() = "1" or
+  e.(ConstantString).getStringValue() = "" or
+  e.(BooleanLiteral).getValue() = "false" or
+  // 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
+  SyntacticConstants::isNullOrUndefined(e)
+}

javascript/ql/src/Declarations/DeadStoreOfLocal.qhelp

@@ -1,49 +1,9 @@
 <!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+
 <qhelp>
-<overview>
-<p>
-A value is assigned to a local variable, but either that variable is never read
-later on, or its value is always overwritten before being read. This means
-that the original assignment has no effect, and could indicate a logic error or
-incomplete code.
-</p>
 
-</overview>
-<recommendation>
+	<include src="DeadStore.qhelp" />
 
-<p>
-Ensure that you check the control and data flow in the method carefully.
-If a value is really not needed, consider omitting the assignment. Be careful,
-though: if the right-hand side has a side-effect (like performing a method call),
-it is important to keep this to preserve the overall behavior.
-</p>
-
-</recommendation>
-<example>
-
-<p>
-In the following example, the return value of the call to <code>send</code> on line 2
-is assigned to the local variable <code>result</code>, but then never used.
-</p>
-
-<sample src="examples/DeadStoreOfLocal.js" />
-
-<p>
-Assuming that <code>send</code> returns a status code indicating whether the operation
-succeeded or not, the value of <code>result</code> should be checked, perhaps like this:
-</p>
-
-<sample src="examples/DeadStoreOfLocalGood.js" />
-
-</example>
-<references>
-
-
-<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Dead_store">Dead store</a>.</li>
-
-
-
-</references>
 </qhelp>

javascript/ql/src/Declarations/DeadStoreOfLocal.ql

@@ -11,6 +11,7 @@
  */
 
 import javascript
+import DeadStore
 
 /**
  * Holds if `vd` is a definition of variable `v` that is dead, that is,
@@ -25,22 +26,6 @@ predicate deadStoreOfLocal(VarDef vd, PurelyLocalVariable v) {
   not exists (SsaExplicitDefinition ssa | ssa.defines(vd, v))
 }
 
-/**
- * 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.
- */
-predicate isDefaultInit(Expr e) {
-  // primitive default values: zero, false, empty string, and (integer) -1
-  e.(NumberLiteral).getValue().toFloat() = 0.0 or
-  e.(NegExpr).getOperand().(NumberLiteral).getValue() = "1" or
-  e.(ConstantString).getStringValue() = "" or
-  e.(BooleanLiteral).getValue() = "false" or
-  // 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
-}
-
 from VarDef dead, PurelyLocalVariable v  // captured variables may be read by closures, so don't flag them
 where deadStoreOfLocal(dead, v) and
       // the variable should be accessed somewhere; otherwise it will be flagged by UnusedVariable

javascript/ql/src/Declarations/DeadStoreOfProperty.qhelp

@@ -0,0 +1,9 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+
+<qhelp>
+
+	<include src="DeadStore.qhelp" />
+
+</qhelp>

javascript/ql/src/Declarations/DeadStoreOfProperty.ql

@@ -0,0 +1,142 @@
+/**
+ * @name Useless assignment to property
+ * @description An assignment to a property whose value is always overwritten has no effect.
+ * @kind problem
+ * @problem.severity warning
+ * @id js/useless-assignment-to-property
+ * @tags maintainability
+ * @precision high
+ */
+
+import javascript
+import Expressions.DOMProperties
+import DeadStore
+
+/**
+ * Holds if `write` writes to property `name` of `base`, and `base` is the only base object of `write`.
+ */
+predicate unambiguousPropWrite(DataFlow::SourceNode base, string name, DataFlow::PropWrite write) {
+  write = base.getAPropertyWrite(name) and
+  not exists (DataFlow::SourceNode otherBase |
+    otherBase != base and
+    write = otherBase.getAPropertyWrite(name)
+  )
+}
+
+/**
+ * Holds if `assign1` and `assign2` both assign property `name` of the same object, and `assign2` post-dominates `assign1`.
+ */
+predicate postDominatedPropWrite(string name, DataFlow::PropWrite assign1, DataFlow::PropWrite assign2) {
+  exists (ControlFlowNode write1, ControlFlowNode write2, DataFlow::SourceNode base, ReachableBasicBlock block1, ReachableBasicBlock block2 |
+    write1 = assign1.getWriteNode() and
+    write2 = assign2.getWriteNode() and
+    block1 = write1.getBasicBlock() and
+    block2 = write2.getBasicBlock() and
+    unambiguousPropWrite(base, name, assign1) and
+    unambiguousPropWrite(base, name, assign2) and
+    block2.postDominates(block1) and
+    (block1 = block2 implies
+      exists (int i1, int i2 |
+        write1 = block1.getNode(i1) and
+        write2 = block2.getNode(i2) and
+        i1 < i2
+      )
+    )
+  )
+}
+
+/**
+ * Holds if `e` may access a property named `name`.
+ */
+bindingset[name]
+predicate maybeAccessesProperty(Expr e, string name) {
+  (e.(PropAccess).getPropertyName() = name and e instanceof RValue) or
+  // conservatively reject all side-effects
+  e.isImpure()
+}
+
+/**
+ * Holds if `assign1` and `assign2` both assign property `name`, but `assign1` is dead because of `assign2`.
+ */
+predicate isDeadAssignment(string name, DataFlow::PropWrite assign1, DataFlow::PropWrite assign2) {
+  postDominatedPropWrite(name, assign1, assign2) and
+  noPropAccessBetween(name, assign1, assign2) and
+  not isDOMProperty(name)
+}
+
+/**
+ * Holds if `assign` assigns a property `name` that may be accessed somewhere else in the same block,
+ * `after` indicates if the access happens before or after the node for `assign`.
+ */
+bindingset[name]
+predicate maybeAccessesAssignedPropInBlock(string name, DataFlow::PropWrite assign, boolean after) {
+  exists (ControlFlowNode write, ReachableBasicBlock block, int i, int j, Expr e |
+    write = assign.getWriteNode() and
+    block = assign.getBasicBlock() and
+    write = block.getNode(i) and
+    e = block.getNode(j) and
+    maybeAccessesProperty(e, name) |
+    after = true and i < j
+    or
+    after = false and j < i
+  )
+}
+
+/**
+ * Holds if `assign1` and `assign2` both assign property `name`, and the assigned property is not accessed between the two assignments.
+ */
+bindingset[name]
+predicate noPropAccessBetween(string name, DataFlow::PropWrite assign1, DataFlow::PropWrite assign2) {
+  exists (ControlFlowNode write1, ControlFlowNode write2, ReachableBasicBlock block1, ReachableBasicBlock block2 |
+    write1 = assign1.getWriteNode() and
+    write2 = assign2.getWriteNode() and
+    write1.getBasicBlock() = block1 and
+    write2.getBasicBlock() = block2 and
+    if block1 = block2 then
+      // same block: check for access between
+      not exists (int i1, Expr mid, int i2 |
+        assign1.getWriteNode() = block1.getNode(i1) and
+        assign2.getWriteNode() = block2.getNode(i2) and
+        mid = block1.getNode([i1+1..i2-1]) and
+        maybeAccessesProperty(mid, name)
+      )
+    else
+      // other block:
+      not (
+        // check for an access after the first write node
+        maybeAccessesAssignedPropInBlock(name, assign1, true) or
+        // check for an access between the two write blocks
+        exists (ReachableBasicBlock mid |
+          block1.getASuccessor+() = mid and
+          mid.getASuccessor+() = block2 |
+          maybeAccessesProperty(mid.getANode(), name)
+        ) or
+        // check for an access before the second write node
+        maybeAccessesAssignedPropInBlock(name, assign2, false)
+      )
+  )
+}
+
+from string name, DataFlow::PropWrite assign1, DataFlow::PropWrite assign2
+where isDeadAssignment(name, assign1, assign2) and
+      // whitelist
+      not (
+        // Google Closure Compiler pattern: `o.p = o['p'] = v`
+        exists (PropAccess p1, PropAccess p2 |
+          p1 = assign1.getAstNode() and
+          p2 = assign2.getAstNode() |
+          p1 instanceof DotExpr and p2 instanceof IndexExpr
+          or
+          p2 instanceof DotExpr and p1 instanceof IndexExpr
+        )
+        or
+        // don't flag overwrites for default values
+        isDefaultInit(assign1.getRhs().asExpr().getUnderlyingValue())
+        or
+        // don't flag assignments in externs
+        assign1.getAstNode().inExternsFile()
+        or
+        // exclude result from js/overwritten-property
+        assign2.getBase() instanceof DataFlow::ObjectLiteralNode
+      )
+select assign1.getWriteNode(), "This write to property '" + name + "' is useless, since $@ always overrides it.", assign2.getWriteNode(), "another property write"

javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll

@@ -459,6 +459,11 @@ module DataFlow {
       prop = getPropertyName() and
       rhs = getRhs()
     }
+
+    /**
+     * Gets the node where the property write happens in the control flow graph.
+     */
+    abstract ControlFlowNode getWriteNode();
   }
 
   /**
@@ -484,6 +489,10 @@ module DataFlow {
     override Node getRhs() {
       result = valueNode(astNode.(LValue).getRhs())
     }
+
+    override ControlFlowNode getWriteNode() {
+      result = astNode.(LValue).getDefNode()
+    }
   }
 
   /**
@@ -508,6 +517,10 @@ module DataFlow {
     override Node getRhs() {
       result = valueNode(prop.(ValueProperty).getInit())
     }
+
+    override ControlFlowNode getWriteNode() {
+      result = prop
+    }
   }
 
   /**
@@ -537,6 +550,10 @@ module DataFlow {
         propdesc.hasPropertyWrite("value", result)
       )
     }
+
+    override ControlFlowNode getWriteNode() {
+      result = odp.getAstNode()
+    }
   }
 
   /**
@@ -563,6 +580,10 @@ module DataFlow {
       not prop instanceof AccessorMethodDefinition and
       result = valueNode(prop.getInit())
     }
+
+    override ControlFlowNode getWriteNode() {
+      result = prop
+    }
   }
 
   /**
@@ -587,6 +608,10 @@ module DataFlow {
     override Node getRhs() {
       result = valueNode(prop.getValue())
     }
+
+    override ControlFlowNode getWriteNode() {
+      result = prop
+    }
   }
 
   /**
@@ -869,6 +894,11 @@ module DataFlow {
     override Node getBase() {
       result = valueNode(arr)
     }
+
+    override ControlFlowNode getWriteNode() {
+      result = arr
+    }
+
   }
 
   /**

javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.expected

@@ -0,0 +1,13 @@
+| real-world-examples.js:5:4:5:11 | o.p = 42 | This write to property 'p' is useless, since $@ always overrides it. | real-world-examples.js:10:2:10:9 | o.p = 42 | another property write |
+| real-world-examples.js:15:9:15:18 | o.p1 += 42 | This write to property 'p1' is useless, since $@ always overrides it. | real-world-examples.js:15:2:15:18 | o.p1 = o.p1 += 42 | another property write |
+| real-world-examples.js:16:11:16:20 | o.p2 *= 42 | This write to property 'p2' is useless, since $@ always overrides it. | real-world-examples.js:16:2:16:21 | o.p2 -= (o.p2 *= 42) | another property write |
+| real-world-examples.js:29:5:29:12 | o.p = 42 | This write to property 'p' is useless, since $@ always overrides it. | real-world-examples.js:32:3:32:10 | o.p = 42 | another property write |
+| real-world-examples.js:38:15:38:24 | o.p = f3() | This write to property 'p' is useless, since $@ always overrides it. | real-world-examples.js:38:2:38:31 | o.p = f ...  : f4() | another property write |
+| tst.js:3:5:3:16 | o.pure1 = 42 | This write to property 'pure1' is useless, since $@ always overrides it. | tst.js:4:5:4:16 | o.pure1 = 42 | another property write |
+| tst.js:6:5:6:16 | o.pure2 = 42 | This write to property 'pure2' is useless, since $@ always overrides it. | tst.js:7:5:7:16 | o.pure2 = 43 | another property write |
+| tst.js:13:5:13:16 | o.pure4 = 42 | This write to property 'pure4' is useless, since $@ always overrides it. | tst.js:15:5:15:16 | o.pure4 = 42 | another property write |
+| tst.js:20:5:20:17 | o.pure6 = f() | This write to property 'pure6' is useless, since $@ always overrides it. | tst.js:21:5:21:16 | o.pure6 = 42 | another property write |
+| tst.js:23:5:23:16 | o.pure7 = 42 | This write to property 'pure7' is useless, since $@ always overrides it. | tst.js:25:5:25:16 | o.pure7 = 42 | another property write |
+| tst.js:76:5:76:34 | o.pure1 ... te = 42 | This write to property 'pure16_simpleAliasWrite' is useless, since $@ always overrides it. | tst.js:77:5:77:36 | o16.pur ... te = 42 | another property write |
+| tst.js:95:5:95:17 | o.pure18 = 42 | This write to property 'pure18' is useless, since $@ always overrides it. | tst.js:96:5:96:17 | o.pure18 = 42 | another property write |
+| tst.js:96:5:96:17 | o.pure18 = 42 | This write to property 'pure18' is useless, since $@ always overrides it. | tst.js:97:5:97:17 | o.pure18 = 42 | another property write |

javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.qlref

@@ -0,0 +1 @@
+Declarations/DeadStoreOfProperty.ql

javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/closure-compiler-tricks.js

@@ -0,0 +1,12 @@
+(function(){
+    var o = {};
+    o.prop1 = o['prop1'] = x;
+
+    o['prop2'] = o.prop2 = x;
+
+    o.prop3 = x
+    o['prop3'] = x;
+
+    o['prop4'] = x;
+    o.prop4 = x
+});

javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/externs.js

@@ -0,0 +1,43 @@
+// Adapted from the Google Closure externs; original copyright header included below.
+/*
+ * Copyright 2008 The Closure Compiler Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * @interface
+ */
+function EventTarget() {}
+
+/**
+ * @constructor
+ * @implements {EventTarget}
+ * @see http://www.w3.org/TR/1998/REC-DOM-Level-1-19981001/level-one-core.html#ID-1950641247
+ */
+function Node() {}
+
+/**
+ * @constructor
+ * @extends {Node}
+ * @see http://www.w3.org/TR/1998/REC-DOM-Level-1-19981001/level-one-core.html#ID-745549614
+ */
+function Element() {}
+
+/**
+ * @type {number}
+ * @see http://www.w3.org/TR/cssom-view/#dom-element-clienttop
+ */
+Element.prototype.clientTop;
+
+//semmle-extractor-options: --externs

javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/list-manipulation.js

@@ -0,0 +1,13 @@
+(function(){
+    var first = f();
+
+    function flush() {
+        var flushed = first;
+
+        var next = g();
+        first = next;
+        next.prev = 42;
+
+        flushed.prev = 42;
+    }
+});

javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/real-world-examples.js

@@ -0,0 +1,39 @@
+(function(){
+	var o = f1();
+	while (f2()) {
+		if (f4()) {
+			o.p = 42; // NOT OK
+			break;
+		}
+		f5();
+	}
+	o.p = 42;
+});
+
+(function(){
+	var o = f1();
+	o.p1 = o.p1 += 42; // NOT OK
+	o.p2 -= (o.p2 *= 42); // NOT OK
+});
+
+(function(){
+	var o = f1();
+	o.m(function () {
+		if (f2()) {
+
+		} else {
+			try {
+				f3();
+			} catch (e) {
+				f4();
+				o.p = 42; // NOT OK
+			}
+		}
+		o.p = 42;
+	});
+});
+
+(function(){
+	var o = f1();
+	o.p = f2() ? o.p = f3() : f4(); // NOT OK
+});

javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/tst.js

@@ -0,0 +1,98 @@
+(function(){
+    var o = {};
+    o.pure1 = 42; // NOT OK
+    o.pure1 = 42;
+
+    o.pure2 = 42; // NOT OK
+    o.pure2 = 43;
+
+    o.impure3 = 42;
+    f();
+    o.impure3 = 42;
+
+    o.pure4 = 42; // NOT OK
+    43;
+    o.pure4 = 42;
+
+    o.impure5 = 42;
+    o.impure5 = f();
+
+    o.pure6 = f(); // NOT OK
+    o.pure6 = 42;
+
+    o.pure7 = 42; // NOT OK
+    if(x){}
+    o.pure7 = 42;
+
+    o.pure8_cond = 42;
+    if(x){
+        o.pure8_cond = 42;
+    }
+
+    o.impure9 = 42;
+    f();
+    if(x){
+    }
+    o.impure9 = 42;
+
+    o.impure10 = 42;
+    if(x){
+        f();
+    }
+    o.impure10 = 42;
+
+    o.impure11 = 42;
+    if(x){
+
+    }
+    f();
+    o.impure11 = 42;
+
+    o.pure12_read = 42;
+    o.pure12_read;
+    o.pure12_read = 42;
+
+    var o2;
+    o.pure13_otherRead = 42;
+    o2.pure13_otherRead;
+    o.pure13_otherRead = 42;
+
+    function id14(e) {
+        return e;
+    }
+    var o14 = id14(o);
+    o.pure14_aliasRead = 42;
+    o14.pure14_aliasRead;
+    o.pure14_aliasRead = 42;
+
+    function id15(e) {
+        return e;
+    }
+    var o15 = id15(o);
+    o.pure15_aliasWrite = 42;
+    o15.pure15_aliasWrite = 42;
+
+    var o16 = x? o: null;
+    o.pure16_simpleAliasWrite = 42; // NOT OK
+    o16.pure16_simpleAliasWrite = 42;
+
+    var o17 = {
+        duplicate17: 42,
+        duplicate17: 42
+    }
+
+    // DOM
+    o.clientTop = 42;
+    o.clientTop = 42;
+
+    o.defaulted1 = null;
+    o.defaulted1 = 42;
+
+    o.defaulted2 = -1;
+    o.defaulted2 = 42;
+
+    var o = {};
+    o.pure18 = 42; // NOT OK
+    o.pure18 = 42; // NOT OK
+    o.pure18 = 42;
+});