From 8d14b517bc4f103de11eb291a34960a12cdd1f15 Mon Sep 17 00:00:00 2001 From: johnplaisted Date: Tue, 13 Nov 2018 14:02:09 -0800 Subject: [PATCH] Have ES6RewriteClass handle getter and setters with quoted keys correctly. This may break @nocollapse and/or @export on these properties, but it is a super minor case. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=221329456 --- .../google/javascript/jscomp/AstFactory.java | 11 + .../javascript/jscomp/Es6RewriteClass.java | 204 +++++++++++++----- .../jscomp/Es6RewriteClassTest.java | 82 +++++++ 3 files changed, 242 insertions(+), 55 deletions(-) diff --git a/src/com/google/javascript/jscomp/AstFactory.java b/src/com/google/javascript/jscomp/AstFactory.java index 01c0dd4bac5..795c1f552a9 100644 --- a/src/com/google/javascript/jscomp/AstFactory.java +++ b/src/com/google/javascript/jscomp/AstFactory.java @@ -90,6 +90,17 @@ boolean isAddingTypes() { return registry != null; } + /** + * Returns a new EXPR_RESULT node. + * + *

Blocks have no type information, so this is functionally the same as calling {@code + * IR.exprResult(expr)}. It exists so that a pass can be consistent about always using {@code + * AstFactory} to create new nodes. + */ + Node exprResult(Node expr) { + return IR.exprResult(expr); + } + /** * Returns a new BLOCK node. * diff --git a/src/com/google/javascript/jscomp/Es6RewriteClass.java b/src/com/google/javascript/jscomp/Es6RewriteClass.java index 6349bd47931..a4a5939d515 100644 --- a/src/com/google/javascript/jscomp/Es6RewriteClass.java +++ b/src/com/google/javascript/jscomp/Es6RewriteClass.java @@ -402,7 +402,11 @@ private void addToDefinePropertiesObject(ClassDeclarationMetadata metadata, Node obj.addChildToBack( astFactory.createComputedProperty(member.getFirstChild().cloneTree(), prop)); } else { - obj.addChildToBack(astFactory.createStringKey(member.getString(), prop)); + Node stringKey = astFactory.createStringKey(member.getString(), prop); + if (member.isQuotedString()) { + stringKey.putBooleanProp(Node.QUOTED_PROP, true); + } + obj.addChildToBack(stringKey); } } @@ -441,22 +445,30 @@ private void visitNonMethodMember(Node member, ClassDeclarationMetadata metadata JSTypeExpression typeExpr = getTypeFromGetterOrSetter(member); addToDefinePropertiesObject(metadata, member); - Map membersToDeclare; + Map membersToDeclare = + member.isStaticMember() + ? metadata.getClassMembersToDeclare() + : metadata.getPrototypeMembersToDeclare(); + ClassProperty.Builder builder = ClassProperty.builder(); String memberName; + if (member.isComputedProp()) { checkState(!member.isStaticMember()); - membersToDeclare = metadata.getPrototypeComputedPropsToDeclare(); memberName = member.getFirstChild().getQualifiedName(); + builder.kind(ClassProperty.PropertyKind.COMPUTED_PROPERTY); + } else if (member.isQuotedString()) { + memberName = member.getString(); + builder.kind(ClassProperty.PropertyKind.QUOTED_PROPERTY); } else { - membersToDeclare = - member.isStaticMember() - ? metadata.getClassMembersToDeclare() - : metadata.getPrototypeMembersToDeclare(); memberName = member.getString(); + builder.kind(ClassProperty.PropertyKind.NORMAL_PROPERTY); } - JSDocInfo existingJSDoc = membersToDeclare.get(memberName); - JSTypeExpression existingType = existingJSDoc == null ? null : existingJSDoc.getType(); - if (existingType != null && typeExpr != null && !existingType.equals(typeExpr)) { + + builder.propertyKey(memberName); + ClassProperty existingProperty = membersToDeclare.get(memberName); + JSTypeExpression existingType = + existingProperty == null ? null : existingProperty.jsDocInfo().getType(); + if (existingProperty != null && typeExpr != null && !existingType.equals(typeExpr)) { compiler.report(JSError.make(member, CONFLICTING_GETTER_SETTER_TYPE, memberName)); } else { JSDocInfoBuilder jsDoc = new JSDocInfoBuilder(false); @@ -475,7 +487,8 @@ private void visitNonMethodMember(Node member, ClassDeclarationMetadata metadata if (member.isStaticMember() && !member.isComputedProp()) { jsDoc.recordNoCollapse(); } - membersToDeclare.put(memberName, jsDoc.build()); + builder.jsDocInfo(jsDoc.build()); + membersToDeclare.put(memberName, builder.build()); } } @@ -515,41 +528,17 @@ private void visitMethod(Node member, ClassDeclarationMetadata metadata) { */ private void addTypeDeclarations( Scope scope, ClassDeclarationMetadata metadata, Node insertionPoint) { - for (Map.Entry entry : metadata.getPrototypeMembersToDeclare().entrySet()) { - String declaredMember = entry.getKey(); - Node declaration = - astFactory.createGetProp(metadata.getClassPrototypeNode().cloneTree(), declaredMember); - declaration.setJSDocInfo(entry.getValue()); - declaration = - IR.exprResult(declaration).useSourceInfoIfMissingFromForTree(metadata.getClassNameNode()); - insertionPoint.getParent().addChildAfter(declaration, insertionPoint); - insertionPoint = declaration; - } - for (Map.Entry entry : metadata.getClassMembersToDeclare().entrySet()) { - String declaredMember = entry.getKey(); + for (ClassProperty property : metadata.getPrototypeMembersToDeclare().values()) { Node declaration = - astFactory.createGetProp(metadata.getFullClassNameNode().cloneTree(), declaredMember); - declaration.setJSDocInfo(entry.getValue()); - declaration = - IR.exprResult(declaration).useSourceInfoIfMissingFromForTree(metadata.getClassNameNode()); + property.getDeclaration(astFactory, scope, metadata.getClassPrototypeNode().cloneTree()); + declaration.useSourceInfoIfMissingFromForTree(metadata.getClassNameNode()); insertionPoint.getParent().addChildAfter(declaration, insertionPoint); insertionPoint = declaration; } - for (Map.Entry entry : - metadata.getPrototypeComputedPropsToDeclare().entrySet()) { - String declaredMember = entry.getKey(); - // Note that at the moment we only allow qualified names as the keys for class computed - // properties. - // TODO(bradfordcsmith): Clone the original declared member qualified name instead of creating - // it from scratch from the string form. That way we would just reuse the type information, - // instead of AstFactory having to re-create it. - Node computedPropertyQName = astFactory.createQName(scope, declaredMember); + for (ClassProperty property : metadata.getClassMembersToDeclare().values()) { Node declaration = - astFactory.createGetElem( - metadata.getClassPrototypeNode().cloneTree(), computedPropertyQName); - declaration.setJSDocInfo(entry.getValue()); - declaration = - IR.exprResult(declaration).useSourceInfoIfMissingFromForTree(metadata.getClassNameNode()); + property.getDeclaration(astFactory, scope, metadata.getFullClassNameNode().cloneTree()); + declaration.useSourceInfoIfMissingFromForTree(metadata.getClassNameNode()); insertionPoint.getParent().addChildAfter(declaration, insertionPoint); insertionPoint = declaration; } @@ -620,6 +609,118 @@ private Node createPropertyDescriptor() { .setJSType(objectPropertyDescriptorType); } + @AutoValue + abstract static class ClassProperty { + enum PropertyKind { + /** + * Any kind of quoted property, which can include numeric properties that we treated as + * quoted. + * + * 

+       * class Example {
+       *   'quoted'() {}
+       *   42() { return 'the answer'; }
+       * }
+       *
+ */ + QUOTED_PROPERTY, + + /** + * A computed property, e.g. using bracket [] access. + * + *

Computed properties *must* currently be qualified names, and not literals, function + * calls, etc. + * + * 

+       * class Example {
+       *   [variable]() {}
+       *   [another.example]() {}
+       * }
+       *
+ */ + COMPUTED_PROPERTY, + + /** + * A normal property definition. + * + * 
+       * class Example {
+       *   normal() {}
+       * }
+       *
+ */ + NORMAL_PROPERTY, + } + + /** + * The name of this ClassProperty for NORMAL_PROPERTY, the string value of this property if + * QUOTED_PROPERTY, or the qualified name of the computed property. + */ + abstract String propertyKey(); + + abstract PropertyKind kind(); + + abstract JSDocInfo jsDocInfo(); + + /** + * Returns an EXPR_RESULT node that declares this property on the given node. + * + *

Examples: + * + * 

+     *   /** @type {string} *\/
+     *   Class.prototype.property;
+     *
+     *   /** @type {string} *\/
+     *   Class.staticProperty;
+     *
+ * + * @param toDeclareOn the node to declare the property on. This should either be a reference to + * the class (if a static property) or a class' prototype (if non-static). This should + * always be a new node, as this method will insert it into the returned EXPR_RESULT. + */ + final Node getDeclaration(AstFactory astFactory, Scope scope, Node toDeclareOn) { + Node decl = null; + + switch (kind()) { + case QUOTED_PROPERTY: + decl = astFactory.createGetElem(toDeclareOn, astFactory.createString(propertyKey())); + break; + case COMPUTED_PROPERTY: + // Note that at the moment we only allow qualified names as the keys for class computed + // properties. + // TODO(bradfordcsmith): Clone the original declared member qualified name instead of + // creating it from scratch from the string form. That way we would just reuse the type + // information, instead of AstFactory having to re-create it. + decl = + astFactory.createGetElem(toDeclareOn, astFactory.createQName(scope, propertyKey())); + break; + case NORMAL_PROPERTY: + decl = astFactory.createGetProp(toDeclareOn, propertyKey()); + break; + } + + decl.setJSDocInfo(jsDocInfo()); + decl = astFactory.exprResult(decl); + return decl; + } + + static Builder builder() { + return new AutoValue_Es6RewriteClass_ClassProperty.Builder(); + } + + @AutoValue.Builder + abstract static class Builder { + abstract Builder propertyKey(String value); + + abstract Builder kind(PropertyKind value); + + abstract Builder jsDocInfo(JSDocInfo value); + + abstract ClassProperty build(); + } + } + /** * Represents static metadata on a class declaration expression - i.e. the qualified name that a * class declares (directly or by assignment), whether it's anonymous, and where transpiled code @@ -645,14 +746,11 @@ abstract static class ClassDeclarationMetadata { */ abstract Node getDefinePropertiesObjForClass(); - // Normal declarations to be added to the prototype: Foo.prototype.bar - abstract Map getPrototypeMembersToDeclare(); + // Property declarations to be added to the prototype + abstract Map getPrototypeMembersToDeclare(); - // Computed property declarations to be added to the prototype: Foo.prototype[bar] - abstract Map getPrototypeComputedPropsToDeclare(); - - // Normal declarations to be added to the class: Foo.bar - abstract Map getClassMembersToDeclare(); + // Property declarations to be added to the class + abstract Map getClassMembersToDeclare(); /** * The fully qualified name of the class, as a cloneable node. May come from the class itself or @@ -682,12 +780,9 @@ abstract static class Builder { abstract Node getFullClassNameNode(); abstract Builder setPrototypeMembersToDeclare( - Map prototypeMembersToDeclare); - - abstract Builder setPrototypeComputedPropsToDeclare( - Map prototypeComputedPropsToDeclare); + Map prototypeMembersToDeclare); - abstract Builder setClassMembersToDeclare(Map classMembersToDeclare); + abstract Builder setClassMembersToDeclare(Map classMembersToDeclare); abstract Builder setAnonymous(boolean anonymous); @@ -707,8 +802,7 @@ abstract Builder setPrototypeComputedPropsToDeclare( public static Builder builder() { return new AutoValue_Es6RewriteClass_ClassDeclarationMetadata.Builder() .setPrototypeMembersToDeclare(new LinkedHashMap<>()) - .setClassMembersToDeclare(new LinkedHashMap<>()) - .setPrototypeComputedPropsToDeclare(new LinkedHashMap<>()); + .setClassMembersToDeclare(new LinkedHashMap<>()); } /** diff --git a/test/com/google/javascript/jscomp/Es6RewriteClassTest.java b/test/com/google/javascript/jscomp/Es6RewriteClassTest.java index 9e878357b92..4b0474f28c1 100644 --- a/test/com/google/javascript/jscomp/Es6RewriteClassTest.java +++ b/test/com/google/javascript/jscomp/Es6RewriteClassTest.java @@ -2819,6 +2819,88 @@ public void testComputedSuper_hasCorrectSourceInfo() { assertNode(thisNode).hasToken(Token.THIS).hasEqualSourceInfoTo(sourceSuperGet); } + @Test + public void testStringKeyGetterAndSetter() { + setLanguageOut(LanguageMode.ECMASCRIPT5); + + // Can't add a property to a struct + disableTypeCheck(); + disableTypeInfoValidation(); + + test( + lines( + "class MyClass {", + " /** @param {number} v */", + " set 'setter'(v) {}", + " /** @return {number} */", + " get 'getter'() { return 1; }", + "}"), + lines( + "/** @constructor @struct */", + "let MyClass=function(){};", + "/** @type {number} */", + "MyClass.prototype['setter'];", + "/** @type {number} */", + "MyClass.prototype['getter'];", + "$jscomp.global.Object.defineProperties(", + " MyClass.prototype,", + " {", + " 'setter': {", + " configurable:true,", + " enumerable:true,", + " /** @this {MyClass} @param {number} v */", + " set:function(v){ }", + " },", + " 'getter': {", + " configurable:true,", + " enumerable:true,", + " /** @this {MyClass} @return {number} */", + " get:function(){ return 1; }", + " },", + " })")); + } + + @Test + public void testNumericGetterAndSetter() { + setLanguageOut(LanguageMode.ECMASCRIPT5); + + // Can't add a property to a struct + disableTypeCheck(); + disableTypeInfoValidation(); + + test( + lines( + "class MyClass {", + " /** @param {number} v */", + " set 1(v) {}", + " /** @return {number} */", + " get 1.5() { return 1; }", + "}"), + lines( + "/** @constructor @struct */", + "let MyClass=function(){};", + "/** @type {number} */", + "MyClass.prototype['1'];", + "/** @type {number} */", + "MyClass.prototype['1.5'];", + "$jscomp.global.Object.defineProperties(", + " MyClass.prototype,", + " {", + " 1: {", + " configurable:true,", + " enumerable:true,", + " /** @this {MyClass} @param {number} v */", + " set:function(v){ }", + " },", + " 1.5: {", + " configurable:true,", + " enumerable:true,", + " /** @this {MyClass} @return {number} */", + " get:function(){ return 1; }", + " },", + " })")); + } + /** Returns the first node (preorder) in the given AST labeled as {@code label} */ private Node getNodeMatchingLabel(Node root, String label) { if (root.isLabel() && root.getFirstChild().getString().equals(label)) {