Skip to content

Commit

Permalink
Resubmit of cl/91210839.
Browse files Browse the repository at this point in the history 
Add type information to nodes that are added when transpiling getters/setters.

The original version of this CL broke code that used getters and setters inside goog.module or goog.scope files, because the same Node was used in JSTypeExpressions in JSDocInfo on two different nodes, so the ScopedAliases pass tried to rewrite the same node twice.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=91264548
  • Loading branch information
@blickly
tbreisacher authored and blickly committed Apr 16, 2015
1 parent e61739f commit 077f7a9
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 24 deletions.
73 changes: 54 additions & 19 deletions src/com/google/javascript/jscomp/Es6ToEs3Converter.java
View file
Expand Up @@ -16,7 +16,7 @@
package com.google.javascript.jscomp;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
import com.google.javascript.rhino.IR;
Expand All @@ -27,7 +27,10 @@
import com.google.javascript.rhino.Token;

import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;

/**
* Converts ES6 code to valid ES3 code.
Expand All @@ -54,6 +57,10 @@ public final class Es6ToEs3Converter implements NodeTraversal.Callback, HotSwapC
"CLASS_REASSIGNMENT",
"Class names defined inside a function cannot be reassigned.");

static final DiagnosticType CONFLICTING_GETTER_SETTER_TYPE = DiagnosticType.error(
"CONFLICTING_GETTER_SETTER_TYPE",
"The types of the getter and setter for property ''{0}'' do not match.");

// The name of the vars that capture 'this' and 'arguments'
// for converting arrow functions.
private static final String THIS_VAR = "$jscomp$this";
Expand Down Expand Up @@ -677,9 +684,11 @@ private void visitObjectWithComputedProperty(Node obj, Node parent) {

/**
* Classes are processed in 3 phases:
* 1) The class name is extracted.
* 2) Class members are processed and rewritten.
* 3) The constructor is built.
* <ol>
* <li>The class name is extracted.
* <li>Class members are processed and rewritten.
* <li>The constructor is built.
* </ol>
*/
private void visitClass(Node classNode, Node parent) {
checkClassReassignment(classNode);
Expand Down Expand Up @@ -709,7 +718,7 @@ private void visitClass(Node classNode, Node parent) {
JSDocInfo ctorJSDocInfo = null;
// Process all members of the class
Node classMembers = classNode.getLastChild();
ImmutableSet.Builder<String> membersToDeclare = ImmutableSet.builder();
Map<String, JSTypeExpression> membersToDeclare = new LinkedHashMap<>();
for (Node member : classMembers.children()) {
if (member.isEmpty()) {
continue;
Expand All @@ -720,8 +729,15 @@ private void visitClass(Node classNode, Node parent) {
"Member variables should have been transpiled earlier: ", member);

if (member.isGetterDef() || member.isSetterDef()) {
JSTypeExpression typeExpr = getTypeFromGetterOrSetter(member).clone();
addToDefinePropertiesObject(metadata, member);
membersToDeclare.add(member.getString());

JSTypeExpression existingType = membersToDeclare.get(member.getString());
if (existingType != null && !existingType.equals(typeExpr)) {
compiler.report(JSError.make(member, CONFLICTING_GETTER_SETTER_TYPE));
} else {
membersToDeclare.put(member.getString(), typeExpr);
}
} else if (member.isMemberFunctionDef() && member.getString().equals("constructor")) {
ctorJSDocInfo = member.getJSDocInfo();
constructor = member.getFirstChild().detachFromParent();
Expand Down Expand Up @@ -760,16 +776,14 @@ private void visitClass(Node classNode, Node parent) {
// so that the typechecker knows those properties exist on the class.
// This is a temporary solution. Eventually, the type checker should understand
// Object.defineProperties calls directly.
for (String declaredMember : membersToDeclare.build()) {
for (Map.Entry<String, JSTypeExpression> entry : membersToDeclare.entrySet()) {
String declaredMember = entry.getKey();
Node declaration = IR.getprop(
prototypeAccess.cloneTree(),
IR.string(declaredMember));
JSDocInfoBuilder declInfo = new JSDocInfoBuilder(true);

// TODO(tbreisacher): Use the type information from the getter/setter instead of just "?"
// and warn if the getter and a setter disagree on the type.
declInfo.recordType(new JSTypeExpression(
new Node(Token.QMARK), classNode.getSourceFileName()));
declInfo.recordType(entry.getValue());
declaration.setJSDocInfo(declInfo.build());
metadata.insertStaticMember(
IR.exprResult(declaration).useSourceInfoIfMissingFromForTree(classNode));
Expand Down Expand Up @@ -825,8 +839,8 @@ private void visitClass(Node classNode, Node parent) {
}

// Classes are @struct by default.
if (!newInfo.isUnrestrictedRecorded() && !newInfo.isDictRecorded() &&
!newInfo.isStructRecorded()) {
if (!newInfo.isUnrestrictedRecorded() && !newInfo.isDictRecorded()
&& !newInfo.isStructRecorded()) {
newInfo.recordStruct();
}

Expand Down Expand Up @@ -868,22 +882,43 @@ private void visitClass(Node classNode, Node parent) {
compiler.reportCodeChange();
}

/**
* @param node A getter or setter node.
*/
private JSTypeExpression getTypeFromGetterOrSetter(Node node) {
JSDocInfo info = node.getJSDocInfo();

if (info != null) {
if (node.isGetterDef()) {
return info.getReturnType();
} else {
Set<String> paramNames = info.getParameterNames();
if (paramNames.size() == 1) {
return info.getParameterType(Iterables.getOnlyElement(info.getParameterNames()));
}
}
}

return new JSTypeExpression(new Node(Token.QMARK), node.getSourceFileName());
}

private void addToDefinePropertiesObject(ClassDeclarationMetadata metadata, Node member) {
Node prop = NodeUtil.getFirstPropMatchingKey(metadata.defineProperties, member.getString());
if (prop == null) {
prop = IR.objectlit();
metadata.defineProperties.addChildToBack(IR.stringKey(member.getString(), prop));
}

Node function = member.getLastChild().detachFromParent();
JSDocInfoBuilder builder = JSDocInfoBuilder.maybeCopyFrom(
Node function = member.getLastChild();
JSDocInfoBuilder info = JSDocInfoBuilder.maybeCopyFrom(
NodeUtil.getBestJSDocInfo(function));

builder.recordThisType(new JSTypeExpression(new Node(
info.recordThisType(new JSTypeExpression(new Node(
Token.BANG, IR.string(metadata.fullClassName)), member.getSourceFileName()));
Node stringKey = IR.stringKey(member.isGetterDef() ? "get" : "set", function);
JSDocInfo info = builder.build();
stringKey.setJSDocInfo(info);
Node stringKey = IR.stringKey(
member.isGetterDef() ? "get" : "set",
function.detachFromParent());
stringKey.setJSDocInfo(info.build());
prop.addChildToBack(stringKey);
prop.useSourceInfoIfMissingFromForTree(member);
}
Expand Down
5 changes: 5 additions & 0 deletions src/com/google/javascript/rhino/JSTypeExpression.java
View file
Expand Up @@ -127,4 +127,9 @@ public Node getRoot() {
public String toString() {
return "type: " + root.toString();
}

@Override
public JSTypeExpression clone() {
return new JSTypeExpression(root.cloneTree(), sourceName);
}
}
70 changes: 65 additions & 5 deletions test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java
View file
Expand Up @@ -846,10 +846,10 @@ public void testInvalidClassUse() {
}

/**
* If languageOut is ES5, getters/setters in object literals are supported.
* Getters/setters in classes will be supported (b/19735276) but are not yet.
* Getters and setters are supported, both in object literals and in classes, but only
* if the output language is ES5.
*/
public void testClassGetterSetter() {
public void testEs5GettersAndSettersClasses() {
languageOut = LanguageMode.ECMASCRIPT5;

test("class C { get value() { return 0; } }", Joiner.on('\n').join(
Expand Down Expand Up @@ -942,6 +942,66 @@ public void testClassGetterSetter() {
"});"));
}

/**
* Check that the types from the getter/setter are copied to the declaration on the prototype.
*/
public void testEs5GettersAndSettersClassesWithTypes() {
languageOut = LanguageMode.ECMASCRIPT5;

test(Joiner.on('\n').join(
"class C {",
" /** @return {number} */",
" get value() { return 0; }",
"}"),

Joiner.on('\n').join(
"/** @constructor @struct */",
"var C = function() {};",
"/** @type {number} */",
"C.prototype.value;",
"Object.defineProperties(C.prototype, {",
" value: {",
" /**",
" * @return {number}",
" * @this {C}",
" */",
" get: function() {",
" return 0;",
" }",
" }",
"});"));

test(Joiner.on('\n').join(
"class C {",
" /** @param {string} v */",
" set value(v) { }",
"}"),

Joiner.on('\n').join(
"/** @constructor @struct */",
"var C = function() {};",
"/** @type {string} */",
"C.prototype.value;",
"Object.defineProperties(C.prototype, {",
" value: {",
" /**",
" * @this {C}",
" * @param {string} v",
" */",
" set: function(v) {}",
" }",
"});"));

testError(Joiner.on('\n').join(
"class C {",
" /** @return {string} */",
" get value() { }",
"",
" /** @param {number} v */",
" set value(v) { }",
"}"), Es6ToEs3Converter.CONFLICTING_GETTER_SETTER_TYPE);
}

/**
* Computed property getters and setters in classes are not supported.
*/
Expand All @@ -961,9 +1021,9 @@ public void testEs5GettersAndSetters_es3() {
}

/**
* ES5 getters and setters should be left alone if the languageOut is ES5.
* ES5 getters and setters on object literals should be left alone if the languageOut is ES5.
*/
public void testEs5GettersAndSetters_es5() {
public void testEs5GettersAndSettersObjLit_es5() {
languageOut = LanguageMode.ECMASCRIPT5;
testSame("var x = { get y() {} };");
testSame("var x = { set y(value) {} };");
Expand Down

0 comments on commit 077f7a9

Please sign in to comment.