From a001a46980d35898a18c2850ef26db07fce40d6c Mon Sep 17 00:00:00 2001 From: Igor Matuszewski Date: Wed, 3 Apr 2024 14:40:36 +0200 Subject: [PATCH] refactor: Use safe generated version `T::into_instance` by napi-rs (#907) Rather than hand-rolling it; The codegen uses exactly the same code but with the benefit of abstracting away the unsafe details, getting rid of the only instance of `unsafe` we use. For reference: - `ToNapiValue`/`into_instance`: https://github.com/napi-rs/napi-rs/blob/0550c56fcf8eeec61d2e5abfa00c92a990c34e25/crates/backend/src/codegen/struct.rs#L278-L330 - `JsObject::from_raw_unchecked`/`ClassInstance::as_object`: https://github.com/napi-rs/napi-rs/blob/0550c56fcf8eeec61d2e5abfa00c92a990c34e25/crates/napi/src/bindgen_runtime/js_values/class.rs#L25 Additionally, I changed `Env` to be passed by value since it's a `Copy` newtype wrapper around pointer (no double indirection) and makes the code a bit clearer: no need to additionally dereference with `*`in the related calls, since napi-rs API accepts it by value. --- .../parser/runtime/src/napi_interface/cst.rs | 31 ++++++++++--------- .../runtime/src/napi_interface/cursor.rs | 4 +-- .../src/napi_interface/parse_output.rs | 2 +- .../templates/ast_selectors.rs.jinja2 | 2 +- .../generated/napi_interface/ast_selectors.rs | 2 +- .../src/generated/napi_interface/cst.rs | 31 ++++++++++--------- .../src/generated/napi_interface/cursor.rs | 4 +-- .../generated/napi_interface/parse_output.rs | 2 +- .../generated/napi_interface/ast_selectors.rs | 2 +- .../src/generated/napi_interface/cst.rs | 31 ++++++++++--------- .../src/generated/napi_interface/cursor.rs | 4 +-- .../generated/napi_interface/parse_output.rs | 2 +- 12 files changed, 60 insertions(+), 57 deletions(-) diff --git a/crates/codegen/parser/runtime/src/napi_interface/cst.rs b/crates/codegen/parser/runtime/src/napi_interface/cst.rs index 8cc4c142a0..ad19bfa629 100644 --- a/crates/codegen/parser/runtime/src/napi_interface/cst.rs +++ b/crates/codegen/parser/runtime/src/napi_interface/cst.rs @@ -1,7 +1,7 @@ use std::rc::Rc; -use napi::bindgen_prelude::{Env, ToNapiValue}; -use napi::{JsObject, NapiValue}; +use napi::bindgen_prelude::Env; +use napi::JsObject; use napi_derive::napi; use crate::napi_interface::cursor::Cursor; @@ -54,7 +54,7 @@ impl RuleNode { self.0 .children .iter() - .map(|child| child.to_js(&env)) + .map(|child| child.to_js(env)) .collect() } @@ -118,29 +118,30 @@ impl TokenNode { } } -pub trait ToJS { - fn to_js(&self, env: &Env) -> JsObject; +pub(crate) trait ToJS { + fn to_js(&self, env: Env) -> JsObject; } impl ToJS for Rc { - fn to_js(&self, env: &Env) -> JsObject { - let obj = - unsafe { ::to_napi_value(env.raw(), RuleNode(self.clone())) }; - unsafe { JsObject::from_raw_unchecked(env.raw(), obj.unwrap()) } + fn to_js(&self, env: Env) -> JsObject { + RuleNode(self.clone()) + .into_instance(env) + .expect("Class constructor to be defined by #[napi]") + .as_object(env) } } impl ToJS for Rc { - fn to_js(&self, env: &Env) -> JsObject { - let obj = unsafe { - ::to_napi_value(env.raw(), TokenNode(self.clone())) - }; - unsafe { JsObject::from_raw_unchecked(env.raw(), obj.unwrap()) } + fn to_js(&self, env: Env) -> JsObject { + TokenNode(self.clone()) + .into_instance(env) + .expect("Class constructor to be defined by #[napi]") + .as_object(env) } } impl ToJS for RustNode { - fn to_js(&self, env: &Env) -> JsObject { + fn to_js(&self, env: Env) -> JsObject { match self { RustNode::Rule(rust_rule_node) => rust_rule_node.to_js(env), RustNode::Token(rust_token_node) => rust_token_node.to_js(env), diff --git a/crates/codegen/parser/runtime/src/napi_interface/cursor.rs b/crates/codegen/parser/runtime/src/napi_interface/cursor.rs index b1ffbdf2c4..a91cfaeaa5 100644 --- a/crates/codegen/parser/runtime/src/napi_interface/cursor.rs +++ b/crates/codegen/parser/runtime/src/napi_interface/cursor.rs @@ -54,7 +54,7 @@ impl Cursor { #[napi(ts_return_type = "cst.Node", catch_unwind)] pub fn node(&self, env: Env) -> JsObject { - self.0.node().to_js(&env) + self.0.node().to_js(env) } #[napi(getter, ts_return_type = "kinds.NodeLabel", catch_unwind)] @@ -82,7 +82,7 @@ impl Cursor { pub fn ancestors(&self, env: Env) -> Vec { self.0 .ancestors() - .map(|rust_rule_node| rust_rule_node.to_js(&env)) + .map(|rust_rule_node| rust_rule_node.to_js(env)) .collect() } diff --git a/crates/codegen/parser/runtime/src/napi_interface/parse_output.rs b/crates/codegen/parser/runtime/src/napi_interface/parse_output.rs index b380b1cb46..d1804af45b 100644 --- a/crates/codegen/parser/runtime/src/napi_interface/parse_output.rs +++ b/crates/codegen/parser/runtime/src/napi_interface/parse_output.rs @@ -17,7 +17,7 @@ impl From for ParseOutput { impl ParseOutput { #[napi(ts_return_type = "cst.Node", catch_unwind)] pub fn tree(&self, env: Env) -> napi::JsObject { - self.0.tree().to_js(&env) + self.0.tree().to_js(env) } #[napi(ts_return_type = "Array", catch_unwind)] diff --git a/crates/codegen/parser/runtime/src/napi_interface/templates/ast_selectors.rs.jinja2 b/crates/codegen/parser/runtime/src/napi_interface/templates/ast_selectors.rs.jinja2 index 90a3854d1f..9d3e5ef8aa 100644 --- a/crates/codegen/parser/runtime/src/napi_interface/templates/ast_selectors.rs.jinja2 +++ b/crates/codegen/parser/runtime/src/napi_interface/templates/ast_selectors.rs.jinja2 @@ -290,7 +290,7 @@ impl Selector { } node if filter(node) => { self.index += 1; - return Ok(Some(node.to_js(&self.env))); + return Ok(Some(node.to_js(self.env))); } _ => { break; diff --git a/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/ast_selectors.rs b/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/ast_selectors.rs index 13c7f46e39..27c971e25e 100644 --- a/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/ast_selectors.rs +++ b/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/ast_selectors.rs @@ -3159,7 +3159,7 @@ impl Selector { } node if filter(node) => { self.index += 1; - return Ok(Some(node.to_js(&self.env))); + return Ok(Some(node.to_js(self.env))); } _ => { break; diff --git a/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/cst.rs b/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/cst.rs index e8dcae0dfc..a942ae4a0a 100644 --- a/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/cst.rs +++ b/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/cst.rs @@ -2,8 +2,8 @@ use std::rc::Rc; -use napi::bindgen_prelude::{Env, ToNapiValue}; -use napi::{JsObject, NapiValue}; +use napi::bindgen_prelude::Env; +use napi::JsObject; use napi_derive::napi; use crate::napi_interface::cursor::Cursor; @@ -56,7 +56,7 @@ impl RuleNode { self.0 .children .iter() - .map(|child| child.to_js(&env)) + .map(|child| child.to_js(env)) .collect() } @@ -120,29 +120,30 @@ impl TokenNode { } } -pub trait ToJS { - fn to_js(&self, env: &Env) -> JsObject; +pub(crate) trait ToJS { + fn to_js(&self, env: Env) -> JsObject; } impl ToJS for Rc { - fn to_js(&self, env: &Env) -> JsObject { - let obj = - unsafe { ::to_napi_value(env.raw(), RuleNode(self.clone())) }; - unsafe { JsObject::from_raw_unchecked(env.raw(), obj.unwrap()) } + fn to_js(&self, env: Env) -> JsObject { + RuleNode(self.clone()) + .into_instance(env) + .expect("Class constructor to be defined by #[napi]") + .as_object(env) } } impl ToJS for Rc { - fn to_js(&self, env: &Env) -> JsObject { - let obj = unsafe { - ::to_napi_value(env.raw(), TokenNode(self.clone())) - }; - unsafe { JsObject::from_raw_unchecked(env.raw(), obj.unwrap()) } + fn to_js(&self, env: Env) -> JsObject { + TokenNode(self.clone()) + .into_instance(env) + .expect("Class constructor to be defined by #[napi]") + .as_object(env) } } impl ToJS for RustNode { - fn to_js(&self, env: &Env) -> JsObject { + fn to_js(&self, env: Env) -> JsObject { match self { RustNode::Rule(rust_rule_node) => rust_rule_node.to_js(env), RustNode::Token(rust_token_node) => rust_token_node.to_js(env), diff --git a/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/cursor.rs b/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/cursor.rs index 4455978600..d837dcfb5e 100644 --- a/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/cursor.rs +++ b/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/cursor.rs @@ -56,7 +56,7 @@ impl Cursor { #[napi(ts_return_type = "cst.Node", catch_unwind)] pub fn node(&self, env: Env) -> JsObject { - self.0.node().to_js(&env) + self.0.node().to_js(env) } #[napi(getter, ts_return_type = "kinds.NodeLabel", catch_unwind)] @@ -84,7 +84,7 @@ impl Cursor { pub fn ancestors(&self, env: Env) -> Vec { self.0 .ancestors() - .map(|rust_rule_node| rust_rule_node.to_js(&env)) + .map(|rust_rule_node| rust_rule_node.to_js(env)) .collect() } diff --git a/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/parse_output.rs b/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/parse_output.rs index a2c4c13600..7f276a0ddc 100644 --- a/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/parse_output.rs +++ b/crates/solidity/outputs/cargo/slang_solidity/src/generated/napi_interface/parse_output.rs @@ -19,7 +19,7 @@ impl From for ParseOutput { impl ParseOutput { #[napi(ts_return_type = "cst.Node", catch_unwind)] pub fn tree(&self, env: Env) -> napi::JsObject { - self.0.tree().to_js(&env) + self.0.tree().to_js(env) } #[napi(ts_return_type = "Array", catch_unwind)] diff --git a/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/ast_selectors.rs b/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/ast_selectors.rs index c3460e9f9f..7cea58e359 100644 --- a/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/ast_selectors.rs +++ b/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/ast_selectors.rs @@ -310,7 +310,7 @@ impl Selector { } node if filter(node) => { self.index += 1; - return Ok(Some(node.to_js(&self.env))); + return Ok(Some(node.to_js(self.env))); } _ => { break; diff --git a/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/cst.rs b/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/cst.rs index e8dcae0dfc..a942ae4a0a 100644 --- a/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/cst.rs +++ b/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/cst.rs @@ -2,8 +2,8 @@ use std::rc::Rc; -use napi::bindgen_prelude::{Env, ToNapiValue}; -use napi::{JsObject, NapiValue}; +use napi::bindgen_prelude::Env; +use napi::JsObject; use napi_derive::napi; use crate::napi_interface::cursor::Cursor; @@ -56,7 +56,7 @@ impl RuleNode { self.0 .children .iter() - .map(|child| child.to_js(&env)) + .map(|child| child.to_js(env)) .collect() } @@ -120,29 +120,30 @@ impl TokenNode { } } -pub trait ToJS { - fn to_js(&self, env: &Env) -> JsObject; +pub(crate) trait ToJS { + fn to_js(&self, env: Env) -> JsObject; } impl ToJS for Rc { - fn to_js(&self, env: &Env) -> JsObject { - let obj = - unsafe { ::to_napi_value(env.raw(), RuleNode(self.clone())) }; - unsafe { JsObject::from_raw_unchecked(env.raw(), obj.unwrap()) } + fn to_js(&self, env: Env) -> JsObject { + RuleNode(self.clone()) + .into_instance(env) + .expect("Class constructor to be defined by #[napi]") + .as_object(env) } } impl ToJS for Rc { - fn to_js(&self, env: &Env) -> JsObject { - let obj = unsafe { - ::to_napi_value(env.raw(), TokenNode(self.clone())) - }; - unsafe { JsObject::from_raw_unchecked(env.raw(), obj.unwrap()) } + fn to_js(&self, env: Env) -> JsObject { + TokenNode(self.clone()) + .into_instance(env) + .expect("Class constructor to be defined by #[napi]") + .as_object(env) } } impl ToJS for RustNode { - fn to_js(&self, env: &Env) -> JsObject { + fn to_js(&self, env: Env) -> JsObject { match self { RustNode::Rule(rust_rule_node) => rust_rule_node.to_js(env), RustNode::Token(rust_token_node) => rust_token_node.to_js(env), diff --git a/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/cursor.rs b/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/cursor.rs index 4455978600..d837dcfb5e 100644 --- a/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/cursor.rs +++ b/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/cursor.rs @@ -56,7 +56,7 @@ impl Cursor { #[napi(ts_return_type = "cst.Node", catch_unwind)] pub fn node(&self, env: Env) -> JsObject { - self.0.node().to_js(&env) + self.0.node().to_js(env) } #[napi(getter, ts_return_type = "kinds.NodeLabel", catch_unwind)] @@ -84,7 +84,7 @@ impl Cursor { pub fn ancestors(&self, env: Env) -> Vec { self.0 .ancestors() - .map(|rust_rule_node| rust_rule_node.to_js(&env)) + .map(|rust_rule_node| rust_rule_node.to_js(env)) .collect() } diff --git a/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/parse_output.rs b/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/parse_output.rs index a2c4c13600..7f276a0ddc 100644 --- a/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/parse_output.rs +++ b/crates/testlang/outputs/cargo/slang_testlang/src/generated/napi_interface/parse_output.rs @@ -19,7 +19,7 @@ impl From for ParseOutput { impl ParseOutput { #[napi(ts_return_type = "cst.Node", catch_unwind)] pub fn tree(&self, env: Env) -> napi::JsObject { - self.0.tree().to_js(&env) + self.0.tree().to_js(env) } #[napi(ts_return_type = "Array", catch_unwind)]