From 30ab198b40cba813f93595360744312d6b3afb27 Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Sun, 17 Sep 2023 21:53:48 +1200 Subject: [PATCH] LibJS: Create const variables in ForIn/OfBodyEvaluation in strict mode Our implementation of environment.CreateImmutableBinding(name, true) in this AO was not correctly initializing const variables in strict mode. This would mean that constant declarations in for loop bodies would not throw if they were modified. To fix this, add a new parameter to CreateVariable to set strict mode. Also remove the vm.is_strict mode check here, as it doesn't look like anywhere in the spec will change strict mode depending on whether the script itself is running in script mode or not. This fixes two of our test-js tests, no change to test262. --- Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp | 2 +- Userland/Libraries/LibJS/Bytecode/Op.cpp | 8 ++++---- Userland/Libraries/LibJS/Bytecode/Op.h | 4 +++- Userland/Libraries/LibJS/Tests/loops/for-in-basic.js | 2 +- Userland/Libraries/LibJS/Tests/loops/for-of-basic.js | 2 +- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp index 7addde23ab..26fa3937e8 100644 --- a/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp +++ b/Userland/Libraries/LibJS/Bytecode/ASTCodegen.cpp @@ -2877,7 +2877,7 @@ static Bytecode::CodeGenerationErrorOr for_in_of_body_evaluation(Bytecode: // a. If IsConstantDeclaration of LetOrConst is true, then if (variable_declaration.is_constant_declaration()) { // i. Perform ! environment.CreateImmutableBinding(name, true). - generator.emit(interned_identifier, Bytecode::Op::EnvironmentMode::Lexical, true); + generator.emit(interned_identifier, Bytecode::Op::EnvironmentMode::Lexical, true, false, true); } // b. Else, else { diff --git a/Userland/Libraries/LibJS/Bytecode/Op.cpp b/Userland/Libraries/LibJS/Bytecode/Op.cpp index e660da5f1b..cdbf0f7113 100644 --- a/Userland/Libraries/LibJS/Bytecode/Op.cpp +++ b/Userland/Libraries/LibJS/Bytecode/Op.cpp @@ -568,15 +568,15 @@ ThrowCompletionOr CreateVariable::execute_impl(Bytecode::Interpreter& inte return vm.throw_completion(TRY_OR_THROW_OOM(vm, String::formatted("Lexical environment already has binding '{}'", name))); if (m_is_immutable) - return vm.lexical_environment()->create_immutable_binding(vm, name, vm.in_strict_mode()); + return vm.lexical_environment()->create_immutable_binding(vm, name, m_is_strict); else - return vm.lexical_environment()->create_mutable_binding(vm, name, vm.in_strict_mode()); + return vm.lexical_environment()->create_mutable_binding(vm, name, m_is_strict); } else { if (!m_is_global) { if (m_is_immutable) - return vm.variable_environment()->create_immutable_binding(vm, name, vm.in_strict_mode()); + return vm.variable_environment()->create_immutable_binding(vm, name, m_is_strict); else - return vm.variable_environment()->create_mutable_binding(vm, name, vm.in_strict_mode()); + return vm.variable_environment()->create_mutable_binding(vm, name, m_is_strict); } else { // NOTE: CreateVariable with m_is_global set to true is expected to only be used in GlobalDeclarationInstantiation currently, which only uses "false" for "can_be_deleted". // The only area that sets "can_be_deleted" to true is EvalDeclarationInstantiation, which is currently fully implemented in C++ and not in Bytecode. diff --git a/Userland/Libraries/LibJS/Bytecode/Op.h b/Userland/Libraries/LibJS/Bytecode/Op.h index 850812e93d..cdef1393b9 100644 --- a/Userland/Libraries/LibJS/Bytecode/Op.h +++ b/Userland/Libraries/LibJS/Bytecode/Op.h @@ -382,12 +382,13 @@ public: class CreateVariable final : public Instruction { public: - explicit CreateVariable(IdentifierTableIndex identifier, EnvironmentMode mode, bool is_immutable, bool is_global = false) + explicit CreateVariable(IdentifierTableIndex identifier, EnvironmentMode mode, bool is_immutable, bool is_global = false, bool is_strict = false) : Instruction(Type::CreateVariable, sizeof(*this)) , m_identifier(identifier) , m_mode(mode) , m_is_immutable(is_immutable) , m_is_global(is_global) + , m_is_strict(is_strict) { } @@ -399,6 +400,7 @@ private: EnvironmentMode m_mode; bool m_is_immutable : 4 { false }; bool m_is_global : 4 { false }; + bool m_is_strict { false }; }; class SetVariable final : public Instruction { diff --git a/Userland/Libraries/LibJS/Tests/loops/for-in-basic.js b/Userland/Libraries/LibJS/Tests/loops/for-in-basic.js index 5fd98f12cb..6abd0d7676 100644 --- a/Userland/Libraries/LibJS/Tests/loops/for-in-basic.js +++ b/Userland/Libraries/LibJS/Tests/loops/for-in-basic.js @@ -99,7 +99,7 @@ describe("special left hand sides", () => { }).toThrowWithMessage(ReferenceError, "Invalid left-hand side in assignment"); }); - test.xfail("Cannot change constant declaration in body", () => { + test("Cannot change constant declaration in body", () => { const vals = []; for (const v in [1, 2]) { expect(() => v++).toThrowWithMessage(TypeError, "Invalid assignment to const variable"); diff --git a/Userland/Libraries/LibJS/Tests/loops/for-of-basic.js b/Userland/Libraries/LibJS/Tests/loops/for-of-basic.js index 29707bf04d..4a7ae1fec6 100644 --- a/Userland/Libraries/LibJS/Tests/loops/for-of-basic.js +++ b/Userland/Libraries/LibJS/Tests/loops/for-of-basic.js @@ -143,7 +143,7 @@ describe("special left hand sides", () => { }).toThrowWithMessage(ReferenceError, "Invalid left-hand side in assignment"); }); - test.xfail("Cannot change constant declaration in body", () => { + test("Cannot change constant declaration in body", () => { const vals = []; for (const v of [1, 2]) { expect(() => v++).toThrowWithMessage(TypeError, "Invalid assignment to const variable");