implement nested references

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 -   Support storing of a custom per-isolate data structure (`Napi::Env::GetInstanceData` and `Napi::Env::SetInstanceData`)
 -   Specify the memory ownership rules for `Buffer`
+-   Support returning references to nested objects
 -   Fix a minor memory leak when calling method with incorrect number of arguments
 
 ### [1.1.1] 2023-12-01

README.md

@@ -426,7 +426,7 @@ The `Nobind::ReturnShared` signals `nobind17` that C++ objects returned by this
 
 `.create()` is a method that creates new objects. The `Nobind::ReturnOwned` signals `nobind17` that C++ objects returned by this method should be considered new objects and should be freed when the GC destroys the JS proxy.
 
-Also, be sure to check [#1](https://github.com/mmomtchev/nobind17/issues/1) for a very important warning about shared references.
+Also, be sure to check [#1](https://github.com/mmomtchev/nobind17/issues/1) for a very important warning about shared references and also read the section on nested references below.
 
 ### Extending classes
 
@@ -469,6 +469,27 @@ NOBIND_MODULE(native, m) {
 }
 ```
 
+### Nested references
+
+Consider the following C++ code:
+
+```cpp
+class Time {
+  unsigned long timestamp;
+public:
+  Time(unsigned long v);
+};
+
+class DateTime {
+  Time time;
+public:
+  DateTime(Time v);
+  operator Time &();
+};
+```
+
+`DateTime` can returned a (non-`const`) reference to its member object `Time`. This reference should obviously use shared semantics as the newly created JS proxy object won't own the underlying C++ object. However, what will happen if the GC collects the parent object while JavaScript is still holding a reference to the returned nested object? This special case, which is somewhat common in the C++ world, requires special handling that can be enabled by using the `Nobind::ReturnNested` return attribute. In this case the returned reference will be bound the parent object which will be protected from the GC until the nested reference exists. This return attribute has a meaning only for class members and it is applied by default for class getters.
+
 ### Storing custom per-isolate data
 
 Sometimes a module needs to store *"global"* data. With `node-addon-api` the proper way to store this data is in a per-isolate data structure - since Node.js is allowed to call the same instance from multiple independent isolates. To access the per-isolate storage with `nobind17`, declare the module specific structure and then use the standard `node-addon-api` calls to access it:

include/noattributes.h

@@ -40,7 +40,7 @@ constexpr PropertyAttribute ReadWrite = PropertyAttribute();
 
 class ReturnAttribute : public Attribute {
 public:
-  enum Return { Shared = 0x1, Owned = 0x2 };
+  enum Return { Shared = 0x1, Owned = 0x2, Nested = 0x40 };
   enum Execution { Sync = 0x4, Async = 0x8 };
   enum Null { Allowed = 0x10, Forbidden = 0x20 };
 
@@ -53,6 +53,7 @@ class ReturnAttribute : public Attribute {
   }
   constexpr bool isShared() const { return (flags & Shared) == Shared; }
   constexpr bool isOwned() const { return (flags & Owned) == Owned; }
+  constexpr bool isNested() const { return (flags & Nested) == Nested; }
   constexpr bool isReturnNullAccept() const { return (flags & Allowed) == Allowed; }
   constexpr bool isReturnNullThrow() const { return (flags & Forbidden) == Forbidden; }
   constexpr bool isAsync() const { return (flags & Async) == Async; }
@@ -61,6 +62,8 @@ class ReturnAttribute : public Attribute {
       return false;
     if (isOwned())
       return true;
+    if (isNested())
+      return false;
     return DEFAULT;
   }
 
@@ -79,6 +82,12 @@ constexpr ReturnAttribute ReturnOwned = ReturnAttribute(ReturnAttribute::Owned);
  */
 constexpr ReturnAttribute ReturnShared = ReturnAttribute(ReturnAttribute::Shared);
 
+/**
+ * Returned objects will be bound the to the lifespan of the parent, valid
+ * only for class methods
+ */
+constexpr ReturnAttribute ReturnNested = ReturnAttribute(ReturnAttribute::Nested);
+
 /**
  * Returned pointers will be freed and returned references won't be freed (this is the default)
  */

include/noobject.h

@@ -24,19 +24,25 @@ template <typename CLASS> class NoObjectWrap : public Napi::ObjectWrap<NoObjectW
   template <typename T> friend class Typemap::FromJS;
   template <typename T, const ReturnAttribute &RETATTR> friend class Typemap::ToJS;
 
+  // Async worker for async class methods, the wrapper is a private method below
   template <const ReturnAttribute &RETATTR, typename BASE, auto FUNC, typename RETURN, typename... ARGS>
   class MethodWrapperTasklet : public Napi::AsyncWorker {
     Napi::Env env_;
     Napi::Promise::Deferred deferred_;
     std::unique_ptr<ToJS_t<RETURN, RETATTR>> output;
+    // FromJS wrappers also contain persistent references to their underlying JS values
     std::tuple<FromJS_t<ARGS>...> args_;
+    // This is the This persistent
+    Napi::ObjectReference this_ref;
+    // This is the This wrapper
+    NoObjectWrap<CLASS> *wrapper_;
     BASE *self_;
 
   public:
-    MethodWrapperTasklet(Napi::Env env, Napi::Promise::Deferred deferred, CLASS *self,
+    MethodWrapperTasklet(Napi::Env env, Napi::Promise::Deferred deferred, CLASS *self, NoObjectWrap<CLASS> *wrapper,
                          std::tuple<FromJS_t<ARGS>...> &&args)
         : AsyncWorker(env, "nobind_AsyncWorker"), env_(env), deferred_(deferred), output(), args_(std::move(args)),
-          self_(static_cast<BASE *>(self)) {}
+          this_ref(Napi::Persistent(wrapper->Value())), wrapper_(wrapper), self_(static_cast<BASE *>(self)) {}
 
     template <std::size_t... I> void ExecuteImpl(std::index_sequence<I...>) {
       try {
@@ -61,8 +67,7 @@ template <typename CLASS> class NoObjectWrap : public Napi::ObjectWrap<NoObjectW
         deferred_.Resolve(env_.Undefined());
       } else {
         try {
-          auto result = output->Get();
-          deferred_.Resolve(result);
+          deferred_.Resolve(wrapper_->SetupNested<RETATTR>(output->Get()));
         } catch (const std::exception &e) {
           deferred_.Reject(Napi::String::New(env_, e.what()));
         }
@@ -113,7 +118,7 @@ template <typename CLASS> class NoObjectWrap : public Napi::ObjectWrap<NoObjectW
 
   template <typename T, T CLASS::*MEMBER> Napi::Value GetterWrapper(const Napi::CallbackInfo &info) {
     Napi::Env env = info.Env();
-    return ToJS<T, ReturnDefault>(env, self->*MEMBER).Get();
+    return SetupNested<ReturnNested>(ToJS<T, ReturnNested>(env, self->*MEMBER).Get());
   }
 
   template <typename T, T CLASS::*MEMBER> void SetterWrapper(const Napi::CallbackInfo &info, const Napi::Value &val) {
@@ -187,7 +192,7 @@ template <typename CLASS> class NoObjectWrap : public Napi::ObjectWrap<NoObjectW
         // Call the ToJS constructor
         auto output = ToJS_t<RETURN, RETATTR>(env, result);
         // Convert
-        return output.Get();
+        return SetupNested<RETATTR>(output.Get());
         // FromJS/ToJS objects are destroyed
       }
     } catch (const std::exception &e) {
@@ -222,6 +227,7 @@ template <typename CLASS> class NoObjectWrap : public Napi::ObjectWrap<NoObjectW
     return MethodWrapperAsync<RETATTR, BASE, RETURN, FUNC, ARGS...>(info, std::index_sequence_for<ARGS...>{});
   }
 
+  // The actual wrapper for async class methods
   template <const ReturnAttribute &RETATTR, typename BASE, typename RETURN, auto FUNC, typename... ARGS,
             std::size_t... I>
   inline Napi::Value MethodWrapperAsync(const Napi::CallbackInfo &info, std::index_sequence<I...>) {
@@ -239,7 +245,7 @@ template <typename CLASS> class NoObjectWrap : public Napi::ObjectWrap<NoObjectW
       // Alas, std::forward_as_tuple does not guarantee
       // the evaluation order of its arguments, only *braced-init-list* lists do
       // https://en.cppreference.com/w/cpp/language/list_initialization
-      auto tasklet = new MethodWrapperTasklet<RETATTR, BASE, FUNC, RETURN, ARGS...>(env, deferred, self,
+      auto tasklet = new MethodWrapperTasklet<RETATTR, BASE, FUNC, RETURN, ARGS...>(env, deferred, self, this,
                                                                                     {FromJSArgs<ARGS>(info, idx)...});
       try {
         CheckArgLength(env, idx, info.Length());
@@ -303,14 +309,26 @@ template <typename CLASS> class NoObjectWrap : public Napi::ObjectWrap<NoObjectW
         // Call the ToJS constructor
         auto output = ToJS_t<RETURN, RETATTR>(env, result);
         // Convert
-        return output.Get();
+        return SetupNested<RETATTR>(output.Get());
         // FromJS/ToJS objects are destroyed
       }
     } catch (const std::exception &e) {
       throw Napi::Error::New(env, e.what());
     }
   }
 
+  // Setup nested objects
+  template <const ReturnAttribute &RETATTR> inline Napi::Value SetupNested(Napi::Value returned) {
+    if constexpr (RETATTR.isNested()) {
+      if (returned.IsObject()) {
+        // We simply attach the parent (this) as a hidden property in the nested object
+        // This way the parent cannot be GCed until the nested objects is GCed
+        returned.ToObject().DefineProperty(Napi::PropertyDescriptor::Value("__nobind_parent_reference", this->Value()));
+      }
+    }
+    return returned;
+  }
+
   // To look up the class constructor in the per-instance data
   static size_t class_idx;
   // Mainly for debug purposes

test/fixtures/nested.cc

@@ -0,0 +1,7 @@
+#include "nested.h"
+
+Time::Time(unsigned long v) : timestamp(v){};
+Time::operator unsigned long() const { return timestamp; };
+
+DateTime::DateTime(Time v) : time(v){};
+DateTime::operator Time &() { return time; };

test/fixtures/nested.h

@@ -0,0 +1,14 @@
+class Time {
+  unsigned long timestamp;
+
+public:
+  Time(unsigned long v);
+  operator unsigned long() const;
+};
+
+class DateTime {
+public:
+  Time time;
+  DateTime(Time v);
+  operator Time &();
+};

test/tests/nested.cc

@@ -0,0 +1,15 @@
+#include <fixtures/nested.h>
+
+#include <nobind.h>
+
+NOBIND_MODULE(nested, m) {
+  m.def<Time>("Time").cons<unsigned long>().def < &Time::operator unsigned long>("get");
+
+  m.def<DateTime>("DateTime")
+      .cons<Time>()
+      .cons<unsigned long>()
+      // Getters of object members automatically return nested references
+      .def<&DateTime::time>("time")
+      // Explicitly return a nested reference
+      .def<&DateTime::operator Time &, Nobind::ReturnNested>("get");
+}

test/tests/nested.js

@@ -0,0 +1,42 @@
+const { assert } = require('chai');
+
+describe('nested objects', () => {
+  it('constructors', () => {
+    const time = new dll.Time(100);
+    assert.instanceOf(time, dll.Time);
+
+    const dt1 = new dll.DateTime(100);
+    assert.instanceOf(dt1, dll.DateTime);
+
+    const dt2 = new dll.DateTime(time);
+    assert.instanceOf(dt2, dll.DateTime);
+  });
+
+  it('operators', () => {
+    const time = new dll.Time(100);
+    assert.isNumber(time.get());
+    assert.strictEqual(time.get(), 100);
+
+    const dt1 = new dll.DateTime(101);
+    assert.instanceOf(dt1, dll.DateTime);
+    assert.strictEqual(dt1.get().get(), 101);
+
+    const dt2 = new dll.DateTime(time);
+    assert.instanceOf(dt2, dll.DateTime);
+    assert.strictEqual(dt2.get().get(), 100);
+  });
+
+  it('explicit nested references', () => {
+    const dt = new dll.DateTime(new dll.Time(111));
+    assert.instanceOf(dt, dll.DateTime);
+    assert.strictEqual(dt.get().__nobind_parent_reference, dt);
+    assert.strictEqual(dt.get().get(), 111);
+  });
+
+  it('implicit nested references', () => {
+    const dt = new dll.DateTime(new dll.Time(117));
+    assert.instanceOf(dt, dll.DateTime);
+    assert.strictEqual(dt.time.__nobind_parent_reference, dt);
+    assert.strictEqual(dt.get().get(), 117);
+  });
+});