From 2e024c3c61b076d56b234309fd2ef19f9f571eab Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Wed, 20 Jan 2021 15:26:39 +0100
Subject: [PATCH] fix that type inference assumed every compound-assignment
 have type number

---
 .../ql/src/Expressions/ImplicitOperandConversion.ql  |  8 +-------
 javascript/ql/src/semmle/javascript/Expr.qll         | 12 ++++++++++++
 .../dataflow/internal/BasicExprTypeInference.qll     |  4 +++-
 .../LanguageFeatures/PropertyWriteOnPrimitive/tst.js |  6 ++++++
 4 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/javascript/ql/src/Expressions/ImplicitOperandConversion.ql b/javascript/ql/src/Expressions/ImplicitOperandConversion.ql
index a10b5b9c0c3..00196d9ef25 100644
--- a/javascript/ql/src/Expressions/ImplicitOperandConversion.ql
+++ b/javascript/ql/src/Expressions/ImplicitOperandConversion.ql
@@ -130,13 +130,7 @@ class NumericConversion extends ImplicitConversion {
     or
     parent instanceof ArithmeticExpr and not parent instanceof AddExpr
     or
-    parent instanceof CompoundAssignExpr and
-    not (
-      parent instanceof AssignAddExpr or
-      parent instanceof AssignLogOrExpr or
-      parent instanceof AssignLogAndExpr or
-      parent instanceof AssignNullishCoalescingExpr
-    )
+    parent.(CompoundAssignExpr).isNumeric()
     or
     parent instanceof UpdateExpr
   }
diff --git a/javascript/ql/src/semmle/javascript/Expr.qll b/javascript/ql/src/semmle/javascript/Expr.qll
index 1a932a7c08b..8d15653c5b2 100644
--- a/javascript/ql/src/semmle/javascript/Expr.qll
+++ b/javascript/ql/src/semmle/javascript/Expr.qll
@@ -1921,6 +1921,18 @@ private class TCompoundAssignExpr =
  */
 class CompoundAssignExpr extends TCompoundAssignExpr, Assignment {
   override string getAPrimaryQlClass() { result = "CompoundAssignExpr" }
+
+  /**
+   * Holds if this compound assignment always returns a number value.
+   */
+  predicate isNumeric() {
+    not (
+      this instanceof AssignAddExpr or
+      this instanceof AssignLogOrExpr or
+      this instanceof AssignLogAndExpr or
+      this instanceof AssignNullishCoalescingExpr
+    )
+  }
 }
 
 /**
diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/BasicExprTypeInference.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/BasicExprTypeInference.qll
index ab31b07f31c..776eacb5804 100644
--- a/javascript/ql/src/semmle/javascript/dataflow/internal/BasicExprTypeInference.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/internal/BasicExprTypeInference.qll
@@ -391,13 +391,15 @@ private class AnalyzedUpdateExpr extends DataFlow::AnalyzedValueNode {
 private class AnalyzedCompoundAssignExpr extends DataFlow::AnalyzedValueNode {
   override CompoundAssignExpr astNode;
 
+  AnalyzedCompoundAssignExpr() { astNode.isNumeric() }
+
   override AbstractValue getALocalValue() { result = abstractValueOfType(TTNumber()) }
 }
 
 /**
  * Flow analysis for add-assign.
  */
-private class AnalyzedAssignAddExpr extends AnalyzedCompoundAssignExpr {
+private class AnalyzedAssignAddExpr extends DataFlow::AnalyzedValueNode {
   override AssignAddExpr astNode;
 
   override AbstractValue getALocalValue() {
diff --git a/javascript/ql/test/query-tests/LanguageFeatures/PropertyWriteOnPrimitive/tst.js b/javascript/ql/test/query-tests/LanguageFeatures/PropertyWriteOnPrimitive/tst.js
index 40f77627e32..5d83bad877d 100644
--- a/javascript/ql/test/query-tests/LanguageFeatures/PropertyWriteOnPrimitive/tst.js
+++ b/javascript/ql/test/query-tests/LanguageFeatures/PropertyWriteOnPrimitive/tst.js
@@ -20,4 +20,10 @@ function g(b) {
   return;
   // OK: no types inferred for `z`, since this is dead code
   z.y = true;
+}
+
+function h() {
+  let tmp;
+  let obj = (tmp ||= {});
+  obj.p = 42;
 }
\ No newline at end of file