From 74f431acb666d9e02f5953ff4a154c32f7d58cf6 Mon Sep 17 00:00:00 2001
From: nickreid <nickreid@google.com>
Date: Fri, 1 Mar 2019 15:00:45 -0800
Subject: [PATCH] Eliminates crash in `DisambiguateProperties` when traversing
 object spread.

Nothing in particular is done to actually support object spread in disambiguation; however, the existing implementation backs-off sufficiently to not be dangerous. Tests are added to confirm this, and comments are added to document why.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=236384469
---
 .../jscomp/DisambiguateProperties.java        | 61 +++++++++++++------
 .../jscomp/DisambiguatePropertiesTest.java    | 52 +++++++++++++++-
 2 files changed, 93 insertions(+), 20 deletions(-)

diff --git a/src/com/google/javascript/jscomp/DisambiguateProperties.java b/src/com/google/javascript/jscomp/DisambiguateProperties.java
index cd893c7fd18..15840bd1746 100644
--- a/src/com/google/javascript/jscomp/DisambiguateProperties.java
+++ b/src/com/google/javascript/jscomp/DisambiguateProperties.java
@@ -531,26 +531,49 @@ private void handleObjectLit(Node n) {
         return;
       }
 
-      for (Node child = n.getFirstChild();
-          child != null;
-          child = child.getNext()) {
-        if (child.isQuotedString() || child.isComputedProp()) {
-          // Ignore properties that the compiler does not rename
-          //   var obj = {'quoted': 0, ['computed']: 1};
-          continue;
-        }
+      for (Node child = n.getFirstChild(); child != null; child = child.getNext()) {
+        switch (child.getToken()) {
+          case COMPUTED_PROP:
+            // These won't be renamed due to our assumptions. Ignore them.
+          case SPREAD:
+            // Ignore properties added via spread. All properties accessed from object literals are
+            // invalidated regardless, so we don't have to explicitly do that here. Additionally,
+            // even if we invalidated all the properties known to be on the spread type, there may
+            // be others we're unaware of, so it would be insufficient.
+            continue;
 
-        // We should never see a mix of numbers and strings.
-        String name = child.getString();
-        JSType objlitType = getType(n);
-        Property prop = getProperty(name);
-        if (!prop.scheduleRenaming(child, objlitType)) {
-          // TODO(user): It doesn't look like the user can do much in this
-          // case right now.
-          if (propertiesToErrorFor.containsKey(name)) {
-            compiler.report(JSError.make(child, propertiesToErrorFor.get(name),
-                Warnings.INVALIDATION, name, String.valueOf(objlitType), n.toString(), ""));
-          }
+          case STRING_KEY:
+          case MEMBER_FUNCTION_DEF:
+          case GETTER_DEF:
+          case SETTER_DEF:
+            if (child.isQuotedString()) {
+              continue; // These won't be renamed due to our assumptions. Ignore them.
+            }
+
+            // We should never see a mix of numbers and strings.
+            String name = child.getString();
+            JSType objlitType = getType(n);
+            Property prop = getProperty(name);
+            if (!prop.scheduleRenaming(child, objlitType)) {
+              // TODO(user): It doesn't look like the user can do much in this
+              // case right now.
+              if (propertiesToErrorFor.containsKey(name)) {
+                compiler.report(
+                    JSError.make(
+                        child,
+                        propertiesToErrorFor.get(name),
+                        Warnings.INVALIDATION,
+                        name,
+                        String.valueOf(objlitType),
+                        n.toString(),
+                        ""));
+              }
+            }
+            break;
+
+          default:
+            throw new IllegalStateException(
+                "Unexpected child of OBJECTLIT: " + child.toStringTree());
         }
       }
     }
diff --git a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java
index c601ed6e8c7..f2fdff01cde 100644
--- a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java
+++ b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java
@@ -3370,7 +3370,7 @@ public void testDisambiguatePropertyReference_objectPattern_stringKey_inNestedPa
   }
 
   @Test
-  public void testPropertyReference_leftFromObjectPatternRest_blocksDisambiguation() {
+  public void testObjectRest_blocksDisambiguation_ofRestedType() {
     setAcceptedLanguage(LanguageMode.ECMASCRIPT_2018);
     testSets(
         lines(
@@ -3387,6 +3387,56 @@ public void testPropertyReference_leftFromObjectPatternRest_blocksDisambiguation
         "{}");
   }
 
+  @Test
+  public void testObjectSpread_blocksDisambiguation_ofPropertiesAccessedFromResultType() {
+    setAcceptedLanguage(LanguageMode.ECMASCRIPT_2018);
+    testSets(
+        lines(
+            "class Foo {", //
+            "  constructor() {",
+            "    /** @const {number} */",
+            "    this.prop = 3;",
+            "  }",
+            "",
+            "  /** @return {number} */",
+            "  method() { return 0; }",
+            "}",
+            "",
+            "const spread = {...new Foo()};",
+            "alert(spread.prop);"),
+        // `method` can be disambiguated, because we know it is never accessed via `spread.method`.
+        // This is not true for `prop`.
+        "{method=[[Foo.prototype]]}");
+  }
+
+  @Test
+  public void testObjectSpread_blocksDisambiguation_whenResultIsUsedAsIntermediaryType() {
+    setAcceptedLanguage(LanguageMode.ECMASCRIPT_2018);
+    testSets(
+        lines(
+            "class Foo {", //
+            "  constructor() {",
+            "    /** @const {number} */",
+            "    this.prop = 3;",
+            "  }",
+            "}",
+            "",
+            "/** @record */",
+            "class Bar {", //
+            "  constructor() {",
+            "    /** @type {number} */",
+            "    this.prop = 3;",
+            "  }",
+            "}",
+            "",
+            "const spread = {...new Foo()};",
+            "const /** !Bar */ bar = spread;",
+            "alert(bar.prop);"),
+        // Both Bar and Foo types are conflated with an anonymous object type created via spread,
+        // so none of their properties can be disambiguated.
+        "{}");
+  }
+
   @Test
   public void testObjectPattern_withTypeMismatch_emitsInvalidationError_aboutType() {
     testError(