Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove arguments_binding field from CodeBlock #2969

Merged
merged 2 commits into from
May 29, 2023
Merged

Remove arguments_binding field from CodeBlock #2969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0dde951
Ensure the "arguments" is the first argument
HalidOdat May 28, 2023
6eea363
Remove `arguments_binding` field from `CodeBlock`
HalidOdat May 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions boa_engine/src/builtins/function/arguments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,18 +155,22 @@ impl Arguments {
// We use indices to access environment bindings at runtime.
// To map to function parameters to binding indices, we use the fact, that bindings in a
// function environment start with all of the arguments in order:
//
// Note: The first binding (binding 0) is where "arguments" is stored.
//
// `function f (a,b,c)`
// | binding index | `arguments` property key | identifier |
// | 0 | 0 | a |
// | 1 | 1 | b |
// | 2 | 2 | c |
// | 1 | 0 | a |
// | 2 | 1 | b |
// | 3 | 2 | c |
//
// Notice that the binding index does not correspond to the argument index:
// `function f (a,a,b)` => binding indices 0 (a), 1 (b), 2 (c)
// | binding index | `arguments` property key | identifier |
// | - | 0 | - |
// | 0 | 1 | a |
// | 1 | 2 | b |
// | 1 | 1 | a |
// | 2 | 2 | b |
//
// While the `arguments` object contains all arguments, they must not be all bound.
// In the case of duplicate parameter names, the last one is bound as the environment binding.
//
Expand All @@ -178,10 +182,14 @@ impl Arguments {
if property_index >= len {
break;
}
let binding_index = bindings.len() as u32;

// NOTE(HalidOdat): Offset by +1 to account for the first binding ("argument").
let binding_index = bindings.len() as u32 + 1;

let entry = bindings
.entry(name)
.or_insert((binding_index, property_index));

entry.1 = property_index;
property_index += 1;
}
Expand Down
60 changes: 37 additions & 23 deletions boa_engine/src/bytecompiler/declarations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use crate::{
bytecompiler::{ByteCompiler, FunctionCompiler, FunctionSpec, Label, NodeKind},
vm::{create_function_object_fast, create_generator_function_object, BindingOpcode, Opcode},
vm::{
create_function_object_fast, create_generator_function_object, BindingOpcode,
CodeBlockFlags, Opcode,
},
JsNativeError, JsResult,
};
use boa_ast::{
Expand Down Expand Up @@ -844,7 +847,7 @@ impl ByteCompiler<'_, '_> {
function_names.reverse();
functions_to_initialize.reverse();

//15. Let argumentsObjectNeeded be true.
// 15. Let argumentsObjectNeeded be true.
let mut arguments_object_needed = true;

let arguments = Sym::ARGUMENTS.into();
Expand Down Expand Up @@ -882,26 +885,10 @@ impl ByteCompiler<'_, '_> {
additional_env = true;
}

// 21. For each String paramName of parameterNames, do
for param_name in &parameter_names {
// a. Let alreadyDeclared be ! env.HasBinding(paramName).
let already_declared = self.has_binding(*param_name);

// b. NOTE: Early errors ensure that duplicate parameter names can only occur in non-strict
// functions that do not have parameter default values or rest parameters.

// c. If alreadyDeclared is false, then
if !already_declared {
// i. Perform ! env.CreateMutableBinding(paramName, false).
self.create_mutable_binding(*param_name, false);

// Note: These steps are not necessary in our implementation.
// ii. If hasDuplicates is true, then
// 1. Perform ! env.InitializeBinding(paramName, undefined).
}
}

// 22. If argumentsObjectNeeded is true, then
//
// NOTE(HalidOdat): Has been moved up, so "arguments" gets registed as
// the first binding in the environment with index 0.
if arguments_object_needed {
// Note: This happens at runtime.
// a. If strict is true or simpleParameterList is false, then
Expand All @@ -918,21 +905,48 @@ impl ByteCompiler<'_, '_> {
// ii. NOTE: In strict mode code early errors prevent attempting to assign
// to this binding, so its mutability is not observable.
self.create_immutable_binding(arguments, false);
self.arguments_binding = Some(self.initialize_immutable_binding(arguments));
}
// d. Else,
else {
// i. Perform ! env.CreateMutableBinding("arguments", false).
self.create_mutable_binding(arguments, false);
self.arguments_binding = Some(self.initialize_mutable_binding(arguments, false));
}

self.code_block_flags |= CodeBlockFlags::NEEDS_ARGUMENTS_OBJECT;
}

// 21. For each String paramName of parameterNames, do
for param_name in &parameter_names {
// a. Let alreadyDeclared be ! env.HasBinding(paramName).
let already_declared = self.has_binding(*param_name);

// b. NOTE: Early errors ensure that duplicate parameter names can only occur in non-strict
// functions that do not have parameter default values or rest parameters.

// c. If alreadyDeclared is false, then
if !already_declared {
// i. Perform ! env.CreateMutableBinding(paramName, false).
self.create_mutable_binding(*param_name, false);

// Note: These steps are not necessary in our implementation.
// ii. If hasDuplicates is true, then
// 1. Perform ! env.InitializeBinding(paramName, undefined).
}
}

// 22. If argumentsObjectNeeded is true, then
if arguments_object_needed {
// MOVED: a-d.
//
// NOTE(HalidOdat): Has been moved up, see comment above.

// Note: This happens at runtime.
// e. Perform ! env.InitializeBinding("arguments", ao).

// f. Let parameterBindings be the list-concatenation of parameterNames and « "arguments" ».
parameter_names.push(arguments);
}

// 23. Else,
// a. Let parameterBindings be parameterNames.
let parameter_bindings = parameter_names.clone();
Expand Down
5 changes: 0 additions & 5 deletions boa_engine/src/bytecompiler/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,6 @@ pub struct ByteCompiler<'ctx, 'host> {
/// Functions inside this function
pub(crate) functions: Vec<Gc<CodeBlock>>,

/// The `arguments` binding location of the function, if set.
pub(crate) arguments_binding: Option<BindingLocator>,

/// Compile time environments in this function.
pub(crate) compile_environments: Vec<Gc<GcRefCell<CompileTimeEnvironment>>>,

Expand Down Expand Up @@ -299,7 +296,6 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> {
functions: Vec::default(),
this_mode: ThisMode::Global,
params: FormalParameterList::default(),
arguments_binding: None,
compile_environments: Vec::default(),
class_field_initializer_name: None,
code_block_flags,
Expand Down Expand Up @@ -1349,7 +1345,6 @@ impl<'ctx, 'host> ByteCompiler<'ctx, 'host> {
private_names: self.private_names.into_boxed_slice(),
bindings: self.bindings.into_boxed_slice(),
functions: self.functions.into_boxed_slice(),
arguments_binding: self.arguments_binding,
compile_environments: self.compile_environments.into_boxed_slice(),
class_field_initializer_name: self.class_field_initializer_name,
flags: Cell::new(self.code_block_flags),
Expand Down
66 changes: 49 additions & 17 deletions boa_engine/src/vm/code_block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ bitflags! {
/// Does this function have a parameters environment.
const PARAMETERS_ENV_BINDINGS = 0b0000_1000;

/// Does this function need a `"arguments"` object.
///
/// The `"arguments"` binding is the first binding.
const NEEDS_ARGUMENTS_OBJECT = 0b0001_0000;

/// Trace instruction execution to `stdout`.
#[cfg(feature = "trace")]
const TRACEABLE = 0b1000_0000;
Expand Down Expand Up @@ -124,10 +129,6 @@ pub struct CodeBlock {
/// Functions inside this function
pub(crate) functions: Box<[Gc<Self>]>,

/// The `arguments` binding location of the function, if set.
#[unsafe_ignore_trace]
pub(crate) arguments_binding: Option<BindingLocator>,

/// Compile time environments in this function.
pub(crate) compile_environments: Box<[Gc<GcRefCell<CompileTimeEnvironment>>]>,

Expand Down Expand Up @@ -155,7 +156,6 @@ impl CodeBlock {
length,
this_mode: ThisMode::Global,
params: FormalParameterList::default(),
arguments_binding: None,
compile_environments: Box::default(),
class_field_initializer_name: None,
}
Expand Down Expand Up @@ -206,6 +206,13 @@ impl CodeBlock {
.get()
.contains(CodeBlockFlags::PARAMETERS_ENV_BINDINGS)
}

/// Does this function need a `"arguments"` object.
pub(crate) fn needs_arguments_object(&self) -> bool {
self.flags
.get()
.contains(CodeBlockFlags::NEEDS_ARGUMENTS_OBJECT)
}
}

/// ---- `CodeBlock` private API ----
Expand Down Expand Up @@ -1078,7 +1085,19 @@ impl JsObject {
.push_lexical(code.compile_environments[last_env].clone());
}

if let Some(binding) = code.arguments_binding {
// Taken from: `FunctionDeclarationInstantiation` abstract function.
//
// Spec: https://tc39.es/ecma262/#sec-functiondeclarationinstantiation
//
// 22. If argumentsObjectNeeded is true, then
if code.needs_arguments_object() {
// a. If strict is true or simpleParameterList is false, then
// i. Let ao be CreateUnmappedArgumentsObject(argumentsList).
// b. Else,
// i. NOTE: A mapped argument object is only provided for non-strict functions
// that don't have a rest parameter, any parameter
// default value initializers, or any destructured parameters.
// ii. Let ao be CreateMappedArgumentsObject(func, formals, argumentsList, env).
let arguments_obj = if code.strict() || !code.params.is_simple() {
Arguments::create_unmapped_arguments_object(args, context)
} else {
Expand All @@ -1091,11 +1110,11 @@ impl JsObject {
context,
)
};
context.vm.environments.put_lexical_value(
binding.environment_index(),
binding.binding_index(),
arguments_obj.into(),
);
let env_index = context.vm.environments.len() as u32 - 1;
context
.vm
.environments
.put_lexical_value(env_index, 0, arguments_obj.into());
}

let argument_count = args.len();
Expand Down Expand Up @@ -1336,7 +1355,19 @@ impl JsObject {
.push_lexical(code.compile_environments[0].clone());
}

if let Some(binding) = code.arguments_binding {
// Taken from: `FunctionDeclarationInstantiation` abstract function.
//
// Spec: https://tc39.es/ecma262/#sec-functiondeclarationinstantiation
//
// 22. If argumentsObjectNeeded is true, then
if code.needs_arguments_object() {
// a. If strict is true or simpleParameterList is false, then
// i. Let ao be CreateUnmappedArgumentsObject(argumentsList).
// b. Else,
// i. NOTE: A mapped argument object is only provided for non-strict functions
// that don't have a rest parameter, any parameter
// default value initializers, or any destructured parameters.
// ii. Let ao be CreateMappedArgumentsObject(func, formals, argumentsList, env).
let arguments_obj = if code.strict() || !code.params.is_simple() {
Arguments::create_unmapped_arguments_object(args, context)
} else {
Expand All @@ -1349,11 +1380,12 @@ impl JsObject {
context,
)
};
context.vm.environments.put_lexical_value(
binding.environment_index(),
binding.binding_index(),
arguments_obj.into(),
);

let env_index = context.vm.environments.len() as u32 - 1;
context
.vm
.environments
.put_lexical_value(env_index, 0, arguments_obj.into());
}

let argument_count = args.len();
Expand Down