From c499239137337967dc803921bf236cadc74a99b9 Mon Sep 17 00:00:00 2001 From: Linus Groh Date: Tue, 16 Mar 2021 09:12:34 +0100 Subject: [PATCH] LibJS: Implement non-value-producing statements properly For various statements the spec states: Return NormalCompletion(empty). In those cases we have been returning undefined so far, which is incorrect. In other cases it states: Return Completion(UpdateEmpty(stmtCompletion, undefined)). Which essentially means a statement is evaluated and its completion value returned if non-empty, and undefined otherwise. While not actually noticeable in normal scripts as the VM's "last value" can't be accessed from JS code directly (with the exception of eval(), see below), it provided an inconsistent experience in the REPL: > if (true) 42; 42 > if (true) { 42; } undefined This also fixes the case where eval() would return undefined if the last executed statement is not a value-producing one: eval("1;;;;;") eval("1;{}") eval("1;var a;") As a consequence of the changes outlined above, these now all correctly return 1. See https://tc39.es/ecma262/#sec-block-runtime-semantics-evaluation, "NOTE 2". Fixes #3609. --- Userland/Libraries/LibJS/AST.cpp | 31 ++++++++++---------- Userland/Libraries/LibJS/AST.h | 8 ++--- Userland/Libraries/LibJS/Interpreter.cpp | 20 +++++++++---- Userland/Libraries/LibJS/Tests/eval-basic.js | 7 +++++ 4 files changed, 40 insertions(+), 26 deletions(-) diff --git a/Userland/Libraries/LibJS/AST.cpp b/Userland/Libraries/LibJS/AST.cpp index 9fb764284a..1c29175f16 100644 --- a/Userland/Libraries/LibJS/AST.cpp +++ b/Userland/Libraries/LibJS/AST.cpp @@ -114,7 +114,7 @@ Value FunctionDeclaration::execute(Interpreter& interpreter, GlobalObject&) cons interpreter.enter_node(*this); ScopeGuard exit_node { [&] { interpreter.exit_node(*this); } }; - return js_undefined(); + return {}; } Value FunctionExpression::execute(Interpreter& interpreter, GlobalObject& global_object) const @@ -315,14 +315,14 @@ Value WhileStatement::execute(Interpreter& interpreter, GlobalObject& global_obj interpreter.enter_node(*this); ScopeGuard exit_node { [&] { interpreter.exit_node(*this); } }; - Value last_value = js_undefined(); + auto last_value = js_undefined(); for (;;) { auto test_result = m_test->execute(interpreter, global_object); if (interpreter.exception()) return {}; if (!test_result.to_boolean()) break; - last_value = interpreter.execute_statement(global_object, *m_body); + last_value = interpreter.execute_statement(global_object, *m_body).value_or(last_value); if (interpreter.exception()) return {}; if (interpreter.vm().should_unwind()) { @@ -345,11 +345,11 @@ Value DoWhileStatement::execute(Interpreter& interpreter, GlobalObject& global_o interpreter.enter_node(*this); ScopeGuard exit_node { [&] { interpreter.exit_node(*this); } }; - Value last_value = js_undefined(); + auto last_value = js_undefined(); for (;;) { if (interpreter.exception()) return {}; - last_value = interpreter.execute_statement(global_object, *m_body); + last_value = interpreter.execute_statement(global_object, *m_body).value_or(last_value); if (interpreter.exception()) return {}; if (interpreter.vm().should_unwind()) { @@ -392,8 +392,7 @@ Value ForStatement::execute(Interpreter& interpreter, GlobalObject& global_objec interpreter.exit_scope(*wrapper); }); - Value last_value = js_undefined(); - + auto last_value = js_undefined(); if (m_init) { m_init->execute(interpreter, global_object); if (interpreter.exception()) @@ -407,7 +406,7 @@ Value ForStatement::execute(Interpreter& interpreter, GlobalObject& global_objec return {}; if (!test_result.to_boolean()) break; - last_value = interpreter.execute_statement(global_object, *m_body); + last_value = interpreter.execute_statement(global_object, *m_body).value_or(last_value); if (interpreter.exception()) return {}; if (interpreter.vm().should_unwind()) { @@ -428,7 +427,7 @@ Value ForStatement::execute(Interpreter& interpreter, GlobalObject& global_objec } } else { while (true) { - last_value = interpreter.execute_statement(global_object, *m_body); + last_value = interpreter.execute_statement(global_object, *m_body).value_or(last_value); if (interpreter.exception()) return {}; if (interpreter.vm().should_unwind()) { @@ -499,7 +498,7 @@ Value ForInStatement::execute(Interpreter& interpreter, GlobalObject& global_obj interpreter.vm().set_variable(variable_name, property_name.value_and_attributes(object).value, global_object, has_declaration); if (interpreter.exception()) return {}; - last_value = interpreter.execute_statement(global_object, *m_body); + last_value = interpreter.execute_statement(global_object, *m_body).value_or(last_value); if (interpreter.exception()) return {}; if (interpreter.vm().should_unwind()) { @@ -543,7 +542,7 @@ Value ForOfStatement::execute(Interpreter& interpreter, GlobalObject& global_obj get_iterator_values(global_object, rhs_result, [&](Value value) { interpreter.vm().set_variable(variable_name, value, global_object, has_declaration); - last_value = interpreter.execute_statement(global_object, *m_body); + last_value = interpreter.execute_statement(global_object, *m_body).value_or(last_value); if (interpreter.exception()) return IterationDecision::Break; if (interpreter.vm().should_unwind()) { @@ -897,7 +896,7 @@ Value ClassDeclaration::execute(Interpreter& interpreter, GlobalObject& global_o interpreter.current_scope()->put_to_scope(m_class_expression->name(), { class_constructor, DeclarationKind::Let }); - return js_undefined(); + return {}; } static void print_indent(int indent) @@ -1604,7 +1603,7 @@ Value VariableDeclaration::execute(Interpreter& interpreter, GlobalObject& globa interpreter.vm().set_variable(variable_name, initalizer_result, global_object, true); } } - return js_undefined(); + return {}; } Value VariableDeclarator::execute(Interpreter& interpreter, GlobalObject&) const @@ -2150,7 +2149,7 @@ Value BreakStatement::execute(Interpreter& interpreter, GlobalObject&) const ScopeGuard exit_node { [&] { interpreter.exit_node(*this); } }; interpreter.vm().unwind(ScopeType::Breakable, m_target_label); - return js_undefined(); + return {}; } Value ContinueStatement::execute(Interpreter& interpreter, GlobalObject&) const @@ -2159,7 +2158,7 @@ Value ContinueStatement::execute(Interpreter& interpreter, GlobalObject&) const ScopeGuard exit_node { [&] { interpreter.exit_node(*this); } }; interpreter.vm().unwind(ScopeType::Continuable, m_target_label); - return js_undefined(); + return {}; } void SwitchStatement::dump(int indent) const @@ -2247,7 +2246,7 @@ Value DebuggerStatement::execute(Interpreter& interpreter, GlobalObject&) const ScopeGuard exit_node { [&] { interpreter.exit_node(*this); } }; // Sorry, no JavaScript debugger available (yet)! - return js_undefined(); + return {}; } void ScopeNode::add_variables(NonnullRefPtrVector variables) diff --git a/Userland/Libraries/LibJS/AST.h b/Userland/Libraries/LibJS/AST.h index b22c99dc4c..1c7267ff16 100644 --- a/Userland/Libraries/LibJS/AST.h +++ b/Userland/Libraries/LibJS/AST.h @@ -91,7 +91,7 @@ public: : Statement(move(source_range)) { } - Value execute(Interpreter&, GlobalObject&) const override { return js_undefined(); } + Value execute(Interpreter&, GlobalObject&) const override { return {}; } }; class ErrorStatement final : public Statement { @@ -100,7 +100,7 @@ public: : Statement(move(source_range)) { } - Value execute(Interpreter&, GlobalObject&) const override { return js_undefined(); } + Value execute(Interpreter&, GlobalObject&) const override { return {}; } }; class ExpressionStatement final : public Statement { @@ -202,7 +202,7 @@ public: : Declaration(move(source_range)) { } - Value execute(Interpreter&, GlobalObject&) const override { return js_undefined(); } + Value execute(Interpreter&, GlobalObject&) const override { return {}; } }; class FunctionNode { @@ -286,7 +286,7 @@ public: { } - Value execute(Interpreter&, GlobalObject&) const override { return js_undefined(); } + Value execute(Interpreter&, GlobalObject&) const override { return {}; } }; class ReturnStatement final : public Statement { diff --git a/Userland/Libraries/LibJS/Interpreter.cpp b/Userland/Libraries/LibJS/Interpreter.cpp index 132bfa32f5..e773fbd34f 100644 --- a/Userland/Libraries/LibJS/Interpreter.cpp +++ b/Userland/Libraries/LibJS/Interpreter.cpp @@ -61,6 +61,8 @@ void Interpreter::run(GlobalObject& global_object, const Program& program) VM::InterpreterExecutionScope scope(*this); + vm.set_last_value({}, {}); + CallFrame global_call_frame; global_call_frame.current_node = &program; global_call_frame.this_value = &global_object; @@ -73,6 +75,9 @@ void Interpreter::run(GlobalObject& global_object, const Program& program) VERIFY(!vm.exception()); program.execute(*this, global_object); vm.pop_call_frame(); + + if (vm.last_value().is_empty()) + vm.set_last_value({}, js_undefined()); } GlobalObject& Interpreter::global_object() @@ -162,11 +167,10 @@ Value Interpreter::execute_statement(GlobalObject& global_object, const Statemen auto& block = static_cast(statement); enter_scope(block, scope_type, global_object); - if (block.children().is_empty()) - vm().set_last_value({}, js_undefined()); - for (auto& node : block.children()) { - vm().set_last_value({}, node.execute(*this, global_object)); + auto value = node.execute(*this, global_object); + if (!value.is_empty()) + vm().set_last_value({}, value); if (vm().should_unwind()) { if (!block.label().is_null() && vm().should_unwind_until(ScopeType::Breakable, block.label())) vm().stop_unwind(); @@ -174,14 +178,18 @@ Value Interpreter::execute_statement(GlobalObject& global_object, const Statemen } } - bool did_return = vm().unwind_until() == ScopeType::Function; + if (scope_type == ScopeType::Function) { + bool did_return = vm().unwind_until() == ScopeType::Function; + if (!did_return) + vm().set_last_value({}, js_undefined()); + } if (vm().unwind_until() == scope_type) vm().unwind(ScopeType::None); exit_scope(block); - return did_return ? vm().last_value() : js_undefined(); + return vm().last_value(); } LexicalEnvironment* Interpreter::current_environment() diff --git a/Userland/Libraries/LibJS/Tests/eval-basic.js b/Userland/Libraries/LibJS/Tests/eval-basic.js index e00c7fcbd9..e3f8a8cb5e 100644 --- a/Userland/Libraries/LibJS/Tests/eval-basic.js +++ b/Userland/Libraries/LibJS/Tests/eval-basic.js @@ -9,6 +9,13 @@ test("basic eval() functionality", () => { expect(foo(7)).toBe(12); }); +test("returns value of last value-producing statement", () => { + // See https://tc39.es/ecma262/#sec-block-runtime-semantics-evaluation + expect(eval("1;;;;;")).toBe(1); + expect(eval("1;{}")).toBe(1); + expect(eval("1;var a;")).toBe(1); +}); + test("syntax error", () => { expect(() => { eval("{");