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

feat(ecmascript): proxy call method #510

Merged
merged 4 commits into from
Jan 6, 2025
Merged

Conversation

yossydev
Copy link
Contributor

@yossydev yossydev commented Jan 5, 2025

I implemented the call method (also part of apply), but the number of crashes in test262 has increased. For example:

https://github.com/tc39/test262/blob/ff9763729d242b54a9f11bbc614670db3888c8d2/test/built-ins/Proxy/apply/call-result.js

When p.call(Function.prototype.call) is invoked, and p.apply is called, it results in the error:

Uncaught exception: TypeError: Not a callable value

I believe this is related to the following section of code:

/// ### [7.2.3 IsCallable ( argument )](https://tc39.es/ecma262/#sec-iscallable)
///
/// The abstract operation IsCallable takes argument argument (an ECMAScript
/// language value) and returns a Boolean. It determines if argument is a
/// callable function with a [[Call]] internal method.
///
/// > #### Note
/// > Nova breaks with the specification to narrow the types automatically, and
/// > returns an `Option<Function>`. Eventually this should become
/// > `Option<Callable>` once callable proxies are supported.
pub(crate) fn is_callable<'a, 'b>(
argument: impl TryInto<Function<'b>>,
_: NoGcScope<'a, '_>,
) -> Option<Function<'a>> {
// 1. If argument is not an Object, return false.
// 2. If argument has a [[Call]] internal method, return true.
// 3. Return false.
if let Ok(f) = argument.try_into() {
Some(f.unbind())
} else {
None
}
}

Here, the issue is that it returns None. As the comment suggests, I suspect that if we change Option<Function> to Option<Callable>, the try_into() method would return true and resolve the issue.

ref: #160
doc: https://tc39.es/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-call-thisargument-argumentslist

@aapoalas
Copy link
Collaborator

aapoalas commented Jan 5, 2025

Yup, we've not had callable proxies so Callable doesn't exist at all.

Do you want to try your hand at implementing it? It would basically just be 90% direct copy of the Function enum but with Proxy added to it. The is_callable function will then need to be changed to check if a Proxy's target is_callable.

@yossydev
Copy link
Contributor Author

yossydev commented Jan 5, 2025

Yup, we've not had callable proxies so Callable doesn't exist at all.

Do you want to try your hand at implementing it? It would basically just be 90% direct copy of the Function enum but with Proxy added to it. The is_callable function will then need to be changed to check if a Proxy's target is_callable.

I’d love to do it! I’ll probably start implementing it tomorrow or the day after!

Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

One nit and one issue (in two places though; remember to fix both call()s to be call_function()).

But generally looks good to me! Thank you again <3 We're really going places this week; we're now well past 50% passing rate!

nova_vm/src/ecmascript/builtins/proxy.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/builtins/proxy.rs Show resolved Hide resolved
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

LGTM, good job! <3

@aapoalas aapoalas merged commit e32ffd0 into trynova:main Jan 6, 2025
1 check passed
@yossydev
Copy link
Contributor Author

yossydev commented Jan 7, 2025

Yup, we've not had callable proxies so Callable doesn't exist at all.

Do you want to try your hand at implementing it? It would basically just be 90% direct copy of the Function enum but with Proxy added to it. The is_callable function will then need to be changed to check if a Proxy's target is_callable.

hi, @aapoalas

The Callable struct would look like the following, right?

#[derive(Clone, Copy, PartialEq)]
#[repr(u8)]
pub enum Callable<'a> {
    BoundFunction(BoundFunction<'a>) = BOUND_FUNCTION_DISCRIMINANT,
    BuiltinFunction(BuiltinFunction<'a>) = BUILTIN_FUNCTION_DISCRIMINANT,
    ECMAScriptFunction(ECMAScriptFunction<'a>) = ECMASCRIPT_FUNCTION_DISCRIMINANT,
    BuiltinGeneratorFunction = BUILTIN_GENERATOR_FUNCTION_DISCRIMINANT,
    BuiltinConstructorFunction(BuiltinConstructorFunction<'a>) =
        BUILTIN_CONSTRUCTOR_FUNCTION_DISCRIMINANT,
    BuiltinPromiseResolvingFunction(BuiltinPromiseResolvingFunction<'a>) =
        BUILTIN_PROMISE_RESOLVING_FUNCTION_DISCRIMINANT,
    BuiltinPromiseCollectorFunction = BUILTIN_PROMISE_COLLECTOR_FUNCTION_DISCRIMINANT,
    BuiltinProxyRevokerFunction = BUILTIN_PROXY_REVOKER_FUNCTION,
    Proxy(Proxy) = PROXY_DISCRIMINANT,
}

And in this case, would you also add things like unbind to the Callable enum? Would it essentially be the same implementation as Function? 👀

@aapoalas
Copy link
Collaborator

aapoalas commented Jan 7, 2025

Yup, exactly

@yossydev
Copy link
Contributor Author

yossydev commented Jan 7, 2025

Yup, exactly

ok, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants