Skip to content

Commit

Permalink
Have ES6RewriteClass handle getter and setters with quoted keys corre…
Browse files Browse the repository at this point in the history 
…ctly.

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
  • Loading branch information
@blickly
johnplaisted authored and blickly committed Nov 14, 2018
1 parent dbe7217 commit 8d14b51
Show file tree
Hide file tree
Showing 3 changed files with 242 additions and 55 deletions.
11 changes: 11 additions & 0 deletions src/com/google/javascript/jscomp/AstFactory.java
View file
Expand Up @@ -90,6 +90,17 @@ boolean isAddingTypes() {
return registry != null;
}

/**
* Returns a new EXPR_RESULT node.
*
* <p>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.
*
Expand Down
204 changes: 149 additions & 55 deletions src/com/google/javascript/jscomp/Es6RewriteClass.java
View file
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -441,22 +445,30 @@ private void visitNonMethodMember(Node member, ClassDeclarationMetadata metadata
JSTypeExpression typeExpr = getTypeFromGetterOrSetter(member);
addToDefinePropertiesObject(metadata, member);

Map<String, JSDocInfo> membersToDeclare;
Map<String, ClassProperty> 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);
Expand All @@ -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());
}
}

Expand Down Expand Up @@ -515,41 +528,17 @@ private void visitMethod(Node member, ClassDeclarationMetadata metadata) {
*/
private void addTypeDeclarations(
Scope scope, ClassDeclarationMetadata metadata, Node insertionPoint) {
for (Map.Entry<String, JSDocInfo> 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<String, JSDocInfo> 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<String, JSDocInfo> 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;
}
Expand Down Expand Up @@ -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.
*
* <pre>
* class Example {
* 'quoted'() {}
* 42() { return 'the answer'; }
* }
* </pre>
*/
QUOTED_PROPERTY,

/**
* A computed property, e.g. using bracket [] access.
*
* <p>Computed properties *must* currently be qualified names, and not literals, function
* calls, etc.
*
* <pre>
* class Example {
* [variable]() {}
* [another.example]() {}
* }
* </pre>
*/
COMPUTED_PROPERTY,

/**
* A normal property definition.
*
* <pre>
* class Example {
* normal() {}
* }
* </pre>
*/
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.
*
* <p>Examples:
*
* <pre>
* /** @type {string} *\/
* Class.prototype.property;
*
* /** @type {string} *\/
* Class.staticProperty;
* </pre>
*
* @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
Expand All @@ -645,14 +746,11 @@ abstract static class ClassDeclarationMetadata {
*/
abstract Node getDefinePropertiesObjForClass();

// Normal declarations to be added to the prototype: Foo.prototype.bar
abstract Map<String, JSDocInfo> getPrototypeMembersToDeclare();
// Property declarations to be added to the prototype
abstract Map<String, ClassProperty> getPrototypeMembersToDeclare();

// Computed property declarations to be added to the prototype: Foo.prototype[bar]
abstract Map<String, JSDocInfo> getPrototypeComputedPropsToDeclare();

// Normal declarations to be added to the class: Foo.bar
abstract Map<String, JSDocInfo> getClassMembersToDeclare();
// Property declarations to be added to the class
abstract Map<String, ClassProperty> getClassMembersToDeclare();

/**
* The fully qualified name of the class, as a cloneable node. May come from the class itself or
Expand Down Expand Up @@ -682,12 +780,9 @@ abstract static class Builder {
abstract Node getFullClassNameNode();

abstract Builder setPrototypeMembersToDeclare(
Map<String, JSDocInfo> prototypeMembersToDeclare);

abstract Builder setPrototypeComputedPropsToDeclare(
Map<String, JSDocInfo> prototypeComputedPropsToDeclare);
Map<String, ClassProperty> prototypeMembersToDeclare);

abstract Builder setClassMembersToDeclare(Map<String, JSDocInfo> classMembersToDeclare);
abstract Builder setClassMembersToDeclare(Map<String, ClassProperty> classMembersToDeclare);

abstract Builder setAnonymous(boolean anonymous);

Expand All @@ -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<>());
}

/**
Expand Down
82 changes: 82 additions & 0 deletions test/com/google/javascript/jscomp/Es6RewriteClassTest.java
View file
Expand Up @@ -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)) {
Expand Down

0 comments on commit 8d14b51

Please sign in to comment.