From 2c2637800a793c14dfb937243e45e9bc20b5d097 Mon Sep 17 00:00:00 2001 From: sdh Date: Mon, 13 Nov 2017 18:54:46 -0800 Subject: [PATCH] Don't copy static-inherited members that are assigned after the $jscomp.inherits call. Because the type checker is not yet aware of static inheritance, this ensures that we get concrete type errors whenever the naive $jscomp.inherits implementation used by IE10 and below will cause problems. Related to https://github.com/google/closure-compiler/issues/2708 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=175623237 --- .../jscomp/Es6ToEs3ClassSideInheritance.java | 35 +++++++++++++---- .../Es6ToEs3ClassSideInheritanceTest.java | 38 +++++++++++++++++++ 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/src/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritance.java b/src/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritance.java index d3b6db10363..db0d0baa0d0 100644 --- a/src/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritance.java +++ b/src/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritance.java @@ -27,10 +27,12 @@ import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; import java.util.Set; /** @@ -132,18 +134,18 @@ public void process(Node externs, Node root) { FindStaticMembers findStaticMembers = new FindStaticMembers(); TranspilationPasses.processTranspile(compiler, externs, findStaticMembers); TranspilationPasses.processTranspile(compiler, root, findStaticMembers); - processInherits(findStaticMembers.inheritsCalls); + processInherits(findStaticMembers); } @Override public void hotSwapScript(Node scriptRoot, Node originalRoot) { FindStaticMembers findStaticMembers = new FindStaticMembers(); TranspilationPasses.processTranspile(compiler, scriptRoot, findStaticMembers); - processInherits(findStaticMembers.inheritsCalls); + processInherits(findStaticMembers); } - private void processInherits(List inheritsCalls) { - for (Node inheritsCall : inheritsCalls) { + private void processInherits(FindStaticMembers findStaticMembers) { + for (Node inheritsCall : findStaticMembers.inheritsCalls) { Node superclassNameNode = inheritsCall.getLastChild(); String superclassQname = superclassNameNode.getQualifiedName(); Node subclassNameNode = superclassNameNode.getPrevious(); @@ -157,7 +159,7 @@ private void processInherits(List inheritsCalls) { if (superClass == null || subClass == null) { continue; } - copyStaticMembers(superClass, subClass, inheritsCall); + copyStaticMembers(superClass, subClass, inheritsCall, findStaticMembers); copyDeclarations(superClass, subClass, inheritsCall); } } @@ -205,7 +207,8 @@ private void copyDeclarations( } private void copyStaticMembers( - JavascriptClass superClass, JavascriptClass subClass, Node inheritsCall) { + JavascriptClass superClass, JavascriptClass subClass, Node inheritsCall, + FindStaticMembers findStaticMembers) { for (Node staticMember : superClass.staticMembers) { checkState(staticMember.isAssign(), staticMember); String memberName = staticMember.getFirstChild().getLastChild().getString(); @@ -215,6 +218,13 @@ private void copyStaticMembers( if (isOverriden(subClass, memberName)) { continue; } + if (findStaticMembers.isBefore(inheritsCall, staticMember)) { + // Don't copy members that are defined after the $jscomp.inherits call, + // since they will not work correctly in IE<11, where static inheritance + // is done by copying, rather than prototype manipulation. + continue; + } + JSDocInfoBuilder info = JSDocInfoBuilder.maybeCopyFrom(staticMember.getJSDocInfo()); Node function = staticMember.getLastChild(); Node sourceInfoNode = function; @@ -279,7 +289,10 @@ private boolean isReferenceToClass(NodeTraversal t, Node n) { } private class FindStaticMembers extends AbstractPostOrderCallback { - private final List inheritsCalls = new ArrayList<>(); + final List inheritsCalls = new ArrayList<>(); + // Store the order we find class definitions and static fields. Copied statics must occur + // after both the namespace and the copied property are defined. + final Map nodeOrder = new HashMap<>(); @Override public void visit(NodeTraversal t, Node n, Node parent) { @@ -287,6 +300,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { case CALL: if (n.getFirstChild().matchesQualifiedName(Es6RewriteClass.INHERITS)) { inheritsCalls.add(n); + nodeOrder.put(n, nodeOrder.size()); } if (NodeUtil.isObjectDefinePropertiesDefinition(n)) { visitDefinedPropertiesCall(t, n); @@ -359,6 +373,7 @@ private void visitAssign(NodeTraversal t, Node n) { Node classNode = getProp.getFirstChild(); if (isReferenceToClass(t, classNode)) { classByAlias.get(classNode.getQualifiedName()).staticMembers.add(n); + nodeOrder.put(n, nodeOrder.size()); } } } @@ -376,5 +391,11 @@ private void visitVar(Node n) { } } } + + boolean isBefore(Node earlier, Node later) { + Integer earlierPosition = nodeOrder.get(earlier); + Integer laterPosition = nodeOrder.get(later); + return earlierPosition != null && laterPosition != null && earlierPosition < laterPosition; + } } } diff --git a/test/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritanceTest.java b/test/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritanceTest.java index 1f6e7d051f1..445fd0b0bf0 100644 --- a/test/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritanceTest.java +++ b/test/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritanceTest.java @@ -44,6 +44,7 @@ protected int getNumRepetitions() { public void testSimple() { test( lines( + // Note: let statement is necessary so that input language is ES6; else pass is skipped. "let x = 1;", "/** @constructor */", "function Example() {}", @@ -95,6 +96,43 @@ public void testTyped() { "Subclass.staticMethod = Example.staticMethod;")); } + + public void testNestedSubclass() { + test( + lines( + "let x = 1;", + "/** @constructor */", + "function A() {}", + "A.a = 19;", + "/** @constructor */", + "A.B = function() {};", + "/** @constructor @extends {A} */", + "A.C = function() {};", + "$jscomp.inherits(A.C, A);", + "/** @constructor */", + "A.D = function() {};", + "A.e = 42;"), + lines( + "let x = 1;", + "/** @constructor */", + "function A() {}", + "A.a = 19;", + "/** @constructor */", + "A.B = function() {}", + "/** @constructor @extends {A} */", + "A.C = function() {};", + "$jscomp.inherits(A.C, A);", + "/** @constructor @extends {A} @suppress {visibility} */", + "A.C.C = A.C;", + "/** @constructor @suppress {visibility} */", + "A.C.B = A.B;", + "/** @suppress {visibility} */", + "A.C.a = A.a;", + "/** @constructor */", + "A.D = function() {};", + "A.e = 42;")); + } + public void testOverride() { testSame( lines(