diff --git a/src/com/google/javascript/jscomp/DefinitionsRemover.java b/src/com/google/javascript/jscomp/DefinitionsRemover.java index a75177be015..4ed5e27875e 100644 --- a/src/com/google/javascript/jscomp/DefinitionsRemover.java +++ b/src/com/google/javascript/jscomp/DefinitionsRemover.java @@ -48,7 +48,7 @@ static Definition getDefinition(Node n, boolean isExtern) { if (NodeUtil.isNameDeclaration(parent) && n.isName() && (isExtern || n.hasChildren())) { return new VarDefinition(n, isExtern); } else if (parent.isFunction() && parent.getFirstChild() == n) { - if (!NodeUtil.isFunctionExpression(parent)) { + if (NodeUtil.isFunctionDeclaration(parent)) { return new NamedFunctionDefinition(parent, isExtern); } else if (!n.getString().isEmpty()) { return new FunctionExpressionDefinition(parent, isExtern); diff --git a/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java b/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java index e58a4f2509a..810eeb2f8cc 100644 --- a/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java +++ b/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java @@ -1227,7 +1227,7 @@ private String createFunctionInternalName(Node fn, Node nameNode) { String internalName = null; // The name of a function expression isn't visible in the scope where the function expression // appears, so don't use it as the internalName. - if (nameNode == fn.getFirstChild() && NodeUtil.isFunctionExpression(fn)) { + if (nameNode == fn.getFirstChild() && !NodeUtil.isFunctionDeclaration(fn)) { nameNode = null; } if (nameNode == null || !nameNode.isName() diff --git a/src/com/google/javascript/jscomp/InlineFunctions.java b/src/com/google/javascript/jscomp/InlineFunctions.java index 9b56067a5af..81d12e4538d 100644 --- a/src/com/google/javascript/jscomp/InlineFunctions.java +++ b/src/com/google/javascript/jscomp/InlineFunctions.java @@ -215,7 +215,7 @@ public void findNamedFunctions(NodeTraversal t, Node n, Node parent) { // function Foo(x) { return ... } case FUNCTION: Preconditions.checkState(NodeUtil.isStatementBlock(parent) || parent.isLabel()); - if (!NodeUtil.isFunctionExpression(n)) { + if (NodeUtil.isFunctionDeclaration(n)) { Function fn = new NamedFunction(n); maybeAddFunction(fn, t.getModule()); } diff --git a/src/com/google/javascript/jscomp/NodeTraversal.java b/src/com/google/javascript/jscomp/NodeTraversal.java index 6bc69ec6031..8b6cd8fc894 100644 --- a/src/com/google/javascript/jscomp/NodeTraversal.java +++ b/src/com/google/javascript/jscomp/NodeTraversal.java @@ -772,9 +772,9 @@ private void traverseBranch(Node n, Node parent) { /** Traverses a function. */ private void traverseFunction(Node n, Node parent) { final Node fnName = n.getFirstChild(); - boolean isFunctionExpression = parent != null && NodeUtil.isFunctionExpression(n); + boolean isFunctionDeclaration = parent != null && NodeUtil.isFunctionDeclaration(n); - if (!isFunctionExpression) { + if (isFunctionDeclaration) { // Function declarations are in the scope containing the declaration. traverseBranch(fnName, n); } @@ -782,7 +782,7 @@ private void traverseFunction(Node n, Node parent) { curNode = n; pushScope(n); - if (isFunctionExpression) { + if (!isFunctionDeclaration) { // Function expression names are only accessible within the function // scope. traverseBranch(fnName, n); diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index 0f917ba9218..6797c5856f9 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -1153,7 +1153,7 @@ private static boolean checkForStateChangeHelper( // Function expressions don't have side-effects, but function // declarations change the namespace. Either way, we don't need to // check the children, since they aren't executed at declaration time. - return checkForNewObjects || !isFunctionExpression(n); + return checkForNewObjects || isFunctionDeclaration(n); case CLASS: return checkForNewObjects || isClassDeclaration(n) @@ -1554,7 +1554,9 @@ static boolean canBeSideEffected( case FUNCTION: // Function expression are not changed by side-effects, // and function declarations are not part of expressions. - Preconditions.checkState(isFunctionExpression(n)); + // TODO(bradfordcsmith): Do we need to add a case for CLASS here? + // This checkState currently does not exclude class methods. + Preconditions.checkState(!isFunctionDeclaration(n)); return false; default: break; @@ -3009,9 +3011,55 @@ static boolean isDeclaration(Node n) { * is not part of a expression; see {@link #isFunctionExpression}). */ public static boolean isFunctionDeclaration(Node n) { + // Note: There is currently one case where an unnamed function has a declaration parent. + // `export default function() {...}` + // In this case we consider the function to be an expression. return n.isFunction() && isDeclarationParent(n.getParent()) && isNamedFunction(n); } + /** + * Is this node a class or object literal member function? + * + *

examples: + * + * 


+   *   class C {
+   *     f() {}
+   *     get x() { return this.x_; }
+   *     set x(v) { this.x_ = v; }
+   *     [someExpr]() {}
+   *   }
+   *   obj = {
+   *     f() {}
+   *     get x() { return this.x_; }
+   *     set x(v) { this.x_ = v; }
+   *     [someExpr]() {}
+   *   }
+   *
+ */ + public static boolean isMethodDeclaration(Node n) { + if (n.isFunction()) { + Node parent = n.getParent(); + switch (parent.getToken()) { + case GETTER_DEF: + case SETTER_DEF: + case MEMBER_FUNCTION_DEF: + // `({ get x() {} })` + // `({ set x(v) {} })` + // `({ f() {} })` + return true; + case COMPUTED_PROP: + // `({ [expression]() {} })` + // The first child is the expression, and could possibly be a function. + return parent.getLastChild() == n; + default: + return false; + } + } else { + return false; + } + } + /** * see {@link #isClassDeclaration} */ @@ -3063,31 +3111,56 @@ static boolean isFunctionBlock(Node n) { } /** - * Is a FUNCTION node a function expression? A function expression is one - * that has either no name or a name that is not added to the current scope. + * Is a FUNCTION node a function expression? + * + *

A function expression is a function that: + *

* *

Some examples of function expressions: + * * 

    * (function () {})
    * (function f() {})()
    * [ function f() {} ]
    * var f = function f() {};
    * for (function f() {};;) {}
+   * export default function() {}
+   * () => 1
    *
* *

Some examples of functions that are not expressions: + * * 

    * function f() {}
    * if (x); else function f() {}
    * for (;;) { function f() {} }
    * export default function f() {}
+   * ({
+   *   f() {},
+   *   set x(v) {},
+   *   get x() {},
+   *   [expr]() {}
+   * })
+   * class {
+   *   f() {}
+   *   set x(v) {}
+   *   get x() {}
+   *   [expr]() {}
+   * }
    *
* * @param n A node * @return Whether n is a function used within an expression. */ static boolean isFunctionExpression(Node n) { - return n.isFunction() && (!isNamedFunction(n) || !isDeclarationParent(n.getParent())); + return n.isFunction() + && !NodeUtil.isFunctionDeclaration(n) + && !NodeUtil.isMethodDeclaration(n); } /** diff --git a/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java b/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java index 6cc8ab0d935..7e8998029a7 100644 --- a/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java +++ b/src/com/google/javascript/jscomp/ProcessClosurePrimitives.java @@ -371,7 +371,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { case FUNCTION: // If this is a declaration of a provided named function, this is an // error. Hoisted functions will explode if they're provided. - if (t.inGlobalHoistScope() && !NodeUtil.isFunctionExpression(n)) { + if (t.inGlobalHoistScope() && NodeUtil.isFunctionDeclaration(n)) { String name = n.getFirstChild().getString(); ProvidedName pn = providedNames.get(name); if (pn != null) { diff --git a/src/com/google/javascript/jscomp/VarCheck.java b/src/com/google/javascript/jscomp/VarCheck.java index 5dd12d7e7a0..907abe7a92f 100644 --- a/src/com/google/javascript/jscomp/VarCheck.java +++ b/src/com/google/javascript/jscomp/VarCheck.java @@ -171,8 +171,16 @@ public void visit(NodeTraversal t, Node n, Node parent) { // Only a function can have an empty name. if (varName.isEmpty()) { - checkState(parent.isFunction()); - checkState(NodeUtil.isFunctionExpression(parent)); + // Name is optional for function expressions + // x = function() {...} + // Arrow functions are also expressions and cannot have a name + // x = () => {...} + // Member functions have an empty NAME node string, because the actual name is stored on the + // MEMBER_FUNCTION_DEF object that contains the FUNCTION. + // class C { foo() {...} } + // x = { foo() {...} } + checkState( + NodeUtil.isFunctionExpression(parent) || NodeUtil.isMethodDeclaration(parent)); return; } diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java index 3da4fcc53aa..00e2d398821 100644 --- a/test/com/google/javascript/jscomp/NodeUtilTest.java +++ b/test/com/google/javascript/jscomp/NodeUtilTest.java @@ -431,6 +431,7 @@ public void testContainsFunctionDeclaration() { public void testIsFunctionDeclaration() { assertTrue(NodeUtil.isFunctionDeclaration(getFunctionNode("function foo(){}"))); assertFalse(NodeUtil.isFunctionDeclaration(getFunctionNode("class C { constructor() {} }"))); + assertFalse(NodeUtil.isFunctionDeclaration(getFunctionNode("({ foo() {} })"))); assertFalse(NodeUtil.isFunctionDeclaration(getFunctionNode("var x = function(){}"))); assertTrue(NodeUtil.isFunctionDeclaration(getFunctionNode("export function f() {}"))); assertFalse(NodeUtil.isFunctionDeclaration(getFunctionNode("export default function() {}"))); @@ -440,6 +441,16 @@ public void testIsFunctionDeclaration() { NodeUtil.isFunctionDeclaration(getFunctionNode("export default (foo) => { alert(foo); }"))); } + public void testIsMethodDeclaration() { + assertTrue( + NodeUtil.isMethodDeclaration(getFunctionNode("class C { constructor() {} }"))); + assertTrue(NodeUtil.isMethodDeclaration(getFunctionNode("class C { a() {} }"))); + assertTrue(NodeUtil.isMethodDeclaration(getFunctionNode("class C { static a() {} }"))); + assertTrue(NodeUtil.isMethodDeclaration(getFunctionNode("({ set foo(v) {} })"))); + assertTrue(NodeUtil.isMethodDeclaration(getFunctionNode("({ get foo() {} })"))); + assertTrue(NodeUtil.isMethodDeclaration(getFunctionNode("({ [foo]() {} })"))); + } + public void testIsClassDeclaration() { assertTrue(NodeUtil.isClassDeclaration(getClassNode("class Foo {}"))); assertFalse(NodeUtil.isClassDeclaration(getClassNode("var Foo = class {}"))); @@ -719,6 +730,9 @@ public void testIsFunctionExpression() { assertContainsAnonFunc(true, "export default function() {};"); assertContainsAnonFunc(false, "export default function a() {};"); assertContainsAnonFunc(false, "export function a() {};"); + assertContainsAnonFunc(false, "class C { a() {} }"); + assertContainsAnonFunc(false, "class C { static a() {} }"); + assertContainsAnonFunc(false, "x = { a() {} }"); } private void assertContainsAnonFunc(boolean expected, String js) {