Merge pull request #2210 from max-schaefer/js/better-destructuring-type-inference

Approved by asger-semmle, esbena

Author: semmle-qlci

javascript/ql/src/semmle/javascript/dataflow/internal/PropertyTypeInference.qll

@@ -50,17 +50,17 @@ abstract class AnalyzedPropertyRead extends DataFlow::AnalyzedNode {
 }
 
 /**
- * Flow analysis for (non-numeric) property read accesses.
+ * Flow analysis for non-numeric property read accesses.
  */
-private class AnalyzedPropertyAccess extends AnalyzedPropertyRead, DataFlow::ValueNode {
-  override PropAccess astNode;
+private class AnalyzedNonNumericPropertyRead extends AnalyzedPropertyRead {
+  DataFlow::PropRead self;
   DataFlow::AnalyzedNode baseNode;
   string propName;
 
-  AnalyzedPropertyAccess() {
-    astNode.accesses(baseNode.asExpr(), propName) and
-    isNonNumericPropertyName(propName) and
-    astNode instanceof RValue
+  AnalyzedNonNumericPropertyRead() {
+    this = self and
+    self.accesses(baseNode, propName) and
+    isNonNumericPropertyName(propName)
   }
 
   override predicate reads(AbstractValue base, string prop) {
@@ -148,26 +148,26 @@ private predicate explicitPropertyWrite(
  * Flow analysis for `arguments.callee`. We assume it is never redefined,
  * which is unsound in practice, but pragmatically useful.
  */
-private class AnalyzedArgumentsCallee extends AnalyzedPropertyAccess {
+private class AnalyzedArgumentsCallee extends AnalyzedNonNumericPropertyRead {
   AnalyzedArgumentsCallee() { propName = "callee" }
 
   override AbstractValue getALocalValue() {
     exists(AbstractArguments baseVal | reads(baseVal, _) |
       result = TAbstractFunction(baseVal.getFunction())
     )
     or
-    hasNonArgumentsBase(astNode) and result = super.getALocalValue()
+    hasNonArgumentsBase(self) and result = super.getALocalValue()
   }
 }
 
 /**
- * Holds if `pacc` is of the form `e.callee` where `e` could evaluate to some
+ * Holds if `pr` is of the form `e.callee` where `e` could evaluate to some
  * value that is not an arguments object.
  */
-private predicate hasNonArgumentsBase(PropAccess pacc) {
-  pacc.getPropertyName() = "callee" and
+private predicate hasNonArgumentsBase(DataFlow::PropRead pr) {
+  pr.getPropertyName() = "callee" and
   exists(AbstractValue baseVal |
-    baseVal = pacc.getBase().analyze().getALocalValue() and
+    baseVal = pr.getBase().analyze().getALocalValue() and
     not baseVal instanceof AbstractArguments
   )
 }

javascript/ql/src/semmle/javascript/dataflow/internal/VariableTypeInference.qll

@@ -223,7 +223,11 @@ abstract class AnalyzedSsaDefinition extends SsaDefinition {
  * Flow analysis for SSA definitions corresponding to `VarDef`s.
  */
 private class AnalyzedExplicitDefinition extends AnalyzedSsaDefinition, SsaExplicitDefinition {
-  override AbstractValue getAnRhsValue() { result = getDef().(AnalyzedVarDef).getAnAssignedValue() }
+  override AbstractValue getAnRhsValue() {
+    result = getDef().(AnalyzedVarDef).getAnAssignedValue()
+    or
+    result = getRhsNode().analyze().getALocalValue()
+  }
 }
 
 /**
@@ -241,6 +245,8 @@ private class AnalyzedVariableCapture extends AnalyzedSsaDefinition, SsaVariable
     exists(LocalVariable v | v = getSourceVariable() |
       result = v.(AnalyzedCapturedVariable).getALocalValue()
       or
+      result = any(AnalyzedExplicitDefinition def | def.getSourceVariable() = v).getAnRhsValue()
+      or
       not guaranteedToBeInitialized(v) and result = getImplicitInitValue(v)
     )
   }

javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/client.js

@@ -0,0 +1,12 @@
+const lib = require("./lib"),
+      { f } = require("./lib");
+
+/** calls:lib.f */
+lib.f();
+/** calls:lib.f */
+f();
+
+(function() {
+  /** calls:lib.f */
+  f();
+})();

javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/lib.js

@@ -0,0 +1,3 @@
+module.exports.f =
+  /** name:lib.f */
+ function() {};

javascript/ql/test/library-tests/Flow/abseval.expected

@@ -80,6 +80,7 @@
 | classAccessors.js:12:9:12:11 | myZ | classAccessors.js:12:15:12:20 | this.z | file://:0:0:0:0 | indefinite value (call) |
 | classAccessors.js:12:9:12:11 | myZ | classAccessors.js:12:15:12:20 | this.z | file://:0:0:0:0 | indefinite value (heap) |
 | destructuring.js:2:7:2:24 | { x, y = (z = x) } | destructuring.js:2:28:2:28 | o | file://:0:0:0:0 | indefinite value (call) |
+| destructuring.js:3:7:3:8 | z1 | destructuring.js:3:12:3:12 | z | file://:0:0:0:0 | indefinite value (call) |
 | destructuring.js:3:7:3:8 | z1 | destructuring.js:3:12:3:12 | z | file://:0:0:0:0 | indefinite value (heap) |
 | es2015.js:1:5:1:7 | Sup | es2015.js:1:11:6:1 | class { ... ;\\n  }\\n} | es2015.js:1:11:6:1 | class Sup |
 | es2015.js:4:9:4:12 | ctor | es2015.js:4:16:4:25 | new.target | file://:0:0:0:0 | indefinite value (call) |