Skip to content

Commit

Permalink
avoid the double-type-check of eagerly downcasting to function when c…
Browse files Browse the repository at this point in the history
…alling .bind(), since it's performed when calling .apply()
  • Loading branch information
dherman committed Sep 10, 2024
1 parent 45be1f3 commit bb4f231
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 15 deletions.
25 changes: 18 additions & 7 deletions crates/neon/src/object/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,20 @@
//! [hierarchy]: crate::types#the-javascript-type-hierarchy
//! [symbol]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol
use smallvec::smallvec;

use crate::{
context::{internal::ContextInternal, Context, Cx},
handle::{Handle, Root},
result::{NeonResult, Throw},
sys::{self, raw},
types::{
build, extract::{TryFromJs, TryIntoJs}, function::{BindOptions, CallOptions}, utf8::Utf8, JsFunction, JsObject, JsUndefined, JsValue, Value,
private::ValueInternal
build,
extract::{TryFromJs, TryIntoJs},
function::{BindOptions, CallOptions},
private::ValueInternal,
utf8::Utf8,
JsFunction, JsObject, JsUndefined, JsValue, Value,
},
};

Expand Down Expand Up @@ -197,10 +203,14 @@ where
/// May throw an exception either during accessing the property or downcasting it
/// to a function.
pub fn bind(&'a mut self) -> NeonResult<BindOptions<'a, 'cx>> {
let callee: Handle<JsFunction> = self.this.get(self.cx, self.key)?;
let mut bind = callee.bind(self.cx);
bind.this(self.this)?;
Ok(bind)
let callee: Handle<JsValue> = self.this.get(self.cx, self.key)?;
let this = Some(self.this.upcast());
Ok(BindOptions {
cx: self.cx,
callee,
this,
args: smallvec![],
})
}
}

Expand All @@ -212,7 +222,8 @@ pub trait Object: Value {
cx: &'a mut Cx<'cx>,
key: K,
) -> PropOptions<'a, 'cx, K> {
let this: Handle<'_, JsObject> = Handle::new_internal(unsafe { ValueInternal::from_local(cx.env(), self.to_local()) });
let this: Handle<'_, JsObject> =
Handle::new_internal(unsafe { ValueInternal::from_local(cx.env(), self.to_local()) });
PropOptions { cx, this, key }
}

Expand Down
7 changes: 5 additions & 2 deletions crates/neon/src/types_impl/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1179,8 +1179,11 @@ where
);

match status {
sys::Status::FunctionExpected => {
return cx.throw_type_error("not a function");
sys::Status::InvalidArg => {
if !sys::tag::is_function(env.to_raw(), callee) {
return cx.throw_error("not a function");
}
panic!("invalid argument");
}
sys::Status::PendingException => {
return Err(Throw::new());
Expand Down
13 changes: 8 additions & 5 deletions test/napi/lib/functions.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
var addon = require("..");
var assert = require("chai").assert;

const STRICT = function() { "use strict"; return this; };
const SLOPPY = Function('return this;');
const STRICT = function () {
"use strict";
return this;
};
const SLOPPY = Function("return this;");

function isStrict(f) {
try {
Expand Down Expand Up @@ -61,7 +64,7 @@ describe("JsFunction", function () {
assert.equal(result, 42);
});

it("bind a strict JsFunction to a number", function() {
it("bind a strict JsFunction to a number", function () {
assert.isTrue(isStrict(STRICT));

// strict mode functions are allowed to have a primitive this binding
Expand All @@ -70,15 +73,15 @@ describe("JsFunction", function () {
assert.strictEqual(result, 42);
});

it("bind a sloppy JsFunction to a primitive", function() {
it("bind a sloppy JsFunction to a primitive", function () {
assert.isFalse(isStrict(SLOPPY));

// legacy JS functions (aka "sloppy mode") replace primitive this bindings
// with object wrappers, so 42 will get wrapped as new Number(42)
const result = addon.bind_js_function_to_number(SLOPPY);

assert.instanceOf(result, Number);
assert.strictEqual(typeof result, 'object');
assert.strictEqual(typeof result, "object");
assert.strictEqual(result.valueOf(), 42);
});

Expand Down
2 changes: 1 addition & 1 deletion test/napi/lib/objects.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,6 @@ describe("JsObject", function () {

assert.throws(() => {
addon.call_non_method_with_prop(obj);
}, /failed to downcast/);
}, /not a function/);
});
});

0 comments on commit bb4f231

Please sign in to comment.