diff --git a/src/com/google/javascript/jscomp/AmbiguateProperties.java b/src/com/google/javascript/jscomp/AmbiguateProperties.java index d610fb66c1f..e7ff1b38095 100644 --- 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 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. + * + *

A type is always related to itself. + */ private final Map relatedBitsets = new HashMap<>(); /** A set of types that invalidate properties from ambiguation. */ @@ -269,9 +273,10 @@ private BitSet getRelatedTypesOnNonUnion(JSType type) { * I * } * - *

Note that we don't need to correctly handle the relationships between functions, because the - * function type is invalidating (i.e. its properties won't be ambiguated). + *

We also need to handle relationships between functions because of ES6 class-side inheritance + * 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())); - computeRelatedTypesForNonUnionType(instanceType); - related.or(relatedBitsets.get(instanceType)); + addRelatedType(instanceType, related); + } + + /** 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 { @@ -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 index 87989f7b1d5..7c80afb9cc7 100644 --- 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 index fe3e00c5a91..75f20220024 100644 --- 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() {