JS: add query js/unused-property

javascript/ql/src/Declarations/UnusedProperty.qhelp

@@ -0,0 +1,34 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+	<overview>
+		<p>
+			Unused object properties make code harder to maintain and use. Clients that are unaware that a
+			property is unused may perform nontrivial computations to compute a value that is ultimately
+			unused.
+		</p>
+
+	</overview>
+	<recommendation>
+		<p>Remove the unused property.</p>
+
+	</recommendation>
+	<example>
+
+		<p>
+			In this code, the function <code>f</code> initializes a property <code>prop_a</code> with a
+			call to the function <code>expensiveComputation</code>, but later on this property is never read.
+			Removing <code>prop</code> would improve code quality and performance.
+		</p>
+
+		<sample src="examples/UnusedProperty.js" />
+
+	</example>
+	<references>
+
+		<li>Coding Horror: <a href="http://blog.codinghorror.com/code-smells/">Code Smells</a>.</li>
+
+
+	</references>
+</qhelp>

javascript/ql/src/Declarations/UnusedProperty.ql

@@ -0,0 +1,74 @@
+/**
+ * @name Unused property
+ * @description Unused properties may be a symptom of a bug and should be examined carefully.
+ * @kind problem
+ * @problem.severity recommendation
+ * @id js/unused-property
+ * @tags maintainability
+ * @precision high
+ */
+
+import javascript
+import semmle.javascript.dataflow.CapturedNodes
+import UnusedVariable
+import UnusedParameter
+import Expressions.ExprHasNoEffect
+
+predicate hasUnknownPropertyRead(CapturedSource obj) {
+  // dynamic reads
+  exists(DataFlow::PropRead r | obj.getAPropertyRead() = r | not exists(r.getPropertyName()))
+  or
+  // reflective reads
+  obj.flowsToExpr(any(EnhancedForLoop l).getIterationDomain())
+  or
+  obj.flowsToExpr(any(InExpr l).getRightOperand())
+  or
+  obj.flowsToExpr(any(SpreadElement e).getOperand())
+  or
+  exists(obj.getAPropertyRead("hasOwnProperty"))
+  or
+  exists(obj.getAPropertyRead("propertyIsEnumerable"))
+}
+
+predicate flowsToTypeRestrictedExpression(CapturedSource n) {
+  exists (Expr restricted, TypeExpr type |
+    n.flowsToExpr(restricted) and
+    not type.isAny() |
+    exists (TypeAssertion assertion |
+      type = assertion.getTypeAnnotation() and
+      restricted = assertion.getExpression()
+    )
+    or
+    exists (BindingPattern v |
+      type = v.getTypeAnnotation() and
+      restricted = v.getAVariable().getAnAssignedExpr()
+    )
+    // no need to reason about writes to typed fields, captured nodes do not reach them
+  )
+}
+
+from DataFlow::PropWrite w, CapturedSource n, string name
+where
+  w = n.getAPropertyWrite(name) and
+  not exists(n.getAPropertyRead(name)) and
+  not w.getBase().analyze().getAValue() != n.analyze().getAValue() and
+  not hasUnknownPropertyRead(n) and
+  // avoid reporting if the definition is unreachable
+  w.getAstNode().getFirstControlFlowNode().getBasicBlock() instanceof ReachableBasicBlock and
+  // avoid implicitly read properties
+  not (
+    name = "toString" or
+    name = "valueOf" or
+    name.matches("@@%") // @@iterator, for example
+  ) and
+  // avoid flagging properties that a type system requires
+  not flowsToTypeRestrictedExpression(n) and
+  // flagged by js/unused-local-variable
+  not exists(UnusedLocal l | l.getAnAssignedExpr().getUnderlyingValue().flow() = n) and
+  // flagged by js/unused-parameter
+  not exists(Parameter p | isAnAccidentallyUnusedParameter(p) |
+    p.getDefault().getUnderlyingValue().flow() = n
+  ) and
+  // flagged by js/useless-expression
+  not inVoidContext(n.asExpr())
+select w, "Unused property " + name + "."

javascript/ql/src/Declarations/UnusedVariable.ql

@@ -10,23 +10,7 @@
  */
 
 import javascript
-
-/**
- * A local variable that is neither used nor exported, and is not a parameter
- * or a function name.
- */
-class UnusedLocal extends LocalVariable {
-  UnusedLocal() {
-    not exists(getAnAccess()) and
-    not exists(Parameter p | this = p.getAVariable()) and
-    not exists(FunctionExpr fe | this = fe.getVariable()) and
-    not exists(ClassExpr ce | this = ce.getVariable()) and
-    not exists(ExportDeclaration ed | ed.exportsAs(this, _)) and
-    not exists(LocalVarTypeAccess type | type.getVariable() = this) and
-    // common convention: variables with leading underscore are intentionally unused
-    getName().charAt(0) != "_"
-  }
-}
+import UnusedVariable
 
 /**
  * Holds if `v` is mentioned in a JSDoc comment in the same file, and that file

javascript/ql/src/Declarations/UnusedVariable.qll

@@ -0,0 +1,22 @@
+/**
+ * This library contains parts of the 'js/unused-local-variable' query implementation.
+ */
+
+import javascript
+
+/**
+ * A local variable that is neither used nor exported, and is not a parameter
+ * or a function name.
+ */
+class UnusedLocal extends LocalVariable {
+  UnusedLocal() {
+    not exists(getAnAccess()) and
+    not exists(Parameter p | this = p.getAVariable()) and
+    not exists(FunctionExpr fe | this = fe.getVariable()) and
+    not exists(ClassExpr ce | this = ce.getVariable()) and
+    not exists(ExportDeclaration ed | ed.exportsAs(this, _)) and
+    not exists(LocalVarTypeAccess type | type.getVariable() = this) and
+    // common convention: variables with leading underscore are intentionally unused
+    getName().charAt(0) != "_"
+  }
+}

javascript/ql/src/Declarations/examples/UnusedProperty.js

@@ -0,0 +1,8 @@
+function f() {
+	var o = {
+		prop_a: expensiveComputation(),
+		prop_b: anotherComputation()
+	};
+
+	return o.prop_b;
+}

javascript/ql/src/Expressions/ExprHasNoEffect.ql

@@ -16,40 +16,7 @@ import javascript
 import DOMProperties
 import semmle.javascript.frameworks.xUnit
 import semmle.javascript.RestrictedLocations
-
-/**
- * Holds if `e` appears in a syntactic context where its value is discarded.
- */
-predicate inVoidContext(Expr e) {
-  exists(ExprStmt parent |
-    // e is a toplevel expression in an expression statement
-    parent = e.getParent() and
-    // but it isn't an HTML attribute or a configuration object
-    not exists(TopLevel tl | tl = parent.getParent() |
-      tl instanceof CodeInAttribute
-      or
-      // if the toplevel in its entirety is of the form `({ ... })`,
-      // it is probably a configuration object (e.g., a require.js build configuration)
-      tl.getNumChildStmt() = 1 and e.stripParens() instanceof ObjectExpr
-    )
-  )
-  or
-  exists(SeqExpr seq, int i, int n |
-    e = seq.getOperand(i) and
-    n = seq.getNumOperands()
-  |
-    i < n - 1 or inVoidContext(seq)
-  )
-  or
-  exists(ForStmt stmt | e = stmt.getUpdate())
-  or
-  exists(ForStmt stmt | e = stmt.getInit() |
-    // Allow the pattern `for(i; i < 10; i++)`
-    not e instanceof VarAccess
-  )
-  or
-  exists(LogicalBinaryExpr logical | e = logical.getRightOperand() and inVoidContext(logical))
-}
+import ExprHasNoEffect
 
 /**
  * Holds if `e` is of the form `x;` or `e.p;` and has a JSDoc comment containing a tag.

javascript/ql/src/Expressions/ExprHasNoEffect.qll

@@ -0,0 +1,39 @@
+/**
+ * This library contains parts of the 'js/useless-expression' query implementation.
+ */
+
+import javascript
+
+/**
+ * Holds if `e` appears in a syntactic context where its value is discarded.
+ */
+predicate inVoidContext(Expr e) {
+  exists(ExprStmt parent |
+    // e is a toplevel expression in an expression statement
+    parent = e.getParent() and
+    // but it isn't an HTML attribute or a configuration object
+    not exists(TopLevel tl | tl = parent.getParent() |
+      tl instanceof CodeInAttribute
+      or
+      // if the toplevel in its entirety is of the form `({ ... })`,
+      // it is probably a configuration object (e.g., a require.js build configuration)
+      tl.getNumChildStmt() = 1 and e.stripParens() instanceof ObjectExpr
+    )
+  )
+  or
+  exists(SeqExpr seq, int i, int n |
+    e = seq.getOperand(i) and
+    n = seq.getNumOperands()
+  |
+    i < n - 1 or inVoidContext(seq)
+  )
+  or
+  exists(ForStmt stmt | e = stmt.getUpdate())
+  or
+  exists(ForStmt stmt | e = stmt.getInit() |
+    // Allow the pattern `for(i; i < 10; i++)`
+    not e instanceof VarAccess
+  )
+  or
+  exists(LogicalBinaryExpr logical | e = logical.getRightOperand() and inVoidContext(logical))
+}