Merge remote-tracking branch 'origin/master' into bugfix/module-complete-specials

# Conflicts:
#	inst/tinytest/cpp/Module.cpp
#	inst/tinytest/test_module.R

Author: kevinushey

ChangeLog

@@ -73,6 +73,13 @@
 	* inst/tinytest/cpp/Module.cpp: Add classes overloading '[['
 	* inst/tinytest/test_module.R: Test their completions
 
+	* vignettes/rmd/Rcpp-modules.Rmd: Document that objects of exposed
+	classes returned by value or reference are copied, while a returned
+	pointer is owned and eventually deleted by R, so that returning this or
+	another pointer the C++ side still owns deletes the object twice (#1533)
+	* inst/tinytest/cpp/Module.cpp: Add a class counting its destructions
+	* inst/tinytest/test_module.R: Test that R deletes a returned pointer
+
 	* inst/include/Rcpp/Module.h (internal::module_class_pointer): New
 	per-library record of the class_<Class> registered for a C++ type
 	* inst/include/Rcpp/Module.h (internal::make_new_object): Pass it to

inst/tinytest/cpp/Module.cpp

@@ -194,6 +194,26 @@ class ModuleBracketsOnly {
     double value;
 };
 
+// counts its destructions, to check who owns objects returned from C++
+class ModuleOwned {
+public:
+    ModuleOwned(int id_) : id(id_) {}
+    ~ModuleOwned() { destroyed++; }
+    int id;
+    static int destroyed;
+};
+int ModuleOwned::destroyed = 0;
+
+RCPP_EXPOSED_CLASS_NODECL(ModuleOwned)
+
+ModuleOwned* ModuleOwned_create(int id) {
+    return new ModuleOwned(id);
+}
+
+int ModuleOwned_destroyed() {
+    return ModuleOwned::destroyed;
+}
+
 class ModuleGadget {
 public:
     ModuleGadget() {}
@@ -320,6 +340,13 @@ RCPP_MODULE(demoModule) {
         .field("value", &ModuleBracketsOnly::value)
         ;
 
+    class_<ModuleOwned>("ModuleOwned")
+        .field_readonly("id", &ModuleOwned::id)
+        ;
+
+    function("ModuleOwned_create", &ModuleOwned_create);
+    function("ModuleOwned_destroyed", &ModuleOwned_destroyed);
+
     class_<ModuleGadget>("ModuleGadget")
         .constructor()
         .method("value", static_cast<void (ModuleGadget::*)(int)>(&ModuleGadget::value))

inst/tinytest/test_module.R

@@ -175,6 +175,15 @@ expect_equal( Rcpp:::complete( brackets ), c("total( ", "value") )
 expect_equal( brackets[[2L]], 2.0 )
 expect_equal( Rcpp:::complete( new( ModuleBracketsOnly ) ), "value" )
 
+## a pointer returned from C++ is owned by the R object wrapping it, and
+## deleted when that object is garbage collected (#1533)
+destroyed <- ModuleOwned_destroyed()
+owned <- ModuleOwned_create( 7L )
+expect_equal( owned$id, 7L )
+rm( owned )
+invisible( gc() )
+expect_equal( ModuleOwned_destroyed(), destroyed + 1L )
+
 ## a body that throws after registering some of its classes and functions
 ## leaves nothing behind, so loading the module again does not end up with
 ## two copies of what the failed run registered

vignettes/rmd/Rcpp-modules.Rmd

@@ -1013,6 +1013,45 @@ bar$handleFoo(foo)
 #> Got a Foo!
 ```
 
+### Returning objects of exposed classes
+
+With `RCPP_EXPOSED_WRAP` (or `RCPP_EXPOSED_CLASS`), functions and
+methods can also return objects of a class exposed by a module. How
+the object is passed to \proglang{R} depends on the return type:
+
+- An object returned by value or by reference is _copied_, using the
+  copy constructor of the class, and the \proglang{R} object owns the
+  copy.
+- A pointer is not copied: the \proglang{R} object takes _ownership_
+  of the object it points to, and deletes it when the \proglang{R}
+  object is garbage collected. This is what a function returning a
+  newly allocated object needs, like the factories shown above, but
+  it means that a pointer may only be returned when nothing else owns
+  or deletes the object it points to. Returning `this` (e.g. to chain
+  method calls), a pointer to a data member or to an element of a
+  container, or the same pointer from more than one call results in the
+  object being deleted more than once, which typically crashes
+  \proglang{R} during garbage collection.
+
+```{Rcpp ownership, eval=FALSE}
+class Counter {
+public:
+    Counter() : n(0) {}
+
+    // fine: R owns, and eventually deletes, the new Counter
+    static Counter* create() { return new Counter; }
+
+    // wrong: the R object returned by inc() would delete *this
+    // a second time
+    Counter* inc() { n++; return this; }
+
+    // fine: return void, or a copy, and chain the calls in R
+    void increment() { n++; }
+
+    int n;
+};
+```
+
 ### Full example
 
 <!-- TODO: maybe replace this by something from wls or RcppModels ? -->