Skip to content

Commit

Permalink
Make collapse properties pass recognize and avoid collapsing class st…
Browse files Browse the repository at this point in the history 
…atic and instance method calls

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=164994860
  • Loading branch information
@simran-arora @blickly
simran-arora authored and blickly committed Aug 11, 2017
1 parent 3687f2a commit 68ed407
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 45 deletions.
56 changes: 40 additions & 16 deletions src/com/google/javascript/jscomp/GlobalNamespace.java
View file
Expand Up @@ -309,6 +309,7 @@ public void collect(JSModule module, Scope scope, Node n) {
boolean isSet = false; boolean isSet = false;
Name.Type type = Name.Type.OTHER; Name.Type type = Name.Type.OTHER;
boolean isPropAssign = false; boolean isPropAssign = false;
boolean shouldCreateProp = true;


switch (n.getToken()) { switch (n.getToken()) {
case GETTER_DEF: case GETTER_DEF:
Expand All @@ -328,6 +329,11 @@ public void collect(JSModule module, Scope scope, Node n) {
isSet = true; isSet = true;
switch (n.getToken()) { switch (n.getToken()) {
case MEMBER_FUNCTION_DEF: case MEMBER_FUNCTION_DEF:
type = getValueType(n.getFirstChild());
if (n.getParent().isClassMembers() && !n.isStaticMember()) {
shouldCreateProp = false;
}
break;
case STRING_KEY: case STRING_KEY:
type = getValueType(n.getFirstChild()); type = getValueType(n.getFirstChild());
break; break;
Expand Down Expand Up @@ -432,9 +438,9 @@ public void collect(JSModule module, Scope scope, Node n) {
scope = scope.getClosestHoistScope(); scope = scope.getClosestHoistScope();
if (isSet) { if (isSet) {
if (scope.isGlobal()) { if (scope.isGlobal()) {
handleSetFromGlobal(module, scope, n, parent, name, isPropAssign, type); handleSetFromGlobal(module, scope, n, parent, name, isPropAssign, type, shouldCreateProp);
} else { } else {
handleSetFromLocal(module, scope, n, parent, name); handleSetFromLocal(module, scope, n, parent, name, shouldCreateProp);
} }
} else { } else {
handleGet(module, scope, n, parent, name); handleGet(module, scope, n, parent, name);
Expand Down Expand Up @@ -587,12 +593,12 @@ Name.Type getValueType(Node n) {
*/ */
void handleSetFromGlobal(JSModule module, Scope scope, void handleSetFromGlobal(JSModule module, Scope scope,
Node n, Node parent, String name, Node n, Node parent, String name,
boolean isPropAssign, Name.Type type) { boolean isPropAssign, Name.Type type, boolean shouldCreateProp) {
if (maybeHandlePrototypePrefix(module, scope, n, parent, name)) { if (maybeHandlePrototypePrefix(module, scope, n, parent, name)) {
return; return;
} }


Name nameObj = getOrCreateName(name); Name nameObj = getOrCreateName(name, shouldCreateProp);
nameObj.type = type; nameObj.type = type;


Ref set = new Ref(module, scope, n, nameObj, Ref.Type.SET_FROM_GLOBAL, Ref set = new Ref(module, scope, n, nameObj, Ref.Type.SET_FROM_GLOBAL,
Expand Down Expand Up @@ -642,12 +648,12 @@ private boolean isTypeDeclaration(Node n) {
* @param name The global name (e.g. "a" or "a.b.c.d") * @param name The global name (e.g. "a" or "a.b.c.d")
*/ */
void handleSetFromLocal(JSModule module, Scope scope, Node n, Node parent, void handleSetFromLocal(JSModule module, Scope scope, Node n, Node parent,
String name) { String name, boolean shouldCreateProp) {
if (maybeHandlePrototypePrefix(module, scope, n, parent, name)) { if (maybeHandlePrototypePrefix(module, scope, n, parent, name)) {
return; return;
} }


Name nameObj = getOrCreateName(name); Name nameObj = getOrCreateName(name, shouldCreateProp);
Ref set = new Ref(module, scope, n, nameObj, Ref set = new Ref(module, scope, n, nameObj,
Ref.Type.SET_FROM_LOCAL, currentPreOrderIndex++); Ref.Type.SET_FROM_LOCAL, currentPreOrderIndex++);
nameObj.addRef(set); nameObj.addRef(set);
Expand Down Expand Up @@ -729,7 +735,7 @@ void handleGet(JSModule module, Scope scope,
break; break;
} }


handleGet(module, scope, n, parent, name, type); handleGet(module, scope, n, parent, name, type, true);
} }


private boolean isClassDefiningCall(Node callNode) { private boolean isClassDefiningCall(Node callNode) {
Expand Down Expand Up @@ -822,8 +828,8 @@ Ref.Type determineGetTypeForHookOrBooleanExpr(
* @param type The reference type * @param type The reference type
*/ */
void handleGet(JSModule module, Scope scope, Node n, Node parent, void handleGet(JSModule module, Scope scope, Node n, Node parent,
String name, Ref.Type type) { String name, Ref.Type type, boolean shouldCreateProp) {
Name nameObj = getOrCreateName(name); Name nameObj = getOrCreateName(name, shouldCreateProp);


// No need to look up additional ancestors, since they won't be used. // No need to look up additional ancestors, since they won't be used.
nameObj.addRef( nameObj.addRef(
Expand Down Expand Up @@ -878,7 +884,7 @@ boolean maybeHandlePrototypePrefix(JSModule module, Scope scope,
n = n.getFirstChild(); n = n.getFirstChild();
} }


handleGet(module, scope, n, parent, prefix, Ref.Type.PROTOTYPE_GET); handleGet(module, scope, n, parent, prefix, Ref.Type.PROTOTYPE_GET, true);
return true; return true;
} }


Expand All @@ -901,14 +907,14 @@ boolean isNestedAssign(Node parent) {
* @param name A global name (e.g. "a", "a.b.c.d") * @param name A global name (e.g. "a", "a.b.c.d")
* @return The {@link Name} instance for {@code name} * @return The {@link Name} instance for {@code name}
*/ */
Name getOrCreateName(String name) { Name getOrCreateName(String name, boolean shouldCreateProp) {
Name node = nameMap.get(name); Name node = nameMap.get(name);
if (node == null) { if (node == null) {
int i = name.lastIndexOf('.'); int i = name.lastIndexOf('.');
if (i >= 0) { if (i >= 0) {
String parentName = name.substring(0, i); String parentName = name.substring(0, i);
Name parent = getOrCreateName(parentName); Name parent = getOrCreateName(parentName, true);
node = parent.addProperty(name.substring(i + 1), inExterns); node = parent.addProperty(name.substring(i + 1), inExterns, shouldCreateProp);
} else { } else {
node = new Name(name, null, inExterns); node = new Name(name, null, inExterns);
globalNames.add(node); globalNames.add(node);
Expand Down Expand Up @@ -971,12 +977,14 @@ enum Type {
this.inExterns = inExterns; this.inExterns = inExterns;
} }


Name addProperty(String name, boolean inExterns) { Name addProperty(String name, boolean inExterns, boolean shouldCreateProp) {
if (props == null) { if (props == null) {
props = new ArrayList<>(); props = new ArrayList<>();
} }
Name node = new Name(name, this, inExterns); Name node = new Name(name, this, inExterns);
props.add(node); if (shouldCreateProp) {
props.add(node);
}
return node; return node;
} }


Expand Down Expand Up @@ -1184,7 +1192,23 @@ boolean canCollapse() {
|| ((parent == null || parent.canCollapseUnannotatedChildNames()) || ((parent == null || parent.canCollapseUnannotatedChildNames())
&& (globalSets > 0 || localSets > 0) && (globalSets > 0 || localSets > 0)
&& localSetsWithNoCollapse == 0 && localSetsWithNoCollapse == 0
&& deleteProps == 0)); && deleteProps == 0))
&& !isStaticClassMemberFunction();
}

boolean isStaticClassMemberFunction() {
// TODO (simranarora) eventually we want to be able to collapse for static class member
// function declarations and get rid of this method. We need to be careful about handling
// super and this so we back off for now and decided not to collapse static class methods.

Ref ref = this.getDeclaration();
if (ref != null) {
Node n = ref.getNode();
if (n != null && n.isStaticMember() && n.getParent().isClassMembers()) {
return true;
}
}
return false;
} }


boolean isGetOrSetDefinition() { boolean isGetOrSetDefinition() {
Expand Down
63 changes: 34 additions & 29 deletions test/com/google/javascript/jscomp/CollapsePropertiesTest.java
View file
Expand Up @@ -1700,7 +1700,27 @@ public void testComputedPropertyNames() {
"bar.foo();")); "bar.foo();"));
} }


public void testClassProperties() { public void testClassGetSetMembers() {
// Get and set methods
testSame(
LINE_JOINER.join(
"class Bar {",
" constructor(x) {",
" this.x = x;",
" }",
" get foo() {",
" return this.x;",
" }",
" set foo(xIn) {",
" x = xIn;",
" }",
"}",
"var barObj = new Bar(1);",
"bar.foo();",
"bar.foo(2);"));
}

public void testClassNonStaticMembers() {
// Call class method inside class scope // Call class method inside class scope
testSame( testSame(
LINE_JOINER.join( LINE_JOINER.join(
Expand All @@ -1726,48 +1746,33 @@ public void testClassProperties() {
"var too = new Bar();", "var too = new Bar();",
"too.getB(too);")); "too.getB(too);"));


// Get and set methods // Non-static method
testSame( testSame(
LINE_JOINER.join( LINE_JOINER.join(
"class Bar {", "class Bar {",
" constructor(x) {", " constructor(x){",
" this.x = x;", " this.x = x;",
" }", " }",
" get foo() {", " double() {",
" return this.x;", " return x*2;",
" }",
" set foo(xIn) {",
" x = xIn;",
" }", " }",
"}", "}",
"var barObj = new Bar(1);", "var too;",
"bar.foo();", "var too = new Bar();",
"bar.foo(2);")); "too.double(1);"));
}


// Static methods public void testClassStaticMembers() {
/*testSame( // TODO (simranarora) Make the pass collapse for static methods. Currently we have backed off
LINE_JOINER.join( // because we will need to handle super and this occurrences within the method.
"class Bar {", testSame(
" static double(n) {",
" return n*2",
" }",
"}",
"Bar.double(1);"));*/
test(
LINE_JOINER.join(
"class Bar {",
" static double(n) {",
" return n*2",
" }",
"}",
"Bar.double(1);"),
LINE_JOINER.join( LINE_JOINER.join(
"class Bar {", "class Bar {",
" static double(n) {", " static double(n) {",
" return n*2", " return n*2",
" }", " }",
"}", "}",
"Bar$double(1);")); "Bar.double(1);"));
} }


public void testSuperExtern() { public void testSuperExtern() {
Expand Down

0 comments on commit 68ed407

Please sign in to comment.