Skip to content

Commit

Permalink
Replace accesses to inherited ES6 superclass static properties with a…
Browse files Browse the repository at this point in the history
…ccess from the superclass.

This makes collapsing static ES6 class properties safer.

e.g.
class A {}
A.foo = 5;
class B extends A {}
use(B.foo); => use(A.foo);

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=174533724
  • Loading branch information
lauraharker authored and dimvar committed Nov 6, 2017
1 parent b49ef87 commit 7383b31
Show file tree
Hide file tree
Showing 4 changed files with 268 additions and 30 deletions.
133 changes: 107 additions & 26 deletions src/com/google/javascript/jscomp/AggressiveInlineAliases.java
Expand Up @@ -34,7 +34,8 @@
import java.util.Set;

/**
* Inlines type aliases if they are explicitly or effectively const.
* Inlines type aliases if they are explicitly or effectively const. Also inlines inherited static
* property accesses for ES6 classes.
*
* <p>This frees subsequent optimization passes from the responsibility of having to reason about
* alias chains and is a requirement for correct behavior in at least CollapseProperties and
Expand Down Expand Up @@ -63,35 +64,45 @@ private void rewriteAliasProps(Name name, Node value, int depth, Set<AstChange>
value,
name.getFullName());
for (Name prop : name.props) {
rewriteAliasProps(prop, value, depth + 1, newNodes);
List<Ref> refs = new ArrayList<>(prop.getRefs());
for (Ref ref : refs) {
Node target = ref.node;
for (int i = 0; i <= depth; i++) {
if (target.isGetProp()) {
target = target.getFirstChild();
} else if (NodeUtil.isObjectLitKey(target)) {
// Object literal key definitions are a little trickier, as we
// need to find the assignment target
Node gparent = target.getGrandparent();
if (gparent.isAssign()) {
target = gparent.getFirstChild();
} else {
checkState(NodeUtil.isObjectLitKey(gparent));
target = gparent;
}
rewriteAliasProp(value, depth, newNodes, prop);
}
}

/**
* @param value The value to use when rewriting.
* @param depth The chain depth.
* @param newNodes Expression nodes that have been updated.
* @param prop The property to rewrite with value.
*/
private void rewriteAliasProp(Node value, int depth, Set<AstChange> newNodes, Name prop) {
rewriteAliasProps(prop, value, depth + 1, newNodes);
List<Ref> refs = new ArrayList<>(prop.getRefs());
for (Ref ref : refs) {
Node target = ref.node;
for (int i = 0; i <= depth; i++) {
if (target.isGetProp()) {
target = target.getFirstChild();
} else if (NodeUtil.isObjectLitKey(target)) {
// Object literal key definitions are a little trickier, as we
// need to find the assignment target
Node gparent = target.getGrandparent();
if (gparent.isAssign()) {
target = gparent.getFirstChild();
} else {
throw new IllegalStateException("unexpected: " + target);
checkState(NodeUtil.isObjectLitKey(gparent));
target = gparent;
}
} else {
throw new IllegalStateException("unexpected: " + target);
}
checkState(target.isGetProp() || target.isName());
Node newValue = value.cloneTree();
target.replaceWith(newValue);
compiler.reportChangeToEnclosingScope(newValue);
prop.removeRef(ref);
// Rescan the expression root.
newNodes.add(new AstChange(ref.module, ref.scope, ref.node));
}
checkState(target.isGetProp() || target.isName());
Node newValue = value.cloneTree();
target.replaceWith(newValue);
compiler.reportChangeToEnclosingScope(newValue);
prop.removeRef(ref);
// Rescan the expression root.
newNodes.add(new AstChange(ref.module, ref.scope, ref.node));
}
}

Expand Down Expand Up @@ -167,6 +178,17 @@ private void inlineAliases(GlobalNamespace namespace) {
}
}

if (!name.inExterns && name.type == Name.Type.CLASS) {
List<Name> subclasses = name.subclasses;
if (subclasses != null && name.props != null) {
for (Name subclass : subclasses) {
for (Name prop : name.props) {
rewriteAllSubclassInheritedAccesses(name, subclass, prop, namespace);
}
}
}
}

// Check if {@code name} has any aliases left after the
// local-alias-inlining above.
if ((name.type == Name.Type.OBJECTLIT || name.type == Name.Type.FUNCTION)
Expand All @@ -179,6 +201,65 @@ private void inlineAliases(GlobalNamespace namespace) {
}
}

/**
* Inline all references to inherited static superclass properties from the subclass or any
* descendant of the given subclass. Avoids inlining references to inherited methods when
* possible, since they may use this or super().
*
* @param superclassNameObj The Name of the superclass
* @param subclassNameObj The Name of the subclass
* @param prop The property on the superclass to rewrite, if any descendant accesses it.
* @param namespace The GlobalNamespace containing superclassNameObj
*/
private boolean rewriteAllSubclassInheritedAccesses(
Name superclassNameObj, Name subclassNameObj, Name prop, GlobalNamespace namespace) {
Ref propDeclRef = prop.getDeclaration();
if (propDeclRef == null
|| propDeclRef.node == null
|| !propDeclRef.node.getParent().isAssign()) {
return false;
}
Node propRhs = propDeclRef.node.getParent().getLastChild();
if (propRhs.isFunction()) {
return false;
}

String subclassQualifiedPropName = subclassNameObj.getFullName() + "." + prop.getBaseName();
Name subclassPropNameObj = namespace.getOwnSlot(subclassQualifiedPropName);
// Don't rewrite if the subclass ever shadows the parent static property.
// This may also back off on cases where the subclass first accesses the parent property, then
// shadows it.
if (subclassPropNameObj != null
&& (subclassPropNameObj.localSets > 0 || subclassPropNameObj.globalSets > 0)) {
return false;
}

// Recurse to find potential sub-subclass accesses of the superclass property.
if (subclassNameObj.subclasses != null) {
for (Name name : subclassNameObj.subclasses) {
rewriteAllSubclassInheritedAccesses(superclassNameObj, name, prop, namespace);
}
}

if (subclassPropNameObj != null) {
Set<AstChange> newNodes = new LinkedHashSet<>();

// Use this node as a template for rewriteAliasProp.
Node superclassNameNode = superclassNameObj.getDeclaration().node;
if (superclassNameNode.isName()) {
superclassNameNode = superclassNameNode.cloneNode();
} else if (superclassNameNode.isGetProp()) {
superclassNameNode = superclassNameNode.cloneTree();
} else {
return false;
}

rewriteAliasProp(superclassNameNode, 0, newNodes, subclassPropNameObj);
namespace.scanNewNodes(newNodes);
}
return true;
}

/**
* Returns true if the alias is possibly defined in the global scope, which we handle with more
* caution than with locally scoped variables. May return false positives.
Expand Down
53 changes: 53 additions & 0 deletions src/com/google/javascript/jscomp/GlobalNamespace.java
Expand Up @@ -607,6 +607,7 @@ void handleSetFromGlobal(JSModule module, Scope scope,

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

Ref set = new Ref(module, scope, n, nameObj, Ref.Type.SET_FROM_GLOBAL,
currentPreOrderIndex++);
Expand All @@ -624,6 +625,46 @@ void handleSetFromGlobal(JSModule module, Scope scope,
}
}

/**
* Given a new node and its name that is an ES6 class, checks if it is an ES6 class with an ES6
* superclass. If the superclass is a simple or qualified names, adds itself to the parent's
* list of subclasses. Otherwise this does nothing.
*
* @param n The node being visited.
* @param parent {@code n}'s parent
* @param subclassNameObj The Name of the new node being visited.
*/
private void maybeRecordEs6Subclass(Node n, Node parent, Name subclassNameObj) {
if (subclassNameObj.type != Name.Type.CLASS || parent == null) {
return;
}

Node superclass = null;
if (parent.isClass()) {
superclass = parent.getSecondChild();
} else {
Node classNode = NodeUtil.getAssignedValue(n);
if (classNode != null && classNode.isClass()) {
superclass = classNode.getSecondChild();
}
}
// If there's no superclass, or the superclass expression is more complicated than a simple
// or qualified name, return.
if (superclass == null
|| superclass.isEmpty()
|| !(superclass.isName() || superclass.isGetProp())) {
return;
}
String superclassName =
superclass.isName() ? superclass.getString() : superclass.getQualifiedName();

Name superclassNameObj = getOrCreateName(superclassName, false);
// If the superclass is an ES3/5 class we don't record its subclasses.
if (superclassNameObj != null && superclassNameObj.type == Name.Type.CLASS) {
superclassNameObj.addSubclass(subclassNameObj);
}
}

/**
* Determines whether a set operation is a constructor or enumeration
* or interface declaration. The set operation may either be an assignment
Expand Down Expand Up @@ -967,6 +1008,9 @@ enum Type {
/** All references to a name. This must contain {@code declaration}. */
private List<Ref> refs;

/** All Es6 subclasses of a name that is an Es6 class. Must be null if not an ES6 class. */
@Nullable List<Name> subclasses;

Type type;
private boolean declaredType = false;
private boolean isDeclared = false;
Expand Down Expand Up @@ -999,6 +1043,15 @@ Name addProperty(String name, boolean inExterns, boolean shouldCreateProp) {
return node;
}

Name addSubclass(Name subclassName) {
checkArgument(this.type == Type.CLASS && subclassName.type == Type.CLASS);
if (subclasses == null) {
subclasses = new ArrayList<>();
}
subclasses.add(subclassName);
return subclassName;
}

String getBaseName() {
return baseName;
}
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/NodeUtil.java
Expand Up @@ -2418,7 +2418,7 @@ static boolean isDestructuringDeclaration(Node n) {
* @return The value node representing the new value.
*/
public static Node getAssignedValue(Node n) {
checkState(n.isName(), n);
checkState(n.isName() || n.isGetProp(), n);
Node parent = n.getParent();
if (NodeUtil.isNameDeclaration(parent)) {
return n.getFirstChild();
Expand Down
110 changes: 107 additions & 3 deletions test/com/google/javascript/jscomp/AggressiveInlineAliasesTest.java
Expand Up @@ -914,11 +914,115 @@ public void testClassStaticInheritance_method() {
test(
"class A { static s() {} } class B extends A {} const C = B; C.s();",
"class A { static s() {} } class B extends A {} const C = null; B.s();");

testSame("class A { static s() {} } class B extends A {} B.s();");
testSame("class A {} A.s = function() {}; class B extends A {} B.s();");
}

public void testClassStaticInheritance_propertyAlias() {
testSame("class A {} A.staticProp = 6; class B extends A {} let b = new B;");

test(
"class A {} A.staticProp = 6; class B extends A {} use(B.staticProp);",
"class A {} A.staticProp = 6; class B extends A {} use(A.staticProp);");
}

public void testClassStaticInheritance_classExpression() {
test(
"var A = class {}; A.staticProp = 6; var B = class extends A {}; use(B.staticProp);",
"var A = class {}; A.staticProp = 6; var B = class extends A {}; use(A.staticProp);");

test(
"var A; A = class {}; A.staticProp = 6; var B = class extends A {}; use(B.staticProp);",
"var A; A = class {}; A.staticProp = 6; var B = class extends A {}; use(A.staticProp);");
test(
"let A = class {}; A.staticProp = 6; let B = class extends A {}; use(B.staticProp);",
"let A = class {}; A.staticProp = 6; let B = class extends A {}; use(A.staticProp);");

test(
"const A = class {}; A.staticProp = 6; const B = class extends A {}; use(B.staticProp);",
"const A = class {}; A.staticProp = 6; const B = class extends A {}; use(A.staticProp);");
}

public void testClassStaticInheritance_propertyWithSubproperty() {
test(
"class A {} A.ns = {foo: 'bar'}; class B extends A {} use(B.ns.foo);",
"class A {} A.ns = {foo: 'bar'}; class B extends A {} use(A.ns.foo);");

test(
"class A {} A.ns = {foo: 'bar'}; class B extends A {} B.ns.foo = 'baz'; use(B.ns.foo);",
"class A {} A.ns = {foo: 'bar'}; class B extends A {} A.ns.foo = 'baz'; use(A.ns.foo);");
}

public void testClassStaticInheritance_propertyWithShadowing() {
testSame("class A {} A.staticProp = 6; class B extends A {} B.staticProp = 7;");

// Here, B.staticProp is a different property from A.staticProp, so don't rewrite.
testSame(
"class A {} A.staticProp = 6; class B extends A {} B.staticProp = 7; use(B.staticProp);");

// At the time use() is called, B.staticProp is still the same as A.staticProp, so we
// *could* rewrite it. But instead we back off because of the shadowing afterwards.
testSame(
"class A {} A.staticProp = 6; class B extends A {} use(B.staticProp); B.staticProp = 7;");
}

public void testClassStaticInheritance_propertyMultiple() {
test(
"class A {} A.foo = 5; A.bar = 6; class B extends A {} use(B.foo); use(B.bar);",
"class A {} A.foo = 5; A.bar = 6; class B extends A {} use(A.foo); use(A.bar);");

testSame("class A {} A.foo = {bar: 5}; A.baz = 6; class B extends A {} B.baz = 7;");

test(
"class A {} A.foo = {}; A.baz = 6; class B extends A {} B.foo.bar = 5; B.baz = 7;",
"class A {} A.foo = {}; A.baz = 6; class B extends A {} A.foo.bar = 5; B.baz = 7;");
}

public void testClassStaticInheritance_property_chainedSubclasses() {
test(
"class A {} A.foo = 5; class B extends A {} class C extends B {} use(C.foo);",
"class A {} A.foo = 5; class B extends A {} class C extends B {} use(A.foo);");
}

public void testClassStaticInheritance_property() {
public void testClassStaticInheritance_namespacedClass() {
test(
"class A {} A.staticProp = 5; class B extends A {} const C = B; C.staticProp = 6;",
"class A {} A.staticProp = 5; class B extends A {} const C = null; B.staticProp = 6;");
LINE_JOINER.join(
"var ns1 = {}, ns2 = {};",
"ns1.A = class {};",
"ns1.A.staticProp = {foo: 'bar'};",
"ns2.B = class extends ns1.A {}",
"use(ns2.B.staticProp.bar);"),
LINE_JOINER.join(
"var ns1 = {}, ns2 = {};",
"ns1.A = class {};",
"ns1.A.staticProp = {foo: 'bar'};",
"ns2.B = class extends ns1.A {}",
"use(ns1.A.staticProp.bar);"));
}

public void testClassStaticInheritance_es5Class() {
// ES6 classes do not inherit static properties of ES5 class constructors.
testSame(
LINE_JOINER.join(
"/** @constructor */",
"function A() {}",
"A.staticProp = 5;",
"class B extends A {}",
"use(B.staticProp);")); // undefined
}

public void testClassStaticInheritance_cantDetermineSuperclass() {
// Currently we only inline inherited properties when the extends clause contains a simple or
// qualified name.
testSame(
LINE_JOINER.join(
"class A {}",
"A.foo = 5;",
"class B {}",
"B.foo = 6;",
"function getSuperclass() { return A; }",
"class C extends getSuperclass() {}",
"use(C.foo);"));
}
}

0 comments on commit 7383b31

Please sign in to comment.