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.

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

[Merged by Bors] - Implement the global eval() function #2041

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
125 changes: 125 additions & 0 deletions boa_engine/src/builtins/eval/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
//! This module implements the global `eval` function.
//!
//! The `eval()` function evaluates JavaScript code represented as a string.
//!
//! More information:
//! - [ECMAScript reference][spec]
//! - [MDN documentation][mdn]
//!
//! [spec]: https://tc39.es/ecma262/#sec-eval-x
//! [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval

use crate::{
builtins::{function::Function, BuiltIn, JsArgs},
object::{JsObject, ObjectData},
property::Attribute,
Context, JsValue,
};
use boa_profiler::Profiler;
use rustc_hash::FxHashSet;

#[derive(Debug, Clone, Copy)]
pub(crate) struct Eval;

impl BuiltIn for Eval {
const NAME: &'static str = "eval";

const ATTRIBUTE: Attribute = Attribute::READONLY
.union(Attribute::NON_ENUMERABLE)
.union(Attribute::PERMANENT);

fn init(context: &mut Context) -> Option<JsValue> {
let _timer = Profiler::global().start_event(Self::NAME, "init");

let object = JsObject::from_proto_and_data(
context.intrinsics().constructors().function().prototype(),
ObjectData::function(Function::Native {
function: Self::eval,
constructor: false,
}),
);

Some(object.into())
}
}

impl Eval {
/// `19.2.1 eval ( x )`
///
/// More information:
/// - [ECMAScript reference][spec]
///
/// [spec]: https://tc39.es/ecma262/#sec-eval-x
fn eval(_: &JsValue, args: &[JsValue], context: &mut Context) -> Result<JsValue, JsValue> {
// 1. Return ? PerformEval(x, false, false).
Self::perform_eval(args.get_or_undefined(0), false, false, context)
}

/// `19.2.1.1 PerformEval ( x, callerRealm, strictCaller, direct )`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, the spec says this is:

19.2.1.1 PerformEval ( x, strictCaller, direct )

No mention of the callerRealm there, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, maybe it changed since I copied it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it changed a few days ago: tc39/ecma262#2670

///
/// More information:
/// - [ECMAScript reference][spec]
///
/// [spec]: https://tc39.es/ecma262/#sec-performeval
pub(crate) fn perform_eval(
x: &JsValue,
direct: bool,
strict: bool,
context: &mut Context,
) -> Result<JsValue, JsValue> {
let x = if let Some(x) = x.as_string() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add the comments relating to the spec directly where possible? It makes it much easier to follow the spec and maintain it in the future.

In any case, I see that the spec has been re-interpreted, not following the strict order as mentioned there, probably due to implementation reasons, but some comments here to explain the logic would be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I will try to add some comments and maybe match them to some of the spec comments. You're right, the implementation details diverge very hard from the spec here.

x.clone()
} else {
return Ok(x.clone());
};

let parsing_result = context.parse(x.as_bytes()).map_err(|e| e.to_string());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would avoid one layer of indirection, but not sure if it can be done like this.

Suggested change
let parsing_result = context.parse(x.as_bytes()).map_err(|e| e.to_string());
let parsing_result = context.parse(x.as_bytes()).map_err(ToString::to_string);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is possible. map_err expects the closure to take E by value but ToString::to_string takes the input by reference. I would expect the compiler to optimize the additional closure away.


let statement_list = match parsing_result {
Ok(statement_list) => statement_list,
Err(e) => return context.throw_syntax_error(e),
};

if direct {
context.realm.environments.poison_current();
context.realm.compile_env = context.realm.environments.current_compile_environment();
let environments_len = context.realm.environments.len();

let mut vars = FxHashSet::default();
statement_list.var_declared_names_new(&mut vars);
if let Some(name) = context
.realm
.environments
.has_lex_binding_until_function_environment(&vars)
{
return context.throw_syntax_error(format!("variable declaration {} in eval function already exists as lexically declaration", context.interner().resolve_expect(name)));
raskad marked this conversation as resolved.
Show resolved Hide resolved
}

let code_block = context.compile_with_new_declarative(&statement_list, strict)?;
context
.realm
.environments
.extend_outer_function_environment();
let result = context.execute(code_block);

context.realm.environments.truncate(environments_len);

result
} else {
context.realm.environments.poison_all();

let environments = context.realm.environments.pop_to_global();
let environments_len = context.realm.environments.len();

context.realm.compile_env = context.realm.environments.current_compile_environment();

let code_block = context.compile_with_new_declarative(&statement_list, false)?;
let result = context.execute(code_block);

context.realm.environments.truncate(environments_len);
context.realm.environments.extend(environments);

result
}
}
}
3 changes: 3 additions & 0 deletions boa_engine/src/builtins/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ pub mod console;
pub mod dataview;
pub mod date;
pub mod error;
pub mod eval;
pub mod function;
pub mod generator;
pub mod generator_function;
Expand Down Expand Up @@ -41,6 +42,7 @@ pub(crate) use self::{
AggregateError, Error, EvalError, RangeError, ReferenceError, SyntaxError, TypeError,
UriError,
},
eval::Eval,
function::BuiltInFunctionObject,
global_this::GlobalThis,
infinity::Infinity,
Expand Down Expand Up @@ -152,6 +154,7 @@ pub fn init(context: &mut Context) {
DataView,
Map,
Number,
Eval,
Set,
String,
RegExp,
Expand Down
Loading