From 7383b312b8e494a9a06fe9ba4a0edda37316f146 Mon Sep 17 00:00:00 2001 From: lharker Date: Fri, 3 Nov 2017 16:56:46 -0700 Subject: [PATCH] Replace accesses to inherited ES6 superclass static properties with access 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 --- .../jscomp/AggressiveInlineAliases.java | 133 ++++++++++++++---- .../javascript/jscomp/GlobalNamespace.java | 53 +++++++ .../google/javascript/jscomp/NodeUtil.java | 2 +- .../jscomp/AggressiveInlineAliasesTest.java | 110 ++++++++++++++- 4 files changed, 268 insertions(+), 30 deletions(-) diff --git a/src/com/google/javascript/jscomp/AggressiveInlineAliases.java b/src/com/google/javascript/jscomp/AggressiveInlineAliases.java index f4ffa88b6a5..506ea06bf5c 100644 --- a/src/com/google/javascript/jscomp/AggressiveInlineAliases.java +++ b/src/com/google/javascript/jscomp/AggressiveInlineAliases.java @@ -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. * *

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 @@ -63,35 +64,45 @@ private void rewriteAliasProps(Name name, Node value, int depth, Set value, name.getFullName()); for (Name prop : name.props) { - rewriteAliasProps(prop, value, depth + 1, newNodes); - List 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 newNodes, Name prop) { + rewriteAliasProps(prop, value, depth + 1, newNodes); + List 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)); } } @@ -167,6 +178,17 @@ private void inlineAliases(GlobalNamespace namespace) { } } + if (!name.inExterns && name.type == Name.Type.CLASS) { + List 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) @@ -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 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. diff --git a/src/com/google/javascript/jscomp/GlobalNamespace.java b/src/com/google/javascript/jscomp/GlobalNamespace.java index e21c6d30a16..acfb7de0c84 100644 --- a/src/com/google/javascript/jscomp/GlobalNamespace.java +++ b/src/com/google/javascript/jscomp/GlobalNamespace.java @@ -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++); @@ -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 @@ -967,6 +1008,9 @@ enum Type { /** All references to a name. This must contain {@code declaration}. */ private List refs; + /** All Es6 subclasses of a name that is an Es6 class. Must be null if not an ES6 class. */ + @Nullable List subclasses; + Type type; private boolean declaredType = false; private boolean isDeclared = false; @@ -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; } diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index 8b54629dbfa..ff960a72148 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -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(); diff --git a/test/com/google/javascript/jscomp/AggressiveInlineAliasesTest.java b/test/com/google/javascript/jscomp/AggressiveInlineAliasesTest.java index 500ca4078ad..0a7cd06b2d7 100644 --- a/test/com/google/javascript/jscomp/AggressiveInlineAliasesTest.java +++ b/test/com/google/javascript/jscomp/AggressiveInlineAliasesTest.java @@ -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);")); } }