From b826e092755ce17e2b3f6ad3f00d0f5220665f09 Mon Sep 17 00:00:00 2001 From: simranarora Date: Fri, 7 Jul 2017 14:48:56 -0700 Subject: [PATCH] Roll-forward the changes for block-scope-declared functions being transpiled to Es3 and Es5. Changes: prevented block-scope-declared feature from causing unnecessary transpilation ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=161249020 --- .../javascript/jscomp/Es6ExternsCheck.java | 10 ++- .../Es6RewriteBlockScopedDeclaration.java | 12 +-- .../jscomp/TranspilationPasses.java | 7 ++ .../javascript/jscomp/parsing/IRFactory.java | 8 +- .../jscomp/parsing/parser/FeatureSet.java | 18 +++- .../jscomp/parsing/parser/Parser.java | 2 +- ...eBlockScopedDeclarationEs6LangOutTest.java | 86 +++++++++++++++++++ .../javascript/jscomp/NodeTraversalTest.java | 2 +- .../javascript/jscomp/parsing/ParserTest.java | 16 +++- 9 files changed, 147 insertions(+), 14 deletions(-) create mode 100644 test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationEs6LangOutTest.java diff --git a/src/com/google/javascript/jscomp/Es6ExternsCheck.java b/src/com/google/javascript/jscomp/Es6ExternsCheck.java index 086b1e0ac76..25c274f1e9f 100644 --- a/src/com/google/javascript/jscomp/Es6ExternsCheck.java +++ b/src/com/google/javascript/jscomp/Es6ExternsCheck.java @@ -18,6 +18,8 @@ import static com.google.common.base.Preconditions.checkState; import com.google.javascript.jscomp.NodeTraversal.AbstractPreOrderCallback; +import com.google.javascript.jscomp.parsing.parser.FeatureSet; +import com.google.javascript.jscomp.parsing.parser.FeatureSet.Feature; import com.google.javascript.rhino.Node; /** @@ -40,7 +42,13 @@ private boolean hasEs6Syntax(Node root) { for (Node script : root.children()) { checkState(script.isScript()); if (TranspilationPasses.isScriptEs6OrHigher(script)) { - return true; + FeatureSet features = (FeatureSet) script.getProp(Node.FEATURE_SET); + if (!features.has(Feature.BLOCK_SCOPED_FUNCTION_DECLARATION)) { + return true; + } else { + features = features.without(Feature.BLOCK_SCOPED_FUNCTION_DECLARATION); + return !FeatureSet.ES5.contains(features); + } } } return false; diff --git a/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java b/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java index 252485203bb..71dfa36d9c7 100644 --- a/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java +++ b/src/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclaration.java @@ -108,8 +108,6 @@ && inLoop(n)) { @Override public void process(Node externs, Node root) { - // Since block-scoped function declarations can appear in any language mode, we need to run - // this pass unconditionally. NodeTraversal.traverseEs6(compiler, root, new CollectUndeclaredNames()); NodeTraversal.traverseEs6(compiler, root, this); // Needed for let / const declarations in .d.ts externs. @@ -119,9 +117,13 @@ public void process(Node externs, Node root) { NodeTraversal.traverseEs6(compiler, root, transformer); transformer.transformLoopClosure(); varify(); - TranspilationPasses.processTranspile( - compiler, externs, new RewriteBlockScopedFunctionDeclaration()); - NodeTraversal.traverseEs6(compiler, root, new RewriteBlockScopedFunctionDeclaration()); + + // Block scoped function declarations can occur in any language mode, however for + // transpilation to ES3 and ES5, we want to hoist the functions from the block-scope by + // redeclaring them in var assignments + RewriteBlockScopedFunctionDeclaration rewriteFunction = + new RewriteBlockScopedFunctionDeclaration(); + TranspilationPasses.processTranspile(compiler, root, rewriteFunction); } @Override diff --git a/src/com/google/javascript/jscomp/TranspilationPasses.java b/src/com/google/javascript/jscomp/TranspilationPasses.java index 7a5805686c4..22eee6877a8 100644 --- a/src/com/google/javascript/jscomp/TranspilationPasses.java +++ b/src/com/google/javascript/jscomp/TranspilationPasses.java @@ -21,6 +21,7 @@ import com.google.javascript.jscomp.NodeTraversal.Callback; import com.google.javascript.jscomp.PassFactory.HotSwapPassFactory; import com.google.javascript.jscomp.parsing.parser.FeatureSet; +import com.google.javascript.jscomp.parsing.parser.FeatureSet.Feature; import com.google.javascript.rhino.Node; import java.util.List; @@ -336,11 +337,17 @@ static void processTranspile( AbstractCompiler compiler, Node combinedRoot, Callback... callbacks) { if (compiler.getOptions().needsTranspilationFrom(FeatureSet.ES6)) { for (Node singleRoot : combinedRoot.children()) { + FeatureSet features = (FeatureSet) singleRoot.getProp(Node.FEATURE_SET); if (isScriptEs6OrHigher(singleRoot)) { for (Callback callback : callbacks) { singleRoot.putBooleanProp(Node.TRANSPILED, true); NodeTraversal.traverseEs6(compiler, singleRoot, callback); } + } else if (features != null && features.has(Feature.BLOCK_SCOPED_FUNCTION_DECLARATION)) { + // We want to traverse, but not give this node the transpiled property. + for (Callback callback : callbacks) { + NodeTraversal.traverseEs6(compiler, singleRoot, callback); + } } } } diff --git a/src/com/google/javascript/jscomp/parsing/IRFactory.java b/src/com/google/javascript/jscomp/parsing/IRFactory.java index f355b8e5f94..e0f7ec5e10d 100644 --- a/src/com/google/javascript/jscomp/parsing/IRFactory.java +++ b/src/com/google/javascript/jscomp/parsing/IRFactory.java @@ -279,7 +279,7 @@ class IRFactory { private boolean currentFileIsExterns = false; private boolean hasJsDocTypeAnnotations = false; - private FeatureSet features = FeatureSet.ES3; + private FeatureSet features = FeatureSet.BARE_MINIMUM; private Node resultNode; private IRFactory(String sourceString, @@ -1256,6 +1256,12 @@ Node processFunction(FunctionDeclarationTree functionTree) { maybeWarnForFeature(functionTree, Feature.ASYNC_FUNCTIONS); } + // Add a feature so that transpilation process hoists block scoped functions through + // var redeclaration in ES3 and ES5 + if (isDeclaration) { + features = features.with(Feature.BLOCK_SCOPED_FUNCTION_DECLARATION); + } + IdentifierToken name = functionTree.name; Node newName; if (name != null) { diff --git a/src/com/google/javascript/jscomp/parsing/parser/FeatureSet.java b/src/com/google/javascript/jscomp/parsing/parser/FeatureSet.java index a22cc83b106..2ae5f6b4dc8 100644 --- a/src/com/google/javascript/jscomp/parsing/parser/FeatureSet.java +++ b/src/com/google/javascript/jscomp/parsing/parser/FeatureSet.java @@ -43,8 +43,11 @@ public final class FeatureSet implements Serializable { private final ImmutableSet features; - /** The bare minimum set of features in ES3. */ - public static final FeatureSet ES3 = new FeatureSet(ImmutableSet.of()); + /** The bare minimum set of features. */ + public static final FeatureSet BARE_MINIMUM = new FeatureSet(ImmutableSet.of()); + + /**Features from ES3.*/ + public static final FeatureSet ES3 = BARE_MINIMUM.with(LangVersion.ES3.features()); /** Features from ES5 only. */ public static final FeatureSet ES5 = ES3.with(LangVersion.ES5.features()); @@ -66,7 +69,7 @@ public final class FeatureSet implements Serializable { public static final FeatureSet TYPESCRIPT = ES8_MODULES.with(LangVersion.TYPESCRIPT.features()); private enum LangVersion { - ES5, ES6, ES7, ES8, TYPESCRIPT; + ES3, ES5, ES6, ES7, ES8, TYPESCRIPT; private Set features() { Set set = new HashSet<>(); @@ -81,6 +84,15 @@ private Set features() { /** Specific features that can be included in a FeatureSet. */ public enum Feature { + //ES3 features + //Functions can be declared in the block scope for all ES versions. However, for ES3 and ES5, + //we want to hoist the functions from the block scope by redeclaring them as vars (i.e. + //{ function f() {} } becomes { var f = function {} }. To prevent this feature from causing + //code to run additional transpilation passes beyond rewriting block scoped functions, we mark + //block scoped functions as an ES3 feature and then manually determine whether to rewrite + //functions inside the processTranspile method of TranspilationPasses.java + BLOCK_SCOPED_FUNCTION_DECLARATION("block function", LangVersion.ES3), + // ES5 features ES3_KEYWORDS_AS_IDENTIFIERS("ES3 keywords as identifiers", LangVersion.ES5), GETTER("getters", LangVersion.ES5), diff --git a/src/com/google/javascript/jscomp/parsing/parser/Parser.java b/src/com/google/javascript/jscomp/parsing/parser/Parser.java index a6a0a8f0f53..27ceeb59d29 100644 --- a/src/com/google/javascript/jscomp/parsing/parser/Parser.java +++ b/src/com/google/javascript/jscomp/parsing/parser/Parser.java @@ -182,7 +182,7 @@ private enum FunctionFlavor { private final Config config; private final CommentRecorder commentRecorder = new CommentRecorder(); private final ArrayDeque functionContextStack = new ArrayDeque<>(); - private FeatureSet features = FeatureSet.ES3; + private FeatureSet features = FeatureSet.BARE_MINIMUM; private SourcePosition lastSourcePosition; @Nullable private String sourceMapURL; diff --git a/test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationEs6LangOutTest.java b/test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationEs6LangOutTest.java new file mode 100644 index 00000000000..4a84f68861a --- /dev/null +++ b/test/com/google/javascript/jscomp/Es6RewriteBlockScopedDeclarationEs6LangOutTest.java @@ -0,0 +1,86 @@ +/* + * Copyright 2017 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.google.javascript.jscomp; + +import com.google.javascript.jscomp.CompilerOptions.LanguageMode; + +/** + * Test case for {@link Es6RewriteBlockScopedDeclaration} with Es6 as the Language Out + * + * Created by simranarora on 7/6/17. + * @author simranarora@google.com (Simran Arora) + */ +public class Es6RewriteBlockScopedDeclarationEs6LangOutTest extends CompilerTestCase { + + @Override + protected void setUp() throws Exception { + super.setUp(); + setAcceptedLanguage(LanguageMode.ECMASCRIPT_2015); + enableRunTypeCheckAfterProcessing(); + } + + @Override + protected CompilerOptions getOptions() { + CompilerOptions options = super.getOptions(); + options.setLanguageOut(LanguageMode.ECMASCRIPT_2015); + return options; + } + + @Override + protected CompilerPass getProcessor(Compiler compiler) { + return new Es6RewriteBlockScopedDeclaration(compiler); + } + + @Override + protected int getNumRepetitions() { + return 1; + } + + public void testNoTranspilationForBlockScopedFunctionDeclarations() { + testSame( + LINE_JOINER.join( + "for (var x of y) {", + " function f() {", + " x;", + " }", + "}")); + } + + // TODO (simarora) Correct output is commented out for now - transpilation mode is being entered + // to convert lets and consts to vars even though output is ES6 + public void testNoTranspilationForLetConstDeclarations() { + /*testSame( + LINE_JOINER.join( + "for (var x of y) {", + " function f() {", + " let z;", + " }", + "}"));*/ + test( + LINE_JOINER.join( + "for (var x of y) {", + " function f() {", + " let z;", + " }", + "}"), + LINE_JOINER.join( + "for (var x of y) {", + " function f() {", + " var z;", + " }", + "}")); + } +} diff --git a/test/com/google/javascript/jscomp/NodeTraversalTest.java b/test/com/google/javascript/jscomp/NodeTraversalTest.java index 309c3aa5566..6ae50ae9bee 100644 --- a/test/com/google/javascript/jscomp/NodeTraversalTest.java +++ b/test/com/google/javascript/jscomp/NodeTraversalTest.java @@ -283,7 +283,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { + "visit FUNCTION foo [source_file: [testcode]] @2:0\n" + "visit SCRIPT [source_file: [testcode]] " + "[input_id: InputId: [testcode]] " - + "[feature_set: []] @1:0\n"; + + "[feature_set: [block function]] @1:0\n"; assertEquals(expectedResult, builder.toString()); } diff --git a/test/com/google/javascript/jscomp/parsing/ParserTest.java b/test/com/google/javascript/jscomp/parsing/ParserTest.java index ebf58355600..3df8f94b4aa 100644 --- a/test/com/google/javascript/jscomp/parsing/ParserTest.java +++ b/test/com/google/javascript/jscomp/parsing/ParserTest.java @@ -82,7 +82,7 @@ protected void setUp() throws Exception { mode = LanguageMode.ECMASCRIPT3; strictMode = SLOPPY; isIdeMode = false; - expectedFeatures = FeatureSet.ES3; + expectedFeatures = FeatureSet.BARE_MINIMUM; } public void testExponentOperator() { @@ -1795,6 +1795,18 @@ public void testLetForbidden2() { getRequiresEs6Message(Feature.LET_DECLARATIONS)); } + // Ensure that we only add the feature for function declarations (i.e. + // function f() {} ) and not function expressions (e.g. var f = function() {} ) + public void testBlockScopeFunctionDeclaration() { + expectFeatures(Feature.BLOCK_SCOPED_FUNCTION_DECLARATION); + parse("if (1) { function f() {} }"); + + expectFeatures(); + Node result = parse("if (1) { var f = function() {} }"); + FeatureSet features = (FeatureSet) result.getProp(Node.FEATURE_SET); + assertFS(features).doesNotHave(Feature.BLOCK_SCOPED_FUNCTION_DECLARATION); + } + public void testLetForbidden3() { mode = LanguageMode.ECMASCRIPT5; strictMode = STRICT; @@ -3800,7 +3812,7 @@ private Config createConfig() { /** Sets expectedFeatures based on the list of features. */ private void expectFeatures(Feature... features) { - expectedFeatures = FeatureSet.ES3.with(features); + expectedFeatures = FeatureSet.BARE_MINIMUM.with(features); } private static class ParserResult {