Skip to content

Commit

Permalink
Do not synthesize setters for final static fields.
Browse files Browse the repository at this point in the history 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=195166677
  • Loading branch information
@rluble @lauraharker
rluble authored and lauraharker committed May 2, 2018
1 parent 061ef1b commit c573371
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 41 deletions.
69 changes: 28 additions & 41 deletions src/com/google/javascript/jscomp/J2clPropertyInlinerPass.java
View file
Expand Up @@ -25,6 +25,7 @@
import com.google.javascript.rhino.IR; import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import java.util.HashMap; import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.Map; import java.util.Map;


/** /**
Expand Down Expand Up @@ -95,7 +96,8 @@ private class J2clPropertyEs5 extends J2clProperty {
public J2clPropertyEs5(Node getKey, Node setKey) { public J2clPropertyEs5(Node getKey, Node setKey) {
super(getKey, setKey); super(getKey, setKey);
checkArgument(getKey.isStringKey() && getKey.getString().equals("get"), getKey); checkArgument(getKey.isStringKey() && getKey.getString().equals("get"), getKey);
checkArgument(setKey.isStringKey() && setKey.getString().equals("set"), setKey); checkArgument(
setKey == null || (setKey.isStringKey() && setKey.getString().equals("set")), setKey);
} }


@Override @Override
Expand All @@ -118,17 +120,19 @@ private class J2clPropertyEs6 extends J2clProperty {
public J2clPropertyEs6(Node getKey, Node setKey) { public J2clPropertyEs6(Node getKey, Node setKey) {
super(getKey, setKey); super(getKey, setKey);
checkArgument(getKey.isGetterDef(), getKey); checkArgument(getKey.isGetterDef(), getKey);
checkArgument(setKey.isSetterDef(), setKey); checkArgument(setKey == null || setKey.isSetterDef(), setKey);
} }


@Override @Override
void remove() { void remove() {
Node classMembers = getKey.getParent(); Node classMembers = getKey.getParent();
checkState(classMembers.isClassMembers(), classMembers); checkState(classMembers.isClassMembers(), classMembers);
classMembers.removeChild(getKey); classMembers.removeChild(getKey);
classMembers.removeChild(setKey);
NodeUtil.markFunctionsDeleted(getKey, compiler); NodeUtil.markFunctionsDeleted(getKey, compiler);
NodeUtil.markFunctionsDeleted(setKey, compiler); if (setKey != null) {
classMembers.removeChild(setKey);
NodeUtil.markFunctionsDeleted(setKey, compiler);
}
compiler.reportChangeToEnclosingScope(classMembers); compiler.reportChangeToEnclosingScope(classMembers);
} }
} }
Expand Down Expand Up @@ -238,10 +242,10 @@ void visitObjectDefineProperties(Node n) {
String classNameString = className.getString(); String classNameString = className.getString();
for (Node p : NodeUtil.getObjectDefinedPropertiesKeys(n)) { for (Node p : NodeUtil.getObjectDefinedPropertiesKeys(n)) {
String name = p.getString(); String name = p.getString();
// J2cl static fields are always synthesized with both a getter and setter.
Node propertyLiteral = p.getFirstChild(); Node propertyLiteral = p.getFirstChild();
Node getKey = null; Node getKey = null;
Node setKey = null; Node setKey = null;
boolean hasSetter = false;
for (Node innerKey : propertyLiteral.children()) { for (Node innerKey : propertyLiteral.children()) {
if (!innerKey.isStringKey()) { if (!innerKey.isStringKey()) {
continue; continue;
Expand All @@ -253,14 +257,15 @@ void visitObjectDefineProperties(Node n) {
} }
break; break;
case "set": case "set":
hasSetter = true;
if (matchesJ2clSetKeySignature(classNameString, innerKey)) { if (matchesJ2clSetKeySignature(classNameString, innerKey)) {
setKey = innerKey; setKey = innerKey;
} }
break; break;
default: // fall out default: // fall out
} }
} }
if (getKey != null && setKey != null) { if (getKey != null && (!hasSetter || setKey != null)) {
j2clPropertiesByName.put( j2clPropertiesByName.put(
classNameString + "." + name, new J2clPropertyEs5(getKey, setKey)); classNameString + "." + name, new J2clPropertyEs5(getKey, setKey));
} }
Expand All @@ -271,54 +276,36 @@ void visitClass(Node classNode) {
String className = NodeUtil.getName(classNode); String className = NodeUtil.getName(classNode);
Node classMembers = NodeUtil.getClassMembers(classNode); Node classMembers = NodeUtil.getClassMembers(classNode);


Node getKey = null; Map<String, Node> setterDefByName = new LinkedHashMap<>();
Node setterDef = null; Map<String, Node> getterDefByName = new LinkedHashMap<>();
String name = "";
Map<String, Node> nameToGetterOrSetterDef = new HashMap<>();


// These J2CL-created getters and setters always come in pairs. The first time seeing a // Collect static setters and getters.
// getter/setter for a given name, add it to the nameToGetterOrSetterDef map.
// Upon seeing another getter/setter for that name, create a new J2clProperty for the
// getter/setter pair.
for (Node memberFunction : classMembers.children()) { for (Node memberFunction : classMembers.children()) {
if (!memberFunction.isStaticMember()) { if (!memberFunction.isStaticMember()) {
// The only getters and setters we care about are static. // The only getters and setters we care about are static.
continue; continue;
} }
switch (memberFunction.getToken()) { switch (memberFunction.getToken()) {
case GETTER_DEF: case GETTER_DEF:
if (matchesJ2clGetKeySignature(className, memberFunction)) { getterDefByName.put(memberFunction.getString(), memberFunction);
getKey = memberFunction;
name = memberFunction.getString();
setterDef = nameToGetterOrSetterDef.get(name);

if (setterDef != null) {
j2clPropertiesByName.put(
className + "." + name, new J2clPropertyEs6(getKey, setterDef));
nameToGetterOrSetterDef.remove(name);
} else {
nameToGetterOrSetterDef.put(name, getKey);
}
}
break; break;
case SETTER_DEF: case SETTER_DEF:
if (matchesJ2clSetKeySignature(className, memberFunction)) { setterDefByName.put(memberFunction.getString(), memberFunction);
setterDef = memberFunction;
name = memberFunction.getString();
getKey = nameToGetterOrSetterDef.get(name);

if (getKey != null) {
j2clPropertiesByName.put(
className + "." + name, new J2clPropertyEs6(getKey, setterDef));
nameToGetterOrSetterDef.remove(name);
} else {
nameToGetterOrSetterDef.put(name, setterDef);
}
}
break; break;
default: // fall out default: // fall out
} }
} }

for (String propertyName : getterDefByName.keySet()) {
Node getterDef = getterDefByName.get(propertyName);
Node setterDef = setterDefByName.get(propertyName);
if (matchesJ2clGetKeySignature(className, getterDef)
&& (setterDef == null || matchesJ2clSetKeySignature(className, setterDef))) {
// This is a J2cl static property.
j2clPropertiesByName.put(
className + "." + propertyName, new J2clPropertyEs6(getterDef, setterDef));
}
}
} }
} }


Expand Down Expand Up @@ -396,7 +383,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
if (assignmentTarget.isGetProp()) { if (assignmentTarget.isGetProp()) {
String accessName = assignmentTarget.getQualifiedName(); String accessName = assignmentTarget.getQualifiedName();
J2clProperty prop = propertiesByName.get(accessName); J2clProperty prop = propertiesByName.get(accessName);
if (prop != null && prop.isSafeToInline) { if (prop != null && prop.setKey != null && prop.isSafeToInline) {
FunctionInjector injector = FunctionInjector injector =
new FunctionInjector( new FunctionInjector(
compiler, compiler.getUniqueNameIdSupplier(), true, true, true); compiler, compiler.getUniqueNameIdSupplier(), true, true, true);
Expand Down
86 changes: 86 additions & 0 deletions test/com/google/javascript/jscomp/J2clPropertyInlinerPassTest.java
View file
Expand Up @@ -246,6 +246,36 @@ public void testInlineGettersInQualifier() {
"var xy = (A.$clinit(), A.$x).y;")))); "var xy = (A.$clinit(), A.$x).y;"))));
} }


public void testInlineGettersInQualifierNoSetter() {
test(
Lists.newArrayList(
SourceFile.fromCode(
"someFile.js",
lines(
"var A = function() {};",
"A.$clinit = function() {",
" A.$x = {y: 2};",
"};",
"Object.defineProperties(A, {x: {",
" configurable:true,",
" enumerable:true,",
" get:function() {",
" return A.$clinit(), A.$x;",
" },",
"}});",
"A.$x = null;",
"var xy = A.x.y;"))),
Lists.newArrayList(
SourceFile.fromCode(
"someFile.js",
lines(
"var A = function() {};",
"A.$clinit = function() {",
" A.$x = {y: 2};",
"};",
"A.$x = null;",
"var xy = (A.$clinit(), A.$x).y;"))));
}


public void testNoInlineCompoundAssignment() { public void testNoInlineCompoundAssignment() {
testDoesntChange( testDoesntChange(
Expand Down Expand Up @@ -319,6 +349,37 @@ public void testNoInlineIncrementGetter() {
"A.x++;")))); "A.x++;"))));
} }


public void testNoInlineSetterOnlyGetter() {
test(
Lists.newArrayList(
SourceFile.fromCode(
"someFile.js",
lines(
"var A = function() {};",
"A.$clinit = function() {",
" A.$x = {y: 2};",
"};",
"Object.defineProperties(A, {x: {",
" configurable:true,",
" enumerable:true,",
" get:function() {",
" return A.$clinit(), A.$x;",
" },",
"}});",
"A.$x = null;",
"A.x = null;"))),
Lists.newArrayList(
SourceFile.fromCode(
"someFile.js",
lines(
"var A = function() {};",
"A.$clinit = function() {",
" A.$x = {y: 2};",
"};",
"A.$x = null;",
"A.x = null;"))));
}

public void testInlineEs6Getter() { public void testInlineEs6Getter() {
test( test(
lines( lines(
Expand Down Expand Up @@ -347,6 +408,31 @@ public void testInlineEs6Getter() {
"var xx = (A.$clinit(), A.$x);")); "var xx = (A.$clinit(), A.$x);"));
} }


public void testInlineEs6GetterNoSetter() {
test(
lines(
"class A {",
" static $clinit() {",
" A.$clinit = function() {};",
" A.$x = 2;",
" }",
" static get x() {",
" return A.$clinit(), A.$x",
" }",
"}",
"A.$x = 3;",
"var xx = A.x"),
lines(
"class A {",
" static $clinit() {",
" A.$clinit = function() {};",
" A.$x = 2;",
" }",
"}",
"A.$x = 3;",
"var xx = (A.$clinit(), A.$x);"));
}

public void testInlineEs6Setter() { public void testInlineEs6Setter() {
test( test(
lines( lines(
Expand Down

0 comments on commit c573371

Please sign in to comment.