diff --git a/src/com/google/javascript/jscomp/AmbiguateProperties.java b/src/com/google/javascript/jscomp/AmbiguateProperties.java index e7ff1b38095..e76262426ac 100644 --- a/src/com/google/javascript/jscomp/AmbiguateProperties.java +++ b/src/com/google/javascript/jscomp/AmbiguateProperties.java @@ -459,7 +459,8 @@ public void visit(NodeTraversal t, Node n, Node parent) { return; case OBJECTLIT: - processObjectLit(n); + case OBJECT_PATTERN: + processObjectLitOrPattern(n); return; case GETELEM: @@ -531,7 +532,7 @@ private void processCall(Node call) { } } - private void processObjectLit(Node objectLit) { + private void processObjectLitOrPattern(Node objectLit) { // Object.defineProperties literals are handled at the CALL node. if (objectLit.getParent().isCall() && NodeUtil.isObjectDefinePropertiesDefinition(objectLit.getParent())) { @@ -544,7 +545,16 @@ private void processObjectLit(Node objectLit) { for (Node key = objectLit.getFirstChild(); key != null; key = key.getNext()) { // We only want keys that were unquoted. // Keys are STRING, GET, SET - if (key.isQuotedString()) { + if (key.isComputedProp()) { + if (key.getFirstChild().isString()) { + // Ensure that we never rename some other property in a way that could conflict with + // this computed prop. This is largely because we store quoted member fns as + // computed properties and want to be consistent with how other quoted properties + // invalidate property names + quotedNames.add(key.getFirstChild().getString()); + } + // Never rename computed properties + } else if (key.isQuotedString()) { // Ensure that we never rename some other property in a way // that could conflict with this quoted key. quotedNames.add(key.getString()); @@ -571,10 +581,18 @@ private void processClass(Node classNode) { ? 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'() {} + if (member.isQuotedString()) { + // ignore get 'foo'() {} and prevent property name collisions // Note that only getters/setters are represented as quoted strings, not 'foo'() {} // see https://github.com/google/closure-compiler/issues/3071 + quotedNames.add(member.getString()); + continue; + } else if (member.isComputedProp()) { + // ignore ['foo']() {} + // for simple cases, we also prevent renaming collisions + if (member.getFirstChild().isString()) { + quotedNames.add(member.getFirstChild().getString()); + } continue; } else if ("constructor".equals(member.getString())) { // don't rename `class C { constructor() {} }` ! @@ -582,6 +600,7 @@ private void processClass(Node classNode) { // 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 diff --git a/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java b/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java index 7c80afb9cc7..c5340be7f35 100644 --- a/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java +++ b/test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java @@ -1276,6 +1276,13 @@ public void testQuotedMemberFnInClass_notAmbiguated() { testSame("/** @dict */ class Foo { 'methodFoo'() {} }"); } + @Test + public void testQuotedMemberFnInClassReservesPropertyName() { + test( + "/** @unrestricted */ class Foo { 'a'() {} foo() {} }", + "/** @unrestricted */ class Foo { 'a'() {} b() {} }"); + } + @Test public void testSingleClass_withTwoMemberFns_notAmbiguated() { test("class Foo { method1() {} method2() {} }", "class Foo { a() {} b() {} }"); @@ -1394,6 +1401,13 @@ public void testQuotedGetterInClass_isNotAmbiguated() { testSame("/** @dict */ class Foo { get 'prop'() {} }"); } + @Test + public void testQuotedGetterInClassReservesPropertyName() { + test( + "/** @unrestricted */ class Foo { get 'a'() {} foo() {} }", + "/** @unrestricted */ class Foo { get 'a'() {} b() {} }"); + } + @Test public void testSetterInClass_isAmbiguated() { test("class Foo { set prop(x) {} }", "class Foo { set a(x) {} }"); @@ -1419,6 +1433,24 @@ public void testComputedMemberFunctionInClass_notAmbiguated() { testSame("/** @dict */ class Foo { ['method']() {}}"); } + @Test + public void testSimpleComputedMemberFnInClassReservesPropertyName() { + test( + "/** @unrestricted */ class Foo { ['a']() {} foo() {} }", + "/** @unrestricted */ class Foo { ['a']() {} b() {} }"); + } + + @Test + public void testComplexComputedMemberFnInClassDoesntReservePropertyName() { + // we don't try to evaluate 'a' + '' to see that it's identical to 'a', and so end up renaming + // 'foo' -> 'a' + // The property name invalidation is already just a heuristic, so just handle the very simple + // case of ['a']() {} + test( + "/** @unrestricted */ class Foo { ['a' + '']() {} foo() {} }", + "/** @unrestricted */ class Foo { ['a' + '']() {} a() {} }"); + } + @Test public void testEs6ClassConstructorMethod_notAmbiguated() { testSame("class Foo { constructor() {} }"); @@ -1463,4 +1495,147 @@ public void testObjectSetPrototypeOfIsIgnored() { // now trying to reference Foo.barMethod will not work, and will call barMethod instead. // AmbiguateProperties currently ignores this case } + + @Test + public void testComputedPropertyInObjectLiteral_notAmbiguated() { + testSame("const obj = {['a']: 3, b: 4}"); + } + + @Test + public void testQuotedPropertyInObjectLiteralReservesPropertyName() { + test( + "const obj = {'a': 3}; class C { method() {}}", + // `method` is renamed to `b`, not `a`, to avoid colliding with the computed prop. + // This is just a heuristic; JSCompiler cannot statically determine all string + // property names. + "const obj = {'a': 3}; class C { b() {}}"); + } + + @Test + public void testQuotedMemberFnInObjectLiteralReservesPropertyName() { + test( + "const obj = {'a'() {}}; class C { method() {}}", + "const obj = {'a'() {}}; class C { b() {}}"); + } + + @Test + public void testComputedPropertyInObjectLiteralReservesPropertyName() { + test( + "const obj = {['a']: 3}; class C { method() {}}", + "const obj = {['a']: 3}; class C { b() {}}"); + } + + @Test + public void testMemberFnInObjectLiteralPreventsPropertyRenaming() { + // Don't rename the class member 'm' because the object literal type is invalidating, and + // also has a property 'm' + testSame("const obj = {m() {}}; class C {m() {}}"); + } + + @Test + public void testSimplePropInObjectLiteralPreventsPropertyRenaminge() { + // Don't rename the class member 'm' because the object literal type is invalidating, and + // also has a property 'm' + testSame("const obj = {m: 0}; class C {m() {}}"); + } + + @Test + public void testObjectPatternDeclarationWithStringKey_ambiguated() { + test( + lines( + "class Foo {", // + " method() {}", + "}", + "const {method} = new Foo();"), + lines( + "class Foo {", // + " a() {}", + "}", + "const {a: method} = new Foo();")); + } + + @Test + public void testObjectPatternDeclarationWithStringKeWithDefault_ambiguated() { + test( + lines( + "class Foo {", // + " method() {}", + "}", + "const {method = () => 3} = new Foo();"), + lines( + "class Foo {", // + " a() {}", + "}", + "const {a: method = () => 3} = new Foo();")); + } + + @Test + public void testNestedObjectPatternWithStringKey_ambiguated() { + test( + lines( + "class Foo {", // + " method() {}", + "}", + "const {foo: {method}} = {foo: new Foo()};"), + lines( + "class Foo {", // + " a() {}", + "}", + // note: we rename the 'method' access but not 'foo', because we don't try ambiguating + // properties on object literal types. + "const {foo: {a: method}} = {foo: new Foo()};")); + } + + @Test + public void testObjectPatternParameterWithStringKey_ambiguated() { + test( + lines( + "class Foo {", // + " method() {}", + "}", + "/** @param {!Foo} foo */", + "function f({method}) {}"), + lines( + "class Foo {", // + " a() {}", + "}", + "/** @param {!Foo} foo */", + "function f({a: method}) {}")); + } + + @Test + public void testObjectPatternQuotedStringKey_notAmbiguated() { + // this emits a warning for a computed property access on a struct, since property ambiguation + // will break this code. + ignoreWarnings(TypeValidator.ILLEGAL_PROPERTY_ACCESS); + test( + lines( + "class Foo {", // + " method() {}", + "}", + "const {'method': method} = new Foo();"), + lines( + "class Foo {", // + " a() {}", + "}", + "const {'method': method} = new Foo();")); + } + + @Test + public void testObjectPatternComputedProperty_notAmbiguated() { + // this emits a warning for a computed property access on a struct, since property ambiguation + // will break this code. + ignoreWarnings(TypeValidator.ILLEGAL_PROPERTY_ACCESS); + test( + lines( + "class Foo {", // + " method() {}", + "}", + "const {['method']: method} = new Foo();"), + lines( + "class Foo {", // + " a() {}", + "}", + "const {['method']: method} = new Foo();")); + } }