diff --git a/javascript/config/suites/javascript/maintainability-more b/javascript/config/suites/javascript/maintainability-more index 353d3e18c04..6cf17736f53 100644 --- a/javascript/config/suites/javascript/maintainability-more +++ b/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 diff --git a/javascript/ql/src/Declarations/DeadStoreOfProperty.qhelp b/javascript/ql/src/Declarations/DeadStoreOfProperty.qhelp new file mode 100644 index 00000000000..fa93ba07198 --- /dev/null +++ b/javascript/ql/src/Declarations/DeadStoreOfProperty.qhelp @@ -0,0 +1,9 @@ + + + + + + + diff --git a/javascript/ql/src/Declarations/DeadStoreOfProperty.ql b/javascript/ql/src/Declarations/DeadStoreOfProperty.ql new file mode 100644 index 00000000000..863ecb923b5 --- /dev/null +++ b/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" diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.expected b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.expected new file mode 100644 index 00000000000..75d85508e5c --- /dev/null +++ b/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 | diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.qlref b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.qlref new file mode 100644 index 00000000000..448709b9054 --- /dev/null +++ b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/DeadStoreOfProperty.qlref @@ -0,0 +1 @@ +Declarations/DeadStoreOfProperty.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/closure-compiler-tricks.js b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/closure-compiler-tricks.js new file mode 100644 index 00000000000..8052fe206c3 --- /dev/null +++ b/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 +}); diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/externs.js b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/externs.js new file mode 100644 index 00000000000..a32e349e56d --- /dev/null +++ b/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 diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/list-manipulation.js b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/list-manipulation.js new file mode 100644 index 00000000000..60728af76e5 --- /dev/null +++ b/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; + } +}); diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/real-world-examples.js b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/real-world-examples.js new file mode 100644 index 00000000000..ff84fa90220 --- /dev/null +++ b/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(); +}); diff --git a/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/tst.js b/javascript/ql/test/query-tests/Declarations/DeadStoreOfProperty/tst.js new file mode 100644 index 00000000000..9c0bcb19561 --- /dev/null +++ b/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; +});