Fix NodeUtil methods to handle method declarations.

NodeUtil.isFunctionExpression(n) should not return true for method declarations.
Add NodeUtil.isMethodDeclaration(n).

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=175601944

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);

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()

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());
           }

src/com/google/javascript/jscomp/NodeTraversal.java

@@ -772,17 +772,17 @@ 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);
     }
 
     curNode = n;
     pushScope(n);
 
-    if (isFunctionExpression) {
+    if (!isFunctionDeclaration) {
       // Function expression names are only accessible within the function
       // scope.
       traverseBranch(fnName, n);

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?
+   *
+   * <p>examples:
+   *
+   * <pre><code>
+   *   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]() {}
+   *   }
+   * </code></pre>
+   */
+  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?
+   *
+   * <p>A function expression is a function that:
+   * <ul>
+   *   <li>has either no name or a name that is not added to the current scope
+   *   <li>AND can be manipulated as an expression
+   *       (assigned to variables, passed to functions, etc.)
+   *       i.e. It is not a method declaration on a class or object literal.
+   * </ul>
    *
    * <p>Some examples of function expressions:
+   *
    * <pre>
    * (function () {})
    * (function f() {})()
    * [ function f() {} ]
    * var f = function f() {};
    * for (function f() {};;) {}
+   * export default function() {}
+   * () => 1
    * </pre>
    *
    * <p>Some examples of functions that are <em>not</em> expressions:
+   *
    * <pre>
    * 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]() {}
+   * }
    * </pre>
    *
    * @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);
   }
 
   /**

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) {

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;
       }
 

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) {