From 25dd07567e03f458e20aca78d2c137e15330f536 Mon Sep 17 00:00:00 2001 From: tbreisacher Date: Wed, 23 Mar 2016 17:24:17 -0700 Subject: [PATCH] Add a check for missing super() calls in constructors. Fixes https://github.com/google/closure-compiler/issues/1160 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=117986475 --- .../javascript/jscomp/CheckMissingSuper.java | 89 +++++++++++++++++++ .../javascript/jscomp/DefaultPassConfig.java | 10 +++ .../javascript/jscomp/LintPassConfig.java | 1 + .../google/javascript/jscomp/NodeUtil.java | 17 ++-- .../refactoring/ErrorToFixMapper.java | 10 +++ .../jscomp/CheckMissingSuperTest.java | 43 +++++++++ 6 files changed, 164 insertions(+), 6 deletions(-) create mode 100644 src/com/google/javascript/jscomp/CheckMissingSuper.java create mode 100644 test/com/google/javascript/jscomp/CheckMissingSuperTest.java diff --git a/src/com/google/javascript/jscomp/CheckMissingSuper.java b/src/com/google/javascript/jscomp/CheckMissingSuper.java new file mode 100644 index 00000000000..2e289855e5a --- /dev/null +++ b/src/com/google/javascript/jscomp/CheckMissingSuper.java @@ -0,0 +1,89 @@ +/* + * Copyright 2016 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.NodeTraversal.AbstractPostOrderCallback; +import com.google.javascript.jscomp.NodeTraversal.Callback; +import com.google.javascript.rhino.Node; + +/** + * Report an error if the user forgot to call super() in the constructor of an ES6 class. + * We don't require that the super() call be the first statement in the constructor, because doing + * so would break J2CL-compiled code. + */ +final class CheckMissingSuper extends AbstractPostOrderCallback implements HotSwapCompilerPass { + static final DiagnosticType MISSING_CALL_TO_SUPER = + DiagnosticType.error("JSC_MISSING_CALL_TO_SUPER", "constructor is missing a call to super()"); + + private final AbstractCompiler compiler; + + public CheckMissingSuper(AbstractCompiler compiler) { + this.compiler = compiler; + } + + @Override + public void process(Node externs, Node root) { + if (!compiler.getOptions().getLanguageIn().isEs6OrHigher()) { + return; + } + NodeTraversal.traverseEs6(compiler, root, this); + } + + @Override + public void hotSwapScript(Node scriptRoot, Node originalRoot) { + process(null, scriptRoot); + } + + @Override + public void visit(NodeTraversal t, Node n, Node parent) { + if (n.isClass()) { + Node superclass = n.getSecondChild(); + if (superclass.isEmpty()) { + return; + } + + Node constructor = + NodeUtil.getFirstPropMatchingKey(NodeUtil.getClassMembers(n), "constructor"); + if (constructor == null) { + return; + } + + FindSuper finder = new FindSuper(); + NodeTraversal.traverseEs6(compiler, NodeUtil.getFunctionBody(constructor), finder); + + if (!finder.found) { + t.report(constructor, MISSING_CALL_TO_SUPER); + } + } + } + + private static final class FindSuper implements Callback { + boolean found = false; + + @Override + public boolean shouldTraverse(NodeTraversal nodeTraversal, Node n, Node parent) { + return !found; + } + + @Override + public void visit(NodeTraversal t, Node n, Node parent) { + if (n.isSuper() && parent.isCall()) { + found = true; + return; + } + } + } +} diff --git a/src/com/google/javascript/jscomp/DefaultPassConfig.java b/src/com/google/javascript/jscomp/DefaultPassConfig.java index 2fa8beda784..e9e922f1ce9 100644 --- a/src/com/google/javascript/jscomp/DefaultPassConfig.java +++ b/src/com/google/javascript/jscomp/DefaultPassConfig.java @@ -221,6 +221,7 @@ protected List getChecks() { checks.add(convertEs6TypedToEs6); } + checks.add(checkMissingSuper); checks.add(checkVariableReferences); if (!options.skipNonTranspilationPasses && options.closurePass) { @@ -1432,6 +1433,15 @@ protected HotSwapCompilerPass create(AbstractCompiler compiler) { } }; + /** Checks that references to variables look reasonable. */ + private final HotSwapPassFactory checkMissingSuper = + new HotSwapPassFactory("checkMissingSuper", true) { + @Override + protected HotSwapCompilerPass create(AbstractCompiler compiler) { + return new CheckMissingSuper(compiler); + } + }; + /** Pre-process goog.testing.ObjectPropertyString. */ private final PassFactory objectPropertyStringPreprocess = new PassFactory("ObjectPropertyStringPreprocess", true) { diff --git a/src/com/google/javascript/jscomp/LintPassConfig.java b/src/com/google/javascript/jscomp/LintPassConfig.java index c3accda968b..e007d4cf596 100644 --- a/src/com/google/javascript/jscomp/LintPassConfig.java +++ b/src/com/google/javascript/jscomp/LintPassConfig.java @@ -69,6 +69,7 @@ protected CompilerPass create(AbstractCompiler compiler) { new CheckJSDocStyle(compiler), new CheckJSDoc(compiler), new CheckMissingSemicolon(compiler), + new CheckMissingSuper(compiler), new CheckRequiresAndProvidesSorted(compiler), new CheckUnusedLabels(compiler), new CheckUselessBlocks(compiler), diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index 3093c04d17e..e18da194832 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -515,6 +515,10 @@ public static String getNearestFunctionName(Node n) { return null; } + public static Node getClassMembers(Node n) { + Preconditions.checkArgument(n.isClass()); + return n.getLastChild(); + } /** * Returns true if this is an immutable value. @@ -1988,11 +1992,12 @@ public static Node getEnclosingNode(Node n, Predicate pred) { } /** - * @return The first property in the objlit that matches the key. + * @return The first property in the objlit or class members, that matches the key. */ - @Nullable static Node getFirstPropMatchingKey(Node objlit, String keyName) { - Preconditions.checkState(objlit.isObjectLit()); - for (Node keyNode : objlit.children()) { + @Nullable + static Node getFirstPropMatchingKey(Node n, String keyName) { + Preconditions.checkState(n.isObjectLit() || n.isClassMembers()); + for (Node keyNode : n.children()) { if ((keyNode.isStringKey() || keyNode.isMemberFunctionDef()) && keyNode.getString().equals(keyName)) { return keyNode.getFirstChild(); @@ -2505,8 +2510,8 @@ public static boolean isCallOrNew(Node node) { /** * Return a BLOCK node for the given FUNCTION node. */ - static Node getFunctionBody(Node fn) { - Preconditions.checkArgument(fn.isFunction()); + public static Node getFunctionBody(Node fn) { + Preconditions.checkArgument(fn.isFunction(), fn); return fn.getLastChild(); } diff --git a/src/com/google/javascript/refactoring/ErrorToFixMapper.java b/src/com/google/javascript/refactoring/ErrorToFixMapper.java index 27572dc3f74..f7cd66dcc89 100644 --- a/src/com/google/javascript/refactoring/ErrorToFixMapper.java +++ b/src/com/google/javascript/refactoring/ErrorToFixMapper.java @@ -75,6 +75,8 @@ public static SuggestedFix getFixForJsError(JSError error, AbstractCompiler comp return removeNode(error); case "JSC_INEXISTENT_PROPERTY": return getFixForInexistentProperty(error); + case "JSC_MISSING_CALL_TO_SUPER": + return getFixForMissingSuper(error); case "JSC_MISSING_REQUIRE_WARNING": return getFixForMissingRequire(error, compiler); case "JSC_DUPLICATE_REQUIRE_WARNING": @@ -114,6 +116,14 @@ private static SuggestedFix getFixForMissingSemicolon(JSError error) { .build(); } + private static SuggestedFix getFixForMissingSuper(JSError error) { + Node body = NodeUtil.getFunctionBody(error.node); + return new SuggestedFix.Builder() + .setOriginalMatchedNode(error.node) + .addChildToFront(body, "super();") + .build(); + } + private static SuggestedFix getFixForInexistentProperty(JSError error) { Matcher m = DID_YOU_MEAN.matcher(error.description); if (m.matches()) { diff --git a/test/com/google/javascript/jscomp/CheckMissingSuperTest.java b/test/com/google/javascript/jscomp/CheckMissingSuperTest.java new file mode 100644 index 00000000000..f49a6e1ecec --- /dev/null +++ b/test/com/google/javascript/jscomp/CheckMissingSuperTest.java @@ -0,0 +1,43 @@ +/* + * Copyright 2016 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 static com.google.javascript.jscomp.CheckMissingSuper.MISSING_CALL_TO_SUPER; + +public final class CheckMissingSuperTest extends Es6CompilerTestCase { + @Override + protected CompilerPass getProcessor(Compiler compiler) { + return new CheckMissingSuper(compiler); + } + + public void testMissingSuper() { + testErrorEs6("class C extends D { constructor() {} }", MISSING_CALL_TO_SUPER); + testErrorEs6("class C extends D { constructor() { super.foo(); } }", MISSING_CALL_TO_SUPER); + } + + public void testNoWarning() { + testSameEs6("class C extends D { constructor() { super(); } }"); + testSameEs6("class C { constructor() {} }"); + testSameEs6("class C extends D {}"); + } + + // We could require that the super() call is the first statement in the constructor, except that + // doing so breaks J2CL-compiled code, which needs to do the static initialization for the class + // before anything else. + public void testNoWarning_J2CL() { + testSameEs6("class C extends D { constructor() { C.init(); super(); } }"); + } +}