JS: add query: js/useless-assignment-to-property

javascript/config/suites/javascript/maintainability-more

@@ -2,6 +2,7 @@
 + semmlecode-javascript-queries/Comments/TodoComments.ql: /Maintainability/Comments
 + semmlecode-javascript-queries/Declarations/DeadStoreOfGlobal.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Declarations/DeadStoreOfLocal.ql: /Maintainability/Declarations
++ semmlecode-javascript-queries/Declarations/DeadStoreOfProperty.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Declarations/DuplicateVarDecl.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Declarations/UnusedParameter.ql: /Maintainability/Declarations
 + semmlecode-javascript-queries/Declarations/UnusedVariable.ql: /Maintainability/Declarations

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,138 @@
+/**
+ * @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 `assign` definitely assigns property `name` of `base`.
+ */
+predicate unambigiousPropWrite(DataFlow::SourceNode base, string name, Assignment assign) {
+  exists(DataFlow::PropWrite lhs |
+    assign.getLhs().flow() = lhs and
+    base.getAPropertyWrite(name) = lhs and
+    not exists (DataFlow::SourceNode otherBase |
+      not otherBase = base and
+      lhs = 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, Assignment assign1, Assignment assign2) {
+  exists (DataFlow::SourceNode base, ReachableBasicBlock block1, ReachableBasicBlock block2 |
+    block1 = assign1.getBasicBlock() and
+    block2 = assign2.getBasicBlock() and
+    unambigiousPropWrite(base, name, assign1) and
+    unambigiousPropWrite(base, name, assign2) and
+    block2.postDominates(block1) and
+    (block1 = block2 implies
+      exists (int i1, int i2 |
+        assign1 = block1.getNode(i1) and
+        assign2 = 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, Assignment assign1, Assignment assign2) {
+  postDominatedPropWrite(name, assign1, assign2) and
+  maybeHasPropAccessBetween(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 if the access happens before or after the node for `assign`.
+ */
+bindingset[name]
+predicate maybeAccessesAssignedPropInBlock(string name, Assignment assign, boolean after) {
+  exists (ReachableBasicBlock block, int i, int j, Expr e |
+    block = assign.getBasicBlock() and
+    assign = 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 may be accessed between the two assignments.
+ */
+bindingset[name]
+predicate maybeHasPropAccessBetween(string name, Assignment assign1, Assignment assign2) {
+  exists (ReachableBasicBlock block1, ReachableBasicBlock block2 |
+    assign1.getBasicBlock() = block1 and
+    assign2.getBasicBlock() = block2 and
+    if block1 = block2 then
+      // same block: check for read between
+      not exists (int i1, int iMid, Expr mid, int i2 |
+        assign1 = block1.getNode(i1) and
+        assign2 = block2.getNode(i2) and
+        i1 < iMid and iMid < i2 and
+        mid = block1.getNode(iMid) and
+        maybeAccessesProperty(mid, name)
+      )
+    else
+      // other block:
+      not (
+        // check for read after in start block
+        maybeAccessesAssignedPropInBlock(name, assign1, true) or
+        // check for read after in other block
+        exists (ReachableBasicBlock mid |
+          block1.getASuccessor+() = mid and
+          mid.getASuccessor+() = block2 |
+          maybeAccessesProperty(mid.getANode(), name)
+        ) or
+        // check for read before in end block
+        maybeAccessesAssignedPropInBlock(name, assign2, false)
+      )
+  )
+}
+
+from string name, Assignment assign1, Assignment 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.getLhs() and
+          p2 = assign2.getLhs() |
+          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().getUnderlyingValue())
+        or
+        // don't flag assignments in externs
+        assign1.inExternsFile()
+      )
+select assign1, "This write to property '" + name + "' is useless, since $@ always overrides it.", assign2, "another property write"

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;
+			break;
+		}
+		f5();
+	}
+	o.p = 42;
+});
+
+(function(){
+	var o = f1();
+	o.p1 = o.p1 += 42;
+	o.p2 -= (o.p2 *= 42);
+});
+
+(function(){
+	var o = f1();
+	o.m(function () {
+		if (f2()) {
+
+		} else {
+			try {
+				f3();
+			} catch (e) {
+				f4();
+				o.p = 42;
+			}
+		}
+		o.p = 42;
+	});
+});
+
+(function(){
+	var o = f1();
+	o.p = f2() ? o.p = f3() : f4();
+});

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

@@ -0,0 +1,98 @@
+(function(){
+    var o = {};
+    o.pure1 = 42;
+    o.pure1 = 42;
+
+    o.pure2 = 42;
+    o.pure2 = 43;
+
+    o.impure3 = 42;
+    f();
+    o.impure3 = 42;
+
+    o.pure4 = 42;
+    43;
+    o.pure4 = 42;
+
+    o.impure5 = 42;
+    o.impure5 = f();
+
+    o.pure6 = f();
+    o.pure6 = 42;
+
+    o.pure7 = 42;
+    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;
+    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;
+    o.pure18 = 42;
+    o.pure18 = 42;
+});