Skip to content

Commit

Permalink
Remove arguments_binding field from CodeBlock (#2969)
Browse files Browse the repository at this point in the history 
* Ensure the "arguments" is the first argument

* Remove `arguments_binding` field from `CodeBlock`
  • Loading branch information
@HalidOdat
HalidOdat authored May 29, 2023
1 parent 374d102 commit f1bab1e
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 51 deletions.
20 changes: 14 additions & 6 deletions boa_engine/src/builtins/function/arguments.rs
View file
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
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
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
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

0 comments on commit f1bab1e

Please sign in to comment.