Eliminates crash in DisambiguateProperties when traversing object s…

…pread. 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
google · Mar 5, 2019 · 74f431a · 74f431a
1 parent 9f5a7bc
commit 74f431a
Show file tree

Hide file tree

Showing 2 changed files with 93 additions and 20 deletions.
diff --git 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();
+      for (Node child = n.getFirstChild(); child != null; child = child.getNext()) {
-          child != null;
+        switch (child.getToken()) {
-          child = child.getNext()) {
+          case COMPUTED_PROP:
-        if (child.isQuotedString() || child.isComputedProp()) {
+            // These won't be renamed due to our assumptions. Ignore them.
-          // Ignore properties that the compiler does not rename
+          case SPREAD:
-          //   var obj = {'quoted': 0, ['computed']: 1};
+            // Ignore properties added via spread. All properties accessed from object literals are
-          continue;
+            // 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.
+          case STRING_KEY:
-        String name = child.getString();
+          case MEMBER_FUNCTION_DEF:
-        JSType objlitType = getType(n);
+          case GETTER_DEF:
-        Property prop = getProperty(name);
+          case SETTER_DEF:
-        if (!prop.scheduleRenaming(child, objlitType)) {
+            if (child.isQuotedString()) {
-          // TODO(user): It doesn't look like the user can do much in this
+              continue; // These won't be renamed due to our assumptions. Ignore them.
-          // case right now.
+            }
-          if (propertiesToErrorFor.containsKey(name)) {
+
-            compiler.report(JSError.make(child, propertiesToErrorFor.get(name),
+            // We should never see a mix of numbers and strings.
-                Warnings.INVALIDATION, name, String.valueOf(objlitType), n.toString(), ""));
+            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
@@ -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(