[kevinushey]

Fixes #1533.

A raw pointer (T*) returned from a module function or method is wrapped in a new R object that owns it (make_new_object() creates XPtr<T>(ptr, true)), so the object is deleted when that R object is collected. That is what factory-style code returning new T(...) relies on, so this PR doesn't change it. But it wasn't documented, and returning this (to chain calls), a pointer to a member or container element, or the same pointer twice deletes the object more than once. See #1533 for the reprex.

Changes:

• vignettes/rmd/Rcpp-modules.Rmd: new subsection "Returning objects of exposed classes", after "Extending Rcpp::as and Rcpp::wrap". It explains that objects returned by value or by reference are copied and owned by R, while a returned pointer is owned and eventually deleted by R, so it may only be returned when nothing else owns the object. It lists the patterns that lead to double deletes, with a short example. Only the Rmd source changes; the PDF can be regenerated with the other vignettes at release time.
• inst/tinytest/cpp/Module.cpp, inst/tinytest/test_module.R: a ModuleOwned class that counts its destructions, and a test that a pointer returned from ModuleOwned_create() is deleted once the R object wrapping it is garbage collected. This pins down the documented behavior. It passes on master too, since nothing changes in the code.

Verification:

• Checked the documented copy semantics: modifying the object returned by a method returning T&, or a free function returning T&, leaves the original untouched.
• test_module.R (47/47) passes with RunAllRcppTests=yes (R 4.6.1, macOS).

Checklist

• Code compiles correctly
• R CMD check still passes all tests (test_module.R run locally; documentation-only otherwise)
• Preferably, new tests were added which fail without the change (not applicable: the new test pins existing behavior that is now documented)
• Document the changes by file in ChangeLog

[eddelbuettel]

@kevinushey Can you look into rebasing this one too given the second dual change?

[eddelbuettel]

This looks good to me and beam-1.0 too, see below.

Plan: Integrate Rcpp PR 1534 (document ownership of objects returned to R from modules)

Evaluation

PR 1534 is documentation plus a pinning test, with no C++ behavior change. It
addresses issue #1533: a module function/method returning a raw T* hands
ownership of that object to R (via XPtr<T>(ptr, true)), so the object is
deleted when the R wrapper is collected. That is correct for factory-style
new T(...) returns, but undocumented, and returning this, a member/element
pointer, or the same pointer twice double-frees.

Code inspection confirms the documentation matches make_new_object() / wrap
dispatch: non-pointer returns use make_new_object(new T(object)) (copy, R owns
the copy), pointer returns use make_new_object(ptr) with XPtr(ptr, true) (R
owns and deletes). The ModuleOwned test passes on the current tree after
rebasing (78 -> 80 tests, 0 failures).

The PR is based on a master snapshot before #1528, so the test_module.R hunk
must be rebased (retry tests now sit between Module_scope_is_cleared() and the
flexible-semantics tests). Module.cpp and the vignette apply cleanly; the
ChangeLog hunk needs manual placement.

Changes

1. Test fixtures

File: inst/tinytest/cpp/Module.cpp.

Add, before class ModuleGadget:

// 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;
}

Register, in RCPP_MODULE(demoModule) before class_<ModuleGadget>:

    class_<ModuleOwned>("ModuleOwned")
        .field_readonly("id", &ModuleOwned::id)
        ;

    function("ModuleOwned_create", &ModuleOwned_create);
    function("ModuleOwned_destroyed", &ModuleOwned_destroyed);

2. Tests

File: inst/tinytest/test_module.R, inserted after
expect_true( Module_scope_is_cleared() ) and before the retry tests:

## 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 )

3. Documentation

File: vignettes/rmd/Rcpp-modules.Rmd.

Add the "Returning objects of exposed classes" subsection after "Extending
Rcpp::as and Rcpp::wrap", as in the PR, explaining that objects returned
by value or by reference are copied (R owns the copy), while a returned pointer
is owned and eventually deleted by R, so it may only be returned when nothing
else owns or deletes it. Include the short example showing the safe (new,
void/copy + chain in R) and unsafe (return this) patterns.

4. ChangeLog

Add an entry documenting the vignette and the new test class/test.

No source/header behavior changes; no DESCRIPTION bump.

Verification

R CMD INSTALL --library=/home/edd/lab/lib .
cd inst/tinytest
R_LIBS=/home/edd/lab/lib Rscript -e 'Sys.setenv(RunAllRcppTests="yes",RunVerboseRcppTests="yes"); tinytest::run_test_file("test_module.R")'

Expected: 80 tests, 0 failures (baseline 78). Also build the vignette (Rmd) at
release time.