Only apply auto-naming of function expressions based on syntax

The auto naming of function expressions is a purely syntactic
decision, so shouldn't be decided based on the dynamic type of
an assignment. This moves the decision making into the parser.

One icky hack is that we add a field to FunctionExpression to
indicate whether we can autoname. The real solution is to actually
generate a CompoundExpression node so that the parser can make
the correct decision, however this would have a potentially
significant run time cost.

This does not correct the behaviour for class expressions.

Patch from Anonymous.

Userland/Libraries/LibJS/AST.cpp

@@ -1429,10 +1429,6 @@ Value AssignmentExpression::execute(Interpreter& interpreter, GlobalObject& glob
         return {};
     }
 
-    // FIXME: We should also check if the LHS is an identifier reference.
-    if (rhs_result.is_function())
-        update_function_name(rhs_result, get_function_name(global_object, reference.name().to_value(interpreter.vm())));
-
     reference.put(global_object, rhs_result);
 
     if (interpreter.exception())
@@ -1573,7 +1569,8 @@ Value VariableDeclaration::execute(Interpreter& interpreter, GlobalObject& globa
             if (interpreter.exception())
                 return {};
             auto variable_name = declarator.id().string();
-            update_function_name(initalizer_result, variable_name);
+            if (is<ClassExpression>(*init))
+                update_function_name(initalizer_result, variable_name);
             interpreter.vm().set_variable(variable_name, initalizer_result, global_object, true);
         }
     }

Userland/Libraries/LibJS/AST.h

@@ -234,6 +234,13 @@ protected:
 
     const NonnullRefPtrVector<VariableDeclaration>& variables() const { return m_variables; }
 
+protected:
+    void set_name(FlyString name)
+    {
+        VERIFY(m_name.is_empty());
+        m_name = move(name);
+    }
+
 private:
     FlyString m_name;
     NonnullRefPtr<Statement> m_body;
@@ -266,7 +273,7 @@ public:
     static bool must_have_name() { return false; }
 
     FunctionExpression(SourceRange source_range, const FlyString& name, NonnullRefPtr<Statement> body, Vector<Parameter> parameters, i32 function_length, NonnullRefPtrVector<VariableDeclaration> variables, bool is_strict_mode, bool is_arrow_function = false)
-        : Expression(move(source_range))
+        : Expression(source_range)
         , FunctionNode(name, move(body), move(parameters), function_length, move(variables), is_strict_mode)
         , m_is_arrow_function(is_arrow_function)
     {
@@ -275,8 +282,20 @@ public:
     virtual Value execute(Interpreter&, GlobalObject&) const override;
     virtual void dump(int indent) const override;
 
+    void set_name_if_possible(FlyString new_name)
+    {
+        if (m_cannot_auto_rename)
+            return;
+        m_cannot_auto_rename = true;
+        if (name().is_empty())
+            set_name(move(new_name));
+    }
+    bool cannot_auto_rename() const { return m_cannot_auto_rename; }
+    void set_cannot_auto_rename() { m_cannot_auto_rename = true; }
+
 private:
-    bool m_is_arrow_function;
+    bool m_cannot_auto_rename { false };
+    bool m_is_arrow_function { false };
 };
 
 class ErrorExpression final : public Expression {

Userland/Libraries/LibJS/Parser.cpp

@@ -635,6 +635,9 @@ NonnullRefPtr<Expression> Parser::parse_primary_expression()
         }
         auto expression = parse_expression(0);
         consume(TokenType::ParenClose);
+        if (is<FunctionExpression>(*expression)) {
+            static_cast<FunctionExpression&>(*expression).set_cannot_auto_rename();
+        }
         return expression;
     }
     case TokenType::This:
@@ -1184,7 +1187,16 @@ NonnullRefPtr<AssignmentExpression> Parser::parse_assignment_expression(Assignme
     } else if (m_parser_state.m_strict_mode && is<CallExpression>(*lhs)) {
         syntax_error("Cannot assign to function call");
     }
-    return create_ast_node<AssignmentExpression>({ m_parser_state.m_current_token.filename(), rule_start.position(), position() }, assignment_op, move(lhs), parse_expression(min_precedence, associativity));
+    auto rhs = parse_expression(min_precedence, associativity);
+    if (assignment_op == AssignmentOp::Assignment && is<FunctionExpression>(*rhs)) {
+        auto ident = lhs;
+        if (is<MemberExpression>(*lhs)) {
+            ident = static_cast<MemberExpression&>(*lhs).property();
+        }
+        if (is<Identifier>(*ident))
+            static_cast<FunctionExpression&>(*rhs).set_name_if_possible(static_cast<Identifier&>(*ident).string());
+    }
+    return create_ast_node<AssignmentExpression>({ m_parser_state.m_current_token.filename(), rule_start.position(), position() }, assignment_op, move(lhs), move(rhs));
 }
 
 NonnullRefPtr<CallExpression> Parser::parse_call_expression(NonnullRefPtr<Expression> lhs)
@@ -1441,7 +1453,11 @@ NonnullRefPtr<VariableDeclaration> Parser::parse_variable_declaration(bool for_l
         } else if (!for_loop_variable_declaration && declaration_kind == DeclarationKind::Const) {
             syntax_error("Missing initializer in 'const' variable declaration");
         }
-        declarations.append(create_ast_node<VariableDeclarator>({ m_parser_state.m_current_token.filename(), rule_start.position(), position() }, create_ast_node<Identifier>({ m_parser_state.m_current_token.filename(), rule_start.position(), position() }, move(id)), move(init)));
+        auto identifier = create_ast_node<Identifier>({ m_parser_state.m_current_token.filename(), rule_start.position(), position() }, move(id));
+        if (init && is<FunctionExpression>(*init)) {
+            static_cast<FunctionExpression&>(*init).set_name_if_possible(id);
+        }
+        declarations.append(create_ast_node<VariableDeclarator>({ m_parser_state.m_current_token.filename(), rule_start.position(), position() }, move(identifier), move(init)));
         if (match(TokenType::Comma)) {
             consume();
             continue;

Userland/Libraries/LibJS/Tests/functions/function-name.js

@@ -48,3 +48,17 @@ test("names of native functions", () => {
     expect((console.debug.name = "warn")).toBe("warn");
     expect(console.debug.name).toBe("debug");
 });
+
+test("no invalid autonaming of anonymous functions", () => {
+    // prettier-ignore
+    let f1 = (function () {});
+    expect(f1.name).toBe("");
+    let f2 = f1;
+    expect(f2.name).toBe("");
+    let f3;
+    f3 = false || f2;
+    expect(f3.name).toBe("");
+    let f4 = false;
+    f4 ||= function () {};
+    expect(f4.name).toBe("");
+});