Support ES6 classes in AmbiguateProperties

Besides adding code to recognize that CLASS members create properties, this CL also begins to look for class-side inheritance in a function's prototype chain. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=222895063
google · Nov 28, 2018 · 41d68f1 · 41d68f1
1 parent d6a1354
commit 41d68f1
Show file tree

Hide file tree

Showing 3 changed files with 265 additions and 6 deletions.
diff --git a/src/com/google/javascript/jscomp/AmbiguateProperties.java b/src/com/google/javascript/jscomp/AmbiguateProperties.java
@@ -114,7 +114,11 @@ public int compare(Property p1, Property p2) {
   /** A map from JSType to a unique representative Integer. */
   private final BiMap<JSType, Integer> intForType = HashBiMap.create();
 
-  /** A map from JSType to JSTypeBitSet representing the types related to the type. */
+  /**
+   * A map from JSType to JSTypeBitSet representing the types related to the type.
+   *
+   * <p>A type is always related to itself.
+   */
   private final Map<JSType, JSTypeBitSet> relatedBitsets = new HashMap<>();
 
   /** A set of types that invalidate properties from ambiguation. */
@@ -269,9 +273,10 @@ private BitSet getRelatedTypesOnNonUnion(JSType type) {
    *                          I
    * }</pre>
    *
-   * <p>Note that we don't need to correctly handle the relationships between functions, because the
+   * <p>We also need to handle relationships between functions because of ES6 class-side inheritance
-   * function type is invalidating (i.e. its properties won't be ambiguated).
+   * although the top Function type itself is invalidating.
    */
+  @SuppressWarnings("ReferenceEquality")
   private void computeRelatedTypesForNonUnionType(JSType type) {
     // This method could be expanded to handle union types if necessary, but currently no union
     // types are ever passed as input so the method doesn't have logic for union types
@@ -302,6 +307,24 @@ private void computeRelatedTypesForNonUnionType(JSType type) {
         addRelatedInstance(subType, related);
       }
     }
+
+    // We only specifically handle implicit prototypes of functions in the case of ES6 classes
+    // For regular functions, the implicit prototype being Function.prototype does not matter
+    // because the type `Function` is invalidating.
+    // This may cause unexpected behavior for code that manually sets a prototype, e.g.
+    //   Object.setPrototypeOf(myFunction, prototypeObj);
+    // but code like that should not be used with --ambiguate_properties or type-based optimizations
+    FunctionType fnType = type.toMaybeFunctionType();
+    if (fnType != null) {
+      for (FunctionType subType : fnType.getDirectSubTypes()) {
+        // We record all subtypes of constructors, but don't care about old 'ES5-style' subtyping,
+        // just ES6-style. This is equivalent to saying that the subtype constructor's implicit
+        // prototype is the given type
+        if (fnType == subType.getImplicitPrototype()) {
+          addRelatedType(subType, related);
+        }
+      }
+    }
   }
 
   /**
@@ -312,9 +335,13 @@ private void addRelatedInstance(FunctionType constructor, JSTypeBitSet related)
     checkArgument(constructor.hasInstanceType(),
         "Constructor %s without instance type.", constructor);
     ObjectType instanceType = constructor.getInstanceType();
-    related.set(getIntForType(instanceType.getImplicitPrototype()));
+    addRelatedType(instanceType, related);
-    computeRelatedTypesForNonUnionType(instanceType);
+  }
-    related.or(relatedBitsets.get(instanceType));
+
+  /** Adds the given type and all its related types to the given bit set. */
+  private void addRelatedType(JSType type, JSTypeBitSet related) {
+    computeRelatedTypesForNonUnionType(type);
+    related.or(relatedBitsets.get(type));
   }
 
   class PropertyGraph implements AdjacencyGraph<Property, Void> {
@@ -439,6 +466,10 @@ public void visit(NodeTraversal t, Node n, Node parent) {
           processGetElem(n);
           return;
 
+        case CLASS:
+          processClass(n);
+          return;
+
         default:
           // Nothing to do.
       }
@@ -533,6 +564,31 @@ private void processGetElem(Node n) {
       }
     }
 
+    private void processClass(Node classNode) {
+      JSType classConstructorType = getJSType(classNode);
+      JSType classPrototype =
+          classConstructorType.isFunctionType()
+              ? classConstructorType.toMaybeFunctionType().getPrototype()
+              : compiler.getTypeRegistry().getNativeType(JSTypeNative.UNKNOWN_TYPE);
+      for (Node member : NodeUtil.getClassMembers(classNode).children()) {
+        if (member.isComputedProp() || member.isQuotedString()) {
+          // ignore ['foo']() {} or get 'foo'() {}
+          // Note that only getters/setters are represented as quoted strings, not 'foo'() {}
+          // see https://github.com/google/closure-compiler/issues/3071
+          continue;
+        } else if ("constructor".equals(member.getString())) {
+          // don't rename `class C { constructor() {} }` !
+          // This only applies for ES6 classes, not generic properties called 'constructor', which
+          // is why it's handled in this method specifically.
+          continue;
+        }
+        JSType memberOwnerType = member.isStaticMember() ? classConstructorType : classPrototype;
+
+        // member could be a MEMBER_FUNCTION_DEF, GETTER_DEF, or SETTER_DEF
+        maybeMarkCandidate(member, memberOwnerType);
+      }
+    }
+
     /**
      * If a property node is eligible for renaming, stashes a reference to it
      * and increments the property name's access count.

diff --git a/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java b/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java
@@ -1265,4 +1265,202 @@ public void testJSCompiler_renameProperty_oneArg_blocksAmbiguation() {
             "Foo.prototype.bar = 3;",
             "const barName = JSCompiler_renameProperty('bar');"));
   }
+
+  @Test
+  public void testSingleClass_withSingleMemberFn_ambiguated() {
+    test("class Foo { methodFoo() {} }", "class Foo { a() {} }");
+  }
+
+  @Test
+  public void testQuotedMemberFnInClass_notAmbiguated() {
+    testSame("/** @dict */ class Foo { 'methodFoo'() {} }");
+  }
+
+  @Test
+  public void testSingleClass_withTwoMemberFns_notAmbiguated() {
+    test("class Foo { method1() {} method2() {} }", "class Foo { a() {} b() {} }");
+  }
+
+  @Test
+  public void testSingleClass_withStaticAndPrototypeMemberFns_ambiguated() {
+    test("class Foo { static method1() {} method2() {} }", "class Foo { static a() {} a() {} }");
+  }
+
+  @Test
+  public void testTwoUnrelatedClasses_withMemberFns_ambiguated() {
+    test(
+        "class Foo { methodFoo() {} } class Bar { methodBar() {} }",
+        "class Foo { a() {} } class Bar { a() {} }");
+  }
+
+  @Test
+  public void testTwoUnrelatedClasses_withStaticMemberFns_ambiguatede() {
+    test(
+        lines(
+            "class Foo { static methodFoo() {} }", //
+            "class Bar { static methodBar() {} }"),
+        lines(
+            "class Foo { a() {} }", //
+            "class Bar { a() {} }"));
+  }
+
+  @Test
+  public void testEs6SuperclassStaticMethod_notAmbiguated() {
+    test(
+        lines(
+            "class Foo { static methodFoo() {} }",
+            "class Bar extends Foo { static methodBar() {} }"),
+        // Since someone could access Foo.methodFoo() through Bar, make sure the methods get
+        // distinct names.
+        lines(
+            "class Foo { static b() {} }", //
+            "class Bar extends Foo { static a() {} }"));
+  }
+
+  @Test
+  public void testEs6SubclassChain_withStaticMethods_notAmbiguated() {
+    test(
+        lines(
+            "class Foo { static methodFoo() { alert('foo'); } }",
+            "class Bar extends Foo { static methodBar() {alert('bar'); } }",
+            "class Baz extends Bar { static methodBaz() {alert('baz'); } }",
+            "class Moo extends Baz { static methodMoo() { alert('moo'); } }",
+            "Moo.methodFoo();"),
+        // All four static methods must get distinct names, so that Moo.a resolves correctly to
+        // Foo.a
+        lines(
+            "class Foo { static a() {alert('foo'); } }", //
+            "class Bar extends Foo { static b() {alert('bar'); } }",
+            "class Baz extends Bar { static c() {alert('baz'); } }",
+            "class Moo extends Baz { static d() { alert('moo'); } }",
+            "Moo.a();"));
+  }
+
+  @Test
+  public void testEs5ClassWithExtendsChainStaticMethods_notAmbiguated() {
+    test(
+        lines(
+            "/** @constructor */ function Foo () {}",
+            "Foo.methodFoo = function() { alert('foo'); };",
+            "class Bar extends Foo { static methodBar() {alert('bar'); } }",
+            "class Baz extends Bar { static methodBaz() {alert('baz'); } }",
+            "class Moo extends Baz { static methodMoo() { alert('moo'); } }",
+            "Moo.methodFoo();"),
+        lines(
+            "/** @constructor */ function Foo () {}",
+            "Foo.a = function() { alert('foo'); };",
+            "class Bar extends Foo { static b() {alert('bar'); } }",
+            "class Baz extends Bar { static c() {alert('baz'); } }",
+            "class Moo extends Baz { static d() { alert('moo'); } }",
+            "Moo.a();"));
+  }
+
+  @Test
+  public void testEs5ClassWithEs5SubclassWtaticMethods_ambiguated() {
+    test(
+        lines(
+            "/** @constructor */ function Foo () {}",
+            "Foo.methodFoo = function() { alert('foo'); };",
+            "/** @constructor @extends {Foo} */ function Bar() {}",
+            "Bar.methodBar = function() { alert('bar'); };"),
+        lines(
+            "/** @constructor */ function Foo () {}",
+            "Foo.a = function() { alert('foo'); };",
+            "/** @constructor @extends {Foo} */ function Bar() {}",
+            "Bar.a = function() { alert('bar'); };"));
+  }
+
+  @Test
+  public void testClassWithSuperclassStaticMethodsCalledWithSuper_ambiguated() {
+
+    test(
+        lines(
+            "class Foo { static methodFoo() {} }",
+            "class Bar extends Foo { static methodBar() { super.methodFoo(); } }"),
+        // Since someone could access Foo.methodFoo() through Bar, make sure the methods get
+        // distinct names.
+        lines(
+            "class Foo { static a() {} }", //
+            "class Bar extends Foo { static b() { super.a(); } }"));
+  }
+
+  @Test
+  public void testGetterInClass_ambiguated() {
+    test("class Foo { get prop() {} }", "class Foo { get a() {} }");
+  }
+
+  @Test
+  public void testQuotedGetterInClass_isNotAmbiguated() {
+    testSame("/** @dict */ class Foo { get 'prop'() {} }");
+  }
+
+  @Test
+  public void testSetterInClass_isAmbiguated() {
+    test("class Foo { set prop(x) {} }", "class Foo { set a(x) {} }");
+  }
+
+  @Test
+  public void testQuotedSetterInClass_notAmbiguated() {
+    testSame("/** @dict */ class Foo { set 'prop'(x) {} }");
+  }
+
+  @Test
+  public void testSameGetterAndSetterInClass_ambiguated() {
+    test("class Foo { get prop() {} set prop(x) {} }", "class Foo { get a() {} set a(x) {} }");
+  }
+
+  @Test
+  public void testDistinctGetterAndSetterInClass_notAmbiguated() {
+    test("class Foo { set propA(x) {} get propB() {} }", "class Foo { set a(x) {} get b() {} }");
+  }
+
+  @Test
+  public void testComputedMemberFunctionInClass_notAmbiguated() {
+    testSame("/** @dict */ class Foo { ['method']() {}}");
+  }
+
+  @Test
+  public void testEs6ClassConstructorMethod_notAmbiguated() {
+    testSame("class Foo { constructor() {} }");
+  }
+
+  @Test
+  public void testAmbiguateEs6ClassMethodsDoesntCrashOnClassInACast() {
+    // the cast causes the actual CLASS node to have the unknown type, so verify that the pass
+    // can handle it not being a function type.
+    testSame(
+        lines(
+            "const Foo = /** @type {?} */ (class {", //
+            "  method() {}",
+            "});",
+            "class Bar {",
+            "  method() {}",
+            "}"));
+  }
+
+  @Test
+  public void testObjectSetPrototypeOfIsIgnored() {
+    test(
+        externs("Object.setPrototypeOf = function(obj, proto) {}"),
+        srcs(
+            lines(
+                "/** @constructor */",
+                "function Foo() {}",
+                "Foo.fooMethod = () => 3;",
+                "/** @constructor */",
+                "function Bar() {}",
+                "Bar.barMethod = () => 4;",
+                "Object.setPrototypeOf(Foo, Bar);")),
+        expected(
+            lines(
+                "/** @constructor */",
+                "function Foo() {}",
+                "Foo.a = () => 3;",
+                "/** @constructor */",
+                "function Bar() {}",
+                "Bar.a = () => 4;",
+                "Object.setPrototypeOf(Foo, Bar);")));
+    // now trying to reference Foo.barMethod will not work, and will call barMethod instead.
+    // AmbiguateProperties currently ignores this case
+  }
 }
diff --git a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java
@@ -2993,6 +2993,11 @@ public void testEs6ClassSideInheritedMethods_referencedWithSuper_areDisambiguate
             "Bar.function_new_Bar___undefined$method();"));
   }
 
+  @Test
+  public void testDoNotDisambiguateEs6ClassConstructor() {
+    testSets("class Foo { constructor() {} } class Bar { constructor() {} }", "{}");
+  }
+
   /** Tests for ES6 object literal features */
   @Test
   public void testIgnoreComputedPropertyInObjectLiteral() {