From d48a0aaa558a5fd3dab6401ebf1e4471450198e1 Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Wed, 1 Jan 2025 22:51:52 +1300 Subject: [PATCH] LibJS: Remove unneeded FIXMEs for suspending an execution context From what I understand, the suspension steps are not required now, or in the future for our implementation, or any other. The intent is already implemented in the spec pushing on another execution context to the stack and leaving the running execution context as-is. The resume steps are a slightly different story as there is some subtle behavior which the spec is trying to convey where some custom logic may need to be done when one execution context changes from one to another. It may be worth implementing those steps at a later point in time so that this behavior is a bit easier to follow in those cases. To make the situation more confusing - from what I can gather from the spec, not all cases that the spec mentions resume actually means anything normative. Resume is only _actually_ needed in a limited set of locations. For now, let's just remove the unneeded FIXMEs that indicate that there is something to be done for the suspension steps, as there is not, and leave the resume steps as is. --- Libraries/LibJS/Bytecode/Interpreter.cpp | 5 ++--- Libraries/LibJS/Runtime/AbstractOperations.cpp | 5 ++--- Libraries/LibJS/Runtime/AsyncFunctionDriverWrapper.cpp | 6 ++---- Libraries/LibJS/Runtime/AsyncGenerator.cpp | 9 +++------ Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp | 2 -- Libraries/LibJS/Runtime/GeneratorObject.cpp | 6 ++---- Libraries/LibJS/Runtime/NativeFunction.cpp | 4 ---- Libraries/LibJS/SourceTextModule.cpp | 6 +++--- Libraries/LibJS/SyntheticModule.cpp | 2 +- 9 files changed, 15 insertions(+), 30 deletions(-) diff --git a/Libraries/LibJS/Bytecode/Interpreter.cpp b/Libraries/LibJS/Bytecode/Interpreter.cpp index 65f721d69b6..2a82237cdf6 100644 --- a/Libraries/LibJS/Bytecode/Interpreter.cpp +++ b/Libraries/LibJS/Bytecode/Interpreter.cpp @@ -216,8 +216,7 @@ ThrowCompletionOr Interpreter::run(Script& script_record, GC::Ptris_strict_mode = script_record.parse_node().is_strict_mode(); - // FIXME: 9. Suspend the currently running execution context. - + // 9. Suspend the currently running execution context. // 10. Push scriptContext onto the execution context stack; scriptContext is now the running execution context. TRY(vm.push_execution_context(*script_context, {})); @@ -260,7 +259,7 @@ ThrowCompletionOr Interpreter::run(Script& script_record, GC::Ptr perform_eval(VM& vm, Value x, CallerMode strict_caller, } // 19. If runningContext is not already suspended, suspend runningContext. - // FIXME: We don't have this concept yet. + // NOTE: Done by the push on step 27. // 20. Let evalContext be a new ECMAScript code execution context. auto eval_context = ExecutionContext::create(); @@ -670,8 +670,7 @@ ThrowCompletionOr perform_eval(VM& vm, Value x, CallerMode strict_caller, // NOTE: We use a ScopeGuard to automatically pop the execution context when any of the `TRY`s below return a throw completion. ScopeGuard pop_guard = [&] { - // FIXME: 31. Suspend evalContext and remove it from the execution context stack. - + // 31. Suspend evalContext and remove it from the execution context stack. // 32. Resume the context that is now on the top of the execution context stack as the running execution context. vm.pop_execution_context(); }; diff --git a/Libraries/LibJS/Runtime/AsyncFunctionDriverWrapper.cpp b/Libraries/LibJS/Runtime/AsyncFunctionDriverWrapper.cpp index 8e59f1fcc6c..4fdde10f4f5 100644 --- a/Libraries/LibJS/Runtime/AsyncFunctionDriverWrapper.cpp +++ b/Libraries/LibJS/Runtime/AsyncFunctionDriverWrapper.cpp @@ -57,8 +57,7 @@ ThrowCompletionOr AsyncFunctionDriverWrapper::await(JS::Value value) // a. Let prevContext be the running execution context. auto& prev_context = vm.running_execution_context(); - // FIXME: b. Suspend prevContext. - + // b. Suspend prevContext. // c. Push asyncContext onto the execution context stack; asyncContext is now the running execution context. TRY(vm.push_execution_context(*m_suspended_execution_context, {})); @@ -85,8 +84,7 @@ ThrowCompletionOr AsyncFunctionDriverWrapper::await(JS::Value value) // a. Let prevContext be the running execution context. auto& prev_context = vm.running_execution_context(); - // FIXME: b. Suspend prevContext. - + // b. Suspend prevContext. // c. Push asyncContext onto the execution context stack; asyncContext is now the running execution context. TRY(vm.push_execution_context(*m_suspended_execution_context, {})); diff --git a/Libraries/LibJS/Runtime/AsyncGenerator.cpp b/Libraries/LibJS/Runtime/AsyncGenerator.cpp index 6356e2cf17f..f783eb643b3 100644 --- a/Libraries/LibJS/Runtime/AsyncGenerator.cpp +++ b/Libraries/LibJS/Runtime/AsyncGenerator.cpp @@ -87,8 +87,7 @@ ThrowCompletionOr AsyncGenerator::await(Value value) // a. Let prevContext be the running execution context. auto& prev_context = vm.running_execution_context(); - // FIXME: b. Suspend prevContext. - + // b. Suspend prevContext. // c. Push asyncContext onto the execution context stack; asyncContext is now the running execution context. TRY(vm.push_execution_context(async_context, {})); @@ -115,8 +114,7 @@ ThrowCompletionOr AsyncGenerator::await(Value value) // a. Let prevContext be the running execution context. auto& prev_context = vm.running_execution_context(); - // FIXME: b. Suspend prevContext. - + // b. Suspend prevContext. // c. Push asyncContext onto the execution context stack; asyncContext is now the running execution context. TRY(vm.push_execution_context(async_context, {})); @@ -309,8 +307,7 @@ ThrowCompletionOr AsyncGenerator::resume(VM& vm, Completion completion) // 3. Let callerContext be the running execution context. auto const& caller_context = vm.running_execution_context(); - // FIXME: 4. Suspend callerContext. - + // 4. Suspend callerContext. // 5. Set generator.[[AsyncGeneratorState]] to executing. m_async_generator_state = State::Executing; diff --git a/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp b/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp index 5eec342016d..d816f138b87 100644 --- a/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp +++ b/Libraries/LibJS/Runtime/ECMAScriptFunctionObject.cpp @@ -623,8 +623,6 @@ ThrowCompletionOr ECMAScriptFunctionObject::prepare_for_ordinary_call(Exec callee_context.private_environment = m_private_environment; // 11. If callerContext is not already suspended, suspend callerContext. - // FIXME: We don't have this concept yet. - // 12. Push calleeContext onto the execution context stack; calleeContext is now the running execution context. TRY(vm.push_execution_context(callee_context, {})); diff --git a/Libraries/LibJS/Runtime/GeneratorObject.cpp b/Libraries/LibJS/Runtime/GeneratorObject.cpp index 76faa12d2db..f46a4d84a8e 100644 --- a/Libraries/LibJS/Runtime/GeneratorObject.cpp +++ b/Libraries/LibJS/Runtime/GeneratorObject.cpp @@ -146,8 +146,7 @@ ThrowCompletionOr GeneratorObject::resume(VM& vm, Value value, Optional GeneratorObject::resume_abrupt(JS::VM& vm, JS::Completi // 6. Let methodContext be the running execution context. auto const& method_context = vm.running_execution_context(); - // FIXME: 7. Suspend methodContext. - + // 7. Suspend methodContext. // 9. Push genContext onto the execution context stack; genContext is now the running execution context. // NOTE: This is done out of order as to not permanently disable the generator if push_execution_context throws, // as `resume_abrupt` will immediately throw when [[GeneratorState]] is "executing", never allowing the state to change. diff --git a/Libraries/LibJS/Runtime/NativeFunction.cpp b/Libraries/LibJS/Runtime/NativeFunction.cpp index f82a735a6f2..7f8ef8ba6b0 100644 --- a/Libraries/LibJS/Runtime/NativeFunction.cpp +++ b/Libraries/LibJS/Runtime/NativeFunction.cpp @@ -118,8 +118,6 @@ ThrowCompletionOr NativeFunction::internal_call(Value this_argument, Read auto& caller_context = vm.running_execution_context(); // 2. If callerContext is not already suspended, suspend callerContext. - // NOTE: We don't support this concept yet. - // 3. Let calleeContext be a new execution context. auto callee_context = ExecutionContext::create(); @@ -181,8 +179,6 @@ ThrowCompletionOr> NativeFunction::internal_construct(ReadonlySp auto& caller_context = vm.running_execution_context(); // 2. If callerContext is not already suspended, suspend callerContext. - // NOTE: We don't support this concept yet. - // 3. Let calleeContext be a new execution context. auto callee_context = ExecutionContext::create(); diff --git a/Libraries/LibJS/SourceTextModule.cpp b/Libraries/LibJS/SourceTextModule.cpp index f49650edc17..ea85d92f2b8 100644 --- a/Libraries/LibJS/SourceTextModule.cpp +++ b/Libraries/LibJS/SourceTextModule.cpp @@ -702,7 +702,7 @@ ThrowCompletionOr SourceTextModule::execute_module(VM& vm, GC::Ptrlexical_environment = environment(); // 8. Suspend the currently running execution context. - // FIXME: We don't have suspend yet + // NOTE: Done by the push of execution context in steps below. // 9. If module.[[HasTLA]] is false, then if (!m_has_top_level_await) { @@ -760,8 +760,8 @@ ThrowCompletionOr SourceTextModule::execute_module(VM& vm, GC::Ptr SyntheticModule::evaluate(VM& vm) { // Note: Has some changes from PR: https://github.com/tc39/proposal-json-modules/pull/13. // 1. Suspend the currently running execution context. - // FIXME: We don't have suspend yet. + // NOTE: Done by the push on step 8. // 2. Let moduleContext be a new ECMAScript code execution context. auto module_context = ExecutionContext::create();