Merge pull request #1656 from asger-semmle/rephrase-useless-def

Approved by xiemaisi

Author: semmle-qlci

javascript/ql/src/Declarations/DeadStoreOfLocal.ql

@@ -16,6 +16,9 @@ import DeadStore
 /**
  * Holds if `vd` is a definition of variable `v` that is dead, that is,
  * the value it assigns to `v` is not read.
+ *
+ * Captured variables may be read by closures, so we restrict this to
+ * purely local variables.
  */
 predicate deadStoreOfLocal(VarDef vd, PurelyLocalVariable v) {
   v = vd.getAVariable() and
@@ -26,7 +29,7 @@ predicate deadStoreOfLocal(VarDef vd, PurelyLocalVariable v) {
   not exists(SsaExplicitDefinition ssa | ssa.defines(vd, v))
 }
 
-from VarDef dead, PurelyLocalVariable v // captured variables may be read by closures, so don't flag them
+from VarDef dead, PurelyLocalVariable v, string msg
 where
   deadStoreOfLocal(dead, v) and
   // the variable should be accessed somewhere; otherwise it will be flagged by UnusedVariable
@@ -51,5 +54,13 @@ where
   // don't flag exported variables
   not any(ES2015Module m).exportsAs(v, _) and
   // don't flag 'exports' assignments in closure modules
-  not any(Closure::ClosureModule mod).getExportsVariable() = v
-select dead, "This definition of " + v.getName() + " is useless, since its value is never read."
+  not any(Closure::ClosureModule mod).getExportsVariable() = v and
+  (
+    // To avoid confusion about the meaning of "definition" and "declaration" we avoid
+    // the term "definition" when the alert location is a variable declaration.
+    if dead instanceof VariableDeclarator then
+      msg = "The initial value of " + v.getName() + " is unused, since it is always overwritten."
+    else
+      msg = "This definition of " + v.getName() + " is useless, since its value is never read."
+  )
+select dead, msg

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

@@ -1,12 +1,12 @@
 | overload.ts:10:12:10:14 | baz | This definition of baz is useless, since its value is never read. |
-| tst2.js:26:9:26:14 | x = 23 | This definition of x is useless, since its value is never read. |
+| tst2.js:26:9:26:14 | x = 23 | The initial value of x is unused, since it is always overwritten. |
 | tst2.js:28:9:28:14 | x = 42 | This definition of x is useless, since its value is never read. |
 | tst3.js:2:1:2:36 | exports ... a: 23 } | This definition of exports is useless, since its value is never read. |
 | tst3b.js:2:18:2:36 | exports = { a: 23 } | This definition of exports is useless, since its value is never read. |
 | tst.js:6:2:6:7 | y = 23 | This definition of y is useless, since its value is never read. |
-| tst.js:13:6:13:11 | a = 23 | This definition of a is useless, since its value is never read. |
+| tst.js:13:6:13:11 | a = 23 | The initial value of a is unused, since it is always overwritten. |
 | tst.js:13:14:13:19 | a = 42 | This definition of a is useless, since its value is never read. |
-| tst.js:45:6:45:11 | x = 23 | This definition of x is useless, since its value is never read. |
-| tst.js:51:6:51:11 | x = 23 | This definition of x is useless, since its value is never read. |
-| tst.js:132:7:132:13 | {x} = o | This definition of x is useless, since its value is never read. |
-| tst.js:162:6:162:14 | [x] = [0] | This definition of x is useless, since its value is never read. |
+| tst.js:45:6:45:11 | x = 23 | The initial value of x is unused, since it is always overwritten. |
+| tst.js:51:6:51:11 | x = 23 | The initial value of x is unused, since it is always overwritten. |
+| tst.js:132:7:132:13 | {x} = o | The initial value of x is unused, since it is always overwritten. |
+| tst.js:162:6:162:14 | [x] = [0] | The initial value of x is unused, since it is always overwritten. |