Skip to content

Commit

Permalink
Caused Object.definedProperties to be incorrectly optimized/rewritten.
Browse files Browse the repository at this point in the history 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=195468990
  • Loading branch information
@kevinoconnor7 @lauraharker
kevinoconnor7 authored and lauraharker committed May 5, 2018
1 parent 436532d commit 0bbe2a7
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 114 deletions.
69 changes: 41 additions & 28 deletions src/com/google/javascript/jscomp/J2clPropertyInlinerPass.java
View file
Expand Up @@ -25,7 +25,6 @@
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.Map;

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

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

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

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

// Collect static setters and getters.
// These J2CL-created getters and setters always come in pairs. The first time seeing a
// 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()) {
if (!memberFunction.isStaticMember()) {
// The only getters and setters we care about are static.
continue;
}
switch (memberFunction.getToken()) {
case GETTER_DEF:
getterDefByName.put(memberFunction.getString(), memberFunction);
if (matchesJ2clGetKeySignature(className, 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;
case SETTER_DEF:
setterDefByName.put(memberFunction.getString(), memberFunction);
if (matchesJ2clSetKeySignature(className, 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;
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 @@ -383,7 +396,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
if (assignmentTarget.isGetProp()) {
String accessName = assignmentTarget.getQualifiedName();
J2clProperty prop = propertiesByName.get(accessName);
if (prop != null && prop.setKey != null && prop.isSafeToInline) {
if (prop != null && prop.isSafeToInline) {
FunctionInjector injector =
new FunctionInjector(
compiler, compiler.getUniqueNameIdSupplier(), true, true, true);
Expand Down
86 changes: 0 additions & 86 deletions test/com/google/javascript/jscomp/J2clPropertyInlinerPassTest.java
View file
Expand Up @@ -246,36 +246,6 @@ public void testInlineGettersInQualifier() {
"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() {
testDoesntChange(
Expand Down Expand Up @@ -349,37 +319,6 @@ public void testNoInlineIncrementGetter() {
"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() {
test(
lines(
Expand Down Expand Up @@ -408,31 +347,6 @@ public void testInlineEs6Getter() {
"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() {
test(
lines(
Expand Down

0 comments on commit 0bbe2a7

Please sign in to comment.