diff --git a/src/com/google/javascript/jscomp/InlineProperties.java b/src/com/google/javascript/jscomp/InlineProperties.java index 29670d9de4e..a03dcb001ae 100644 --- a/src/com/google/javascript/jscomp/InlineProperties.java +++ b/src/com/google/javascript/jscomp/InlineProperties.java @@ -20,11 +20,11 @@ import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; import com.google.javascript.rhino.IR; -import com.google.javascript.rhino.JSDocInfo; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.jstype.FunctionType; import com.google.javascript.rhino.jstype.JSType; import com.google.javascript.rhino.jstype.JSTypeNative; +import com.google.javascript.rhino.jstype.ObjectType; import java.util.HashMap; import java.util.Map; @@ -103,14 +103,17 @@ class GatherCandidates extends AbstractPostOrderCallback { @Override public void visit(NodeTraversal t, Node n, Node parent) { - boolean invalidatingPropRef = false; - String propName = null; + // These are assigned at most once in the branches below + final boolean invalidatingPropRef; + final String propName; + if (n.isGetProp()) { propName = n.getLastChild().getString(); if (parent.isAssign()) { - invalidatingPropRef = !isValidCandidateDefinition(t, n, parent); + invalidatingPropRef = !maybeRecordCandidateDefinition(t, n, parent); } else if (NodeUtil.isLValue(n)) { // Other LValue references invalidate + // e.g. in an enhanced for loop or a destructuring statement invalidatingPropRef = true; } else if (parent.isDelProp()) { // Deletes invalidate @@ -119,13 +122,21 @@ public void visit(NodeTraversal t, Node n, Node parent) { // A property read doesn't invalidate invalidatingPropRef = false; } - } else if (n.isStringKey()) { + } else if ((n.isStringKey() && !n.getParent().isObjectPattern()) + || n.isGetterDef() + || n.isSetterDef() + || n.isMemberFunctionDef()) { propName = n.getString(); // For now, any object literal key invalidates // TODO(johnlenz): support prototype properties like: // foo.prototype = { a: 1, b: 2 }; // TODO(johnlenz): Object.create(), Object.createProperty + // and getter/setter defs and member functions also invalidate + // since we do not inline functions in this pass + // Note that string keys in destructuring patterns are fine, since they just access the prop invalidatingPropRef = true; + } else { + return; } if (invalidatingPropRef) { @@ -135,7 +146,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { } /** @return Whether this is a valid definition for a candidate property. */ - private boolean isValidCandidateDefinition(NodeTraversal t, Node n, Node parent) { + private boolean maybeRecordCandidateDefinition(NodeTraversal t, Node n, Node parent) { checkState(n.isGetProp() && parent.isAssign(), n); Node src = n.getFirstChild(); String propName = n.getLastChild().getString(); @@ -145,9 +156,10 @@ private boolean isValidCandidateDefinition(NodeTraversal t, Node n, Node parent) // This is a simple assignment like: // this.foo = 1; if (inConstructor(t)) { - // This maybe a valid assignment. + // This may be a valid assignment. return maybeStoreCandidateValue(getJSType(src), propName, value); } + return false; } else if (t.inGlobalHoistScope() && src.isGetProp() && src.getLastChild().getString().equals("prototype")) { @@ -198,13 +210,22 @@ private boolean maybeStoreCandidateValue(JSType type, String propName, Node valu return false; } + /** + * Returns whether the traversal is directly in an ES6 class constructor or an @constructor + * function + * + *

This returns false for nested functions inside ctors, including arrow functions (even + * though the `this` is the same). This pass only cares about property definitions executed once + * per ctor invocation, and in general we don't know how many times an arrow fn will be + * executed. In the future, we could special case arrow fn IIFEs in this pass if it becomes + * useful. + */ private boolean inConstructor(NodeTraversal t) { Node root = t.getEnclosingFunction(); - if (root == null) { + if (root == null) { // we might be in the global scope return false; } - JSDocInfo info = NodeUtil.getBestJSDocInfo(root); - return info != null && info.isConstructor(); + return NodeUtil.isConstructor(root); } } @@ -231,17 +252,29 @@ && isMatchingType(target, info.type)) { private boolean isMatchingType(Node n, JSType src) { src = src.restrictByNotNullOrUndefined(); JSType dest = getJSType(n).restrictByNotNullOrUndefined(); - if (!invalidatingTypes.isInvalidating(dest)) { - if (dest.isConstructor() || src.isConstructor()) { - // Don't inline constructor properties referenced from - // subclass constructor references. This would be appropriate - // for ES6 class with Class-side inheritence but not - // traditional Closure classes from which subclass constructor - // don't inherit the super-classes constructor properties. - return dest.equals(src); - } else { - return dest.isSubtypeOf(src); + if (invalidatingTypes.isInvalidating(dest)) { + return false; + } + if (dest.isConstructor() || src.isConstructor()) { + // instead of using .isSubtypeOf for functions, check the prototype chain, since the + // FunctionType subtyping semantics is not what we want. + // This case is for ES6 class-side inheritance + return hasInPrototypeChain(dest.toMaybeFunctionType(), src.toMaybeFunctionType()); + } + return dest.isSubtypeOf(src); + } + + @SuppressWarnings("ReferenceEquality") + private boolean hasInPrototypeChain(FunctionType subCtor, FunctionType superCtor) { + if (subCtor == null || superCtor == null) { + return false; + } + ObjectType proto = subCtor; + while (proto != null) { + if (proto == superCtor) { + return true; } + proto = proto.getImplicitPrototype(); } return false; } diff --git a/test/com/google/javascript/jscomp/InlinePropertiesTest.java b/test/com/google/javascript/jscomp/InlinePropertiesTest.java index 65fe6dc500d..b648367f1a9 100644 --- a/test/com/google/javascript/jscomp/InlinePropertiesTest.java +++ b/test/com/google/javascript/jscomp/InlinePropertiesTest.java @@ -531,4 +531,178 @@ public void testStructuralInterfacesNoPropInlining2() { "function f(/** ? */ x) { return x.foo; }", "f(new C());")); } + + @Test + public void testConstInstanceProp_es6Class() { + // Replace a reference to known constant property. + test( + lines( + "class C {", // + " constructor() {", + " this.foo = 1;", + " }", + "}", + "new C().foo;"), + lines( + "class C {", // + " constructor() {", + " this.foo = 1;", + " }", + "}", + "new C(), 1;")); + } + + @Test + public void testMultipleConstInstanceProp_es6Class() { + test( + lines( + "class Foo {", + " constructor() {", + " /** @type {?number} */", + " this.a = 1;", + " /** @type {number} */", + " this.b = 2;", + " }", + "}", + "var x = (new Foo).b;"), + lines( + "class Foo {", + " constructor() {", + " /** @type {?number} */", + " this.a = 1;", + " /** @type {number} */", + " this.b = 2;", + " }", + "}", + "var x = (new Foo, 2);")); + } + + @Test + public void testConstInstancePropInArrowFunction_es6Class() { + // Don't replace a reference to known constant property defined in an arrow function. + testSame( + lines( + "/** @unrestricted */", // make this not a struct, so we can define this.foo + "class C {", // + " constructor() {", + " (() => {", + " this.foo = 1;", + " })();", + " }", + "}", + "new C().foo;")); + } + + @Test + public void testConstClassProps_es6Class() { + // Inline constant class properties, + test( + lines( + "class C {}", // + "C.bar = 2;", + "C.foo = 1;", + "var z = C.foo;"), + lines( + "class C {}", // + "C.bar = 2;", + "C.foo = 1;", + "var z = 1;")); + } + + @Test + public void testConstClassPropsInheritedProp_es6Class() { + test( + lines( + "class C {}", // + "class D extends C {}", + "C.foo = 1;", + "var z = D.foo;\n"), + lines( + "class C {}", // + "class D extends C {}", + "C.foo = 1;", + "var z = 1;\n")); + } + + @Test + public void testConstClassPropsInheritedPropChain_es6Class() { + test( + lines( + "class C {}", // + "class D extends C {}", + "class E extends D {}", + "class F extends E {}", + "C.foo = 1;", + "var z = F.foo;"), + lines( + "class C {}", // + "class D extends C {}", + "class E extends D {}", + "class F extends E {}", + "C.foo = 1;", + "var z = 1;")); + } + + @Test + public void testConstClassPropsNonInheritedProp_es6Class() { + // Test that we don't accidentally treat the superclass as having a subclass prop + testSame( + lines( + "class C {}", // + "class D extends C {}", + "D.foo = 1;", + "var z = C.foo;")); + } + + @Test + public void testNonConstClassProp_es6ClassWithStaticMethod() { + testSame( + lines( + "class C { static foo() {} }", // + "alert(C.foo);", + "C.foo = 1;")); + } + + @Test + public void testConstPrototypeProp_es6Class() { + test( + lines( + "class C {}", // + "C.prototype.foo = 1;", + "new C().foo;"), + lines( + "class C {}", // + "C.prototype.foo = 1;", + "new C(), 1;")); + } + + @Test + public void testNonConstPrototypePropFromMemberFn() { + testSame( + lines( + "class C {", // + " foo() {}", + "}", + "C.prototype.foo = 4;", + "(new C()).foo")); + } + + @Test + public void testObjectPatternStringKeyDoesntInvalidateProp() { + test( + lines( + "/** @constructor */", + "function C() {", + " this.foo = 3;", + "}", + "(new C()).foo", + "const {foo} = new C();"), + lines( + "/** @constructor */", + "function C() {", + " this.foo = 3;", + "}", + "new C(), 3;", + "const {foo} = new C();")); + } }