From 22c751ced0da1f9d33edea3dad141e9f37c5371f Mon Sep 17 00:00:00 2001
From: LongYinan <lynweklm@gmail.com>
Date: Thu, 18 Apr 2024 00:14:00 +0800
Subject: [PATCH] fix(napi-derive): bail the unexpected factory directive
 (#2051)

- Close https://github.com/napi-rs/napi-rs/issues/2048
---
 crates/macro/src/parser/mod.rs                  | 17 +++++++++++++----
 .../fn_outside_impl_factory.rs                  | 12 ++++++++++++
 .../fn_outside_impl_factory.stderr              |  5 +++++
 examples/napi/tests/build_error_tests/mod.rs    |  1 +
 examples/napi/tests/macro_tests.rs              |  1 +
 5 files changed, 32 insertions(+), 4 deletions(-)
 create mode 100644 examples/napi/tests/build_error_tests/fn_outside_impl_factory.rs
 create mode 100644 examples/napi/tests/build_error_tests/fn_outside_impl_factory.stderr

diff --git a/crates/macro/src/parser/mod.rs b/crates/macro/src/parser/mod.rs
index 8951ffde..837cce46 100644
--- a/crates/macro/src/parser/mod.rs
+++ b/crates/macro/src/parser/mod.rs
@@ -559,7 +559,7 @@ fn napi_fn_from_decl(
     }
   };
 
-  Diagnostic::from_vec(errors).map(|_| {
+  Diagnostic::from_vec(errors).and_then(|_| {
     let js_name = if let Some(prop_name) = opts.getter() {
       opts.js_name().map_or_else(
         || {
@@ -611,7 +611,16 @@ fn napi_fn_from_decl(
       false
     };
 
-    NapiFn {
+    let kind = fn_kind(opts);
+
+    if !matches!(kind, FnKind::Normal) && parent.is_none() {
+      bail_span!(
+        sig.ident,
+        "Only fn in impl block can be marked as factory, constructor, getter or setter"
+      );
+    }
+
+    Ok(NapiFn {
       name: ident.clone(),
       js_name,
       args,
@@ -619,7 +628,7 @@ fn napi_fn_from_decl(
       is_ret_result,
       is_async: asyncness.is_some(),
       vis,
-      kind: fn_kind(opts),
+      kind,
       fn_self,
       parent: parent.cloned(),
       comments: extract_doc_comments(&attrs),
@@ -638,7 +647,7 @@ fn napi_fn_from_decl(
       catch_unwind: opts.catch_unwind().is_some(),
       unsafe_: sig.unsafety.is_some(),
       register_name: get_register_ident(ident.to_string().as_str()),
-    }
+    })
   })
 }
 
diff --git a/examples/napi/tests/build_error_tests/fn_outside_impl_factory.rs b/examples/napi/tests/build_error_tests/fn_outside_impl_factory.rs
new file mode 100644
index 00000000..d7176e81
--- /dev/null
+++ b/examples/napi/tests/build_error_tests/fn_outside_impl_factory.rs
@@ -0,0 +1,12 @@
+//! This is testing that `#[napi(factory)]` outside of an `impl` block fails
+
+use napi_derive::napi;
+
+#[napi(factory)]
+pub fn add() {
+  println!("Hello, world!");
+}
+
+// Needed for the trybuild tests.
+#[allow(unused)]
+fn main() {}
diff --git a/examples/napi/tests/build_error_tests/fn_outside_impl_factory.stderr b/examples/napi/tests/build_error_tests/fn_outside_impl_factory.stderr
new file mode 100644
index 00000000..fd6c72bb
--- /dev/null
+++ b/examples/napi/tests/build_error_tests/fn_outside_impl_factory.stderr
@@ -0,0 +1,5 @@
+error: Only fn in impl block can be marked as factory, constructor, getter or setter
+ --> tests/build_error_tests/fn_outside_impl_factory.rs:6:8
+  |
+6 | pub fn add() {
+  |        ^^^
diff --git a/examples/napi/tests/build_error_tests/mod.rs b/examples/napi/tests/build_error_tests/mod.rs
index 69fc2836..f9017c4a 100644
--- a/examples/napi/tests/build_error_tests/mod.rs
+++ b/examples/napi/tests/build_error_tests/mod.rs
@@ -1,5 +1,6 @@
 //! Include the test files here so they can be formatted properly with `cargo fmt`
 
+pub mod fn_outside_impl_factory;
 pub mod ts_arg_type_1;
 pub mod ts_arg_type_2;
 pub mod ts_arg_type_3;
diff --git a/examples/napi/tests/macro_tests.rs b/examples/napi/tests/macro_tests.rs
index 0431043f..2df7f626 100644
--- a/examples/napi/tests/macro_tests.rs
+++ b/examples/napi/tests/macro_tests.rs
@@ -8,4 +8,5 @@ mod build_error_tests;
 fn run_build_error_tests() {
   let t = trybuild::TestCases::new();
   t.compile_fail("tests/build_error_tests/ts_arg_type_*.rs");
+  t.compile_fail("tests/build_error_tests/fn_outside_impl_factory.rs");
 }