[kevinushey]

Fixes #1527. Stacked on #1526 (same macro); the base branch is bugfix/module-boot-exceptions, so the diff here only shows this change. Once #1526 is merged I'll retarget this to master.

Module("name", PACKAGE = ...) calls the module's boot function every time it is given a module name, and the boot function re-ran the RCPP_MODULE body each time. The body adds to a static Rcpp::Module that lives as long as the DLL, so every load appended another copy of each constructor and method overload, which shows up in show() and leaks. See #1527 for the reprex.

Change (inst/include/Rcpp/Module.h, RCPP_MODULE):

• A static bool flag next to the static Rcpp::Module records that the body has completed; later calls of the boot function skip the body and just return the module pointer. The flag lives in the same DLL instance as the module object, so a DLL that is unloaded and loaded again starts over with a fresh module and flag, as it should.
• The flag is only set after the body returns normally. If the body throws (now an R error, see turn exceptions during module initialization into r errors #1526), a later load runs it again. That can duplicate whatever was registered before the exception, but that is no worse than today, and the module was unusable anyway.

As with #1526, this is confined to the macro and applies once a package is rebuilt.

Test (inst/tinytest/test_module.R): after sourceCpp() has loaded demoModule, load it twice more with Module(..., mustStart = TRUE) and check that ModuleWorld still has one constructor and one greet() overload. It sits at the end of the file because Module() redefines the module's reference classes.

Verification:

• The reprex from each Module() call re-runs the module body, duplicating constructors and methods #1527 prints 2, 3, 4 constructors/overloads against master (and against turn exceptions during module initialization into r errors #1526), and 1 constructor(s), 1 overload(s) of inc() on every iteration with this branch; show() lists a single constructor and method.
• Without the change the new tests fail (Expected '1', got '3'). With it, test_module.R (49/49), test_modref.R, test_expose_class.R and test_module_client_package.R pass with RunAllRcppTests=yes RunVerboseRcppTests=yes (R 4.6.1, macOS).

Checklist

• Code compiles correctly
• R CMD check still passes all tests (module test files run locally as above)
• Preferably, new tests were added which fail without the change
• Document the changes by file in ChangeLog

[eddelbuettel]

I have a new 'PLAN.md' from the LLM, derived on 'master plus #1526 rebased'.

---

Plan: Implement PR #1528 -- run the module body only once per dll

Summary of findings

• PR: run the module body only once per dll #1528 -- "run the module body only once per dll"
  (author: kevinushey). Fixes each Module() call re-runs the module body, duplicating constructors and methods #1527. Stacked on turn exceptions during module initialization into r errors #1526 (same macro).
• Problem: Module("name", PACKAGE = ...) calls the module's boot function
  every time it is given a module name, and the boot function re-ran the
  RCPP_MODULE body on each call. The body appends to a static Rcpp::Module
  that lives as long as the DLL, so every load appended another copy of each
  constructor and method overload -- visible in show(), and leaking. The
  each Module() call re-runs the module body, duplicating constructors and methods #1527 reprex prints 2, 3, 4 constructors/overloads on master.
• Fix: keep a static bool flag next to the static Rcpp::Module recording
  that the body has completed; later calls of the boot function skip the body
  and just return the module pointer. The flag lives in the same DLL instance
  as the module object, so a DLL that is unloaded and reloaded starts over with
  a fresh module and flag.
• Stacking: the base branch is bugfix/module-boot-exceptions (turn exceptions during module initialization into r errors #1526), so
  the run the module body only once per dll #1528 diff is the delta on top of turn exceptions during module initialization into r errors #1526's version of the macro. The local
  snapshot now corresponds to the rebased turn exceptions during module initialization into r errors #1526 and already contains the
  BEGIN_RCPP / ModuleScope form of RCPP_MODULE.

Local verification of the codebase

• Local tree: /home/edd/tmp/rcpp/Rcpp/ -- snapshot corresponds to the rebased
  PR turn exceptions during module initialization into r errors #1526 (not a git repo). The run the module body only once per dll #1528 change is NOT present.
• RCPP_MODULE macro currently at inst/include/Rcpp/Module.h:641-651, already
  wrapped in BEGIN_RCPP / END_RCPP with an RAII
  Rcpp::internal::ModuleScope _rcpp_module_scope(...) (defined at lines
  613-635, save/restore form).
• The run the module body only once per dll #1528 change does not touch ModuleScope, BEGIN_RCPP/END_RCPP, or the
  XPtr return; it only adds state to the macro.
• inst/tinytest/cpp/Module.cpp defines module demoModule with class
  ModuleWorld: one constructor and one overload each for greet, set,
  set_ref, set_const_ref, clear (lines 217-224). It also defines the
  Module_scope_is_cleared() helper added by turn exceptions during module initialization into r errors #1526 (lines 313-316).
• inst/tinytest/test_module.R currently ends at line 186. The turn exceptions during module initialization into r errors #1526 test
  addition sits after line 109. Requires RunAllRcppTests=yes and the
  Rcpp modules capability; does sourceCpp("cpp/Module.cpp") at line 23.
• R-side accessors (confirmed in Rcpp/R/Module.R and Rcpp/R/00_classes.R):
  • Module("demoModule", PACKAGE = "Rcpp", mustStart = TRUE) returns a
    Module object; m$ModuleWorld returns the stored C++Class info object
    (Module.R:286-289 stores the info object in storage under the demangled
    class name; the Module $ method at Module.R:118 reads storage[[name]]).
  • Constructors: m$ModuleWorld@constructors is a list -> length(...).
  • Method overloads: m$ModuleWorld@methods[["greet"]]@size is an integer
    holding the number of greet overloads.
• Toolchain available: R 4.6.2 (/usr/lib/R).

Assessment

Correct and well-scoped. The fix addresses the reported duplication at the
right layer (the RCPP_MODULE macro) and takes effect once a package is
rebuilt, consistent with #1526. Putting the flag in the macro keeps it in the
same DLL instance as the module object, so unload/reload semantics are correct.

Concerns

1. Flag set only on normal return. If the body throws (now an R error, see
   turn exceptions during module initialization into r errors #1526), the flag is not set and a later load runs the body again, which can
   duplicate whatever was registered before the exception. This is no worse than
   today and the module was unusable anyway, but it is worth a comment in the
   macro so the intent is explicit.
2. Guard placement. ModuleScope must be constructed on every call (it
   saves/restores the previous scope), so only the _init() call should be
   inside the if (...), not the ModuleScope creation or the XPtr return.
   Getting this wrong would silently break turn exceptions during module initialization into r errors #1526's save/restore behavior.
3. show() / refclass re-definition. Module() redefines the module's
   reference classes, so the new test must be appended at the end of
   test_module.R; placing it earlier can disturb later expectations.
4. @size-style assertions. The exact slot shape of C++OverloadedMethods
   should be validated by running the module test, not assumed.

Implementation steps

1. inst/include/Rcpp/Module.h

Relative to the current post-#1526 macro, add a static bool line, wrap only
_init() in an if, and set the flag after it returns:

#define RCPP_MODULE(name)                                                         \
    void _rcpp_module_##name##_init() ;                                           \
    static Rcpp::Module _rcpp_module_##name( # name ) ;                           \
    static bool _rcpp_module_##name##_initialized = false ;                      \
    extern "C" SEXP _rcpp_module_boot_##name(){                                   \
        BEGIN_RCPP                                                                \
        if( ! _rcpp_module_##name##_initialized ){                                \
            Rcpp::internal::ModuleScope _rcpp_module_scope( & _rcpp_module_##name ) ; \
            _rcpp_module_##name##_init( ) ;                                       \
            _rcpp_module_##name##_initialized = true ;                            \
        }                                                                         \
        return Rcpp::XPtr<Rcpp::Module>( & _rcpp_module_##name , false ) ;        \
        END_RCPP                                                                  \
    }                                                                             \
    void _rcpp_module_##name##_init()

2. inst/tinytest/test_module.R

Append at the end of the file (after line 186):

## the module body runs only once per DLL, so re-loading a module must not
## duplicate its constructors or method overloads (#1527)
loadDemoModule <- function() {
    m <- Module("demoModule", PACKAGE = "Rcpp", mustStart = TRUE)
    Module("demoModule", PACKAGE = "Rcpp", mustStart = TRUE)
    Module("demoModule", PACKAGE = "Rcpp", mustStart = TRUE)
    m
}
m <- loadDemoModule()
expect_equal(length(m$ModuleWorld@constructors), 1L)
expect_equal(m$ModuleWorld@methods[["greet"]]@size, 1L)

3. ChangeLog

Do not alter (per instruction). Note that #1528's own ChangeLog entry would
normally record that RCPP_MODULE runs the module body once per DLL; it is
omitted here.

4. DESCRIPTION

Do not alter (per instruction).

Verification

1. R CMD INSTALL /home/edd/tmp/rcpp/Rcpp into /home/edd/tmp/lib/ -- the macro
   change compiles and headers resolve.
2. Reprex each Module() call re-runs the module body, duplicating constructors and methods #1527: loop Module("demoModule", PACKAGE = "Rcpp", mustStart = TRUE)
   and check the counts stay at one constructor and one greet() overload on
   every iteration (2, 3, 4... on master).
3. RunAllRcppTests=yes RunVerboseRcppTests=yes -> test_module.R (49/49),
   test_modref.R, test_expose_class.R, test_module_client_package.R.
4. Confirm the new assertions fail without the flag change
   (Expected '1', got '3'), then pass with it.

Open question

• Whether to also keep a brief ChangeLog note for run the module body only once per dll #1528 (omitted here per the
  instruction not to alter ChangeLog).

[eddelbuettel]

Given the slight modification that #1526 brought maybe I will let this be re-analyzed once we merged #1526.

[eddelbuettel]

Rebased with master which contains the updated #1526 whereas this started from the initial #1526.

[eddelbuettel]

Here is an updated review of the rebased PR.

---

Plan (v2): Review PR #1528 against the merged PR #1526

Summary of findings

• PR: run the module body only once per dll #1528 -- "run the module body only once per dll"
  (author: kevinushey). Fixes each Module() call re-runs the module body, duplicating constructors and methods #1527.
• Status change: the PR was retargeted from bugfix/module-boot-exceptions
  (turn exceptions during module initialization into r errors #1526) to master. It is no longer stacked; it was rebased on top of the
  merged turn exceptions during module initialization into r errors #1526. It now has 2 commits and touches 3 files -- ChangeLog,
  inst/include/Rcpp/Module.h, inst/tinytest/test_module.R (38 insertions,
  11 deletions).
• Purpose: Module("name", PACKAGE = ...) calls the module's boot function
  every time, and the boot function re-ran the RCPP_MODULE body on each call.
  The body appends to a static Rcpp::Module that lives as long as the DLL, so
  every load appended another copy of each constructor and method overload --
  visible in show(), and leaking. The each Module() call re-runs the module body, duplicating constructors and methods #1527 reprex prints 2, 3, 4
  constructors/overloads on master.
• Snapshot state: the Rcpp/ directory now contains the rebased turn exceptions during module initialization into r errors #1526 and
  the run the module body only once per dll #1528 change. Rcpp.prev/ is the previous (turn exceptions during module initialization into r errors #1526-only) snapshot.

Local verification of the codebase

• Local tree: /home/edd/tmp/rcpp/Rcpp/ -- corresponds to the merged turn exceptions during module initialization into r errors #1526 plus
  run the module body only once per dll #1528 (not a git repo). Rcpp.prev/ holds the pre-run the module body only once per dll #1528 snapshot.

• RCPP_MODULE macro now at inst/include/Rcpp/Module.h:639-657: the boot
  function is wrapped in BEGIN_RCPP / END_RCPP, uses the RAII
  Rcpp::internal::ModuleScope (lines 613-635, save/restore form) from turn exceptions during module initialization into r errors #1526,
  and adds the run the module body only once per dll #1528 once-only flag:

  static bool _rcpp_module_##name##_initialized = false ;
  extern "C" SEXP _rcpp_module_boot_##name(){
      BEGIN_RCPP
      if( ! _rcpp_module_##name##_initialized ){
          Rcpp::internal::ModuleScope _rcpp_module_scope( & _rcpp_module_##name ) ;
          _rcpp_module_##name##_init( ) ;
          _rcpp_module_##name##_initialized = true ;
      }
      return Rcpp::XPtr<Rcpp::Module>( & _rcpp_module_##name , false ) ;
      END_RCPP
  }

• inst/tinytest/test_module.R:193-201 -- the once-only test appended at the end
  of the file (the current PR form):

  is_demo_module <- function(dll) is.loaded("_rcpp_module_boot_demoModule", PACKAGE = dll[["name"]])
  dll <- Filter(is_demo_module, getLoadedDLLs())[[1L]]
  mod <- Module( "demoModule", PACKAGE = dll, mustStart = TRUE )
  mod <- Module( "demoModule", PACKAGE = dll, mustStart = TRUE )
  expect_equal( length( mod$ModuleWorld@constructors ), 1L )
  expect_equal( length( mod$ModuleWorld@methods$greet@nargs ), 1L )

• ChangeLog:44-50 -- the run the module body only once per dll #1528 entry is present, retargeted (standalone, no
  longer nested under the turn exceptions during module initialization into r errors #1526 entry).

• DESCRIPTION version is 1.1.2.7.3 (one micro bump above Rcpp.prev's
  1.1.2.7.2).

• R-side data model confirmed:

  • C++Class@constructors is a list (R/00_classes.R:93-99) -> length().
  • C++Class@methods is a named list; each element a C++OverloadedMethods
    object (inst/include/Rcpp/module/class.h:464-481, res.names() = mnames).
  • C++OverloadedMethods@nargs is an integer vector, one entry per overload
    (inst/include/Rcpp/Module.h:345-349: nargs[i] = met->nargs()), so
    length( ...@nargs ) counts the number of overloads.
  • Module() accepts a DLLInfo for PACKAGE and, at
    R/Module.R:164-165, forces mustStart = TRUE for one
    (if (inherits(module, "DLLInfo") && missing(mustStart)) mustStart <- TRUE).

• Toolchain: R 4.6.2 (/usr/lib/R); a local library exists at
  /home/edd/tmp/lib.

Assessment

PR #1528 is in order and remains valid on top of the updated #1526. The
snapshot already carries the change, and the three review questions raised for
the stacked version all resolve correctly:

1. Guard placement: ModuleScope is constructed on every call (inside the
   if, but the if only guards the body-plus-flag), so turn exceptions during module initialization into r errors #1526's save/restore
   semantics per boot call are preserved. Only _init() and the flag assignment
   sit inside the if; the XPtr return and BEGIN_RCPP/END_RCPP are
   unchanged. Correct.
2. Flag lifetime: the flag is in the same DLL instance as the static module,
   so a DLL that is unloaded and reloaded starts over with a fresh module and
   flag. Correct.
3. Failure path: the flag is set only after _init() returns normally. If
   the body throws (now an R error, per turn exceptions during module initialization into r errors #1526), the flag is not set and a later
   load re-runs the body. This can duplicate whatever was registered before the
   exception, but is no worse than the pre-run the module body only once per dll #1528 behavior and the module was
   unusable anyway. Acceptable, and matches the PR description.

The #1528 test is the current, more robust form: it locates the DLL that owns
_rcpp_module_boot_demoModule via getLoadedDLLs() and passes that DLLInfo
as PACKAGE, rather than assuming the module lives in the Rcpp namespace. It
asserts one constructor and one greet overload after two extra loads.

Concerns

1. ChangeLog artifact (Rcpp/ChangeLog:58-60): the turn exceptions during module initialization into r errors #1526 entry contains two
   contradictory lines --

   * inst/include/Rcpp/Module.h (internal::ModuleScope): New RAII guard
   restoring the previous module scope however the module body exits
   clearing the current module scope however the module body exits
   

   The "clearing ..." line is a leftover from the earlier
   setCurrentScope(0) implementation that the "restore previous" commit
   superseded. It is cosmetic (ChangeLog only), but it is inconsistent with the
   shipped ModuleScope (which restores). Left for a maintainer decision; per
   the sources skill the ChangeLog must not be edited unless explicitly
   instructed.

2. No action required for run the module body only once per dll #1528 itself; the code and test are already the
   final versions.

Implementation steps

Because #1528 is already present and correct, this plan requires no code
change. The steps are review/verification only:

1. Confirm the snapshot contents (done)

• RCPP_MODULE has the once-only flag and guard (Module.h:639-657).
• test_module.R has the repeated-load test (:193-201).
• ChangeLog has the run the module body only once per dll #1528 entry (:44-50).

2. Optional, only if explicitly instructed

• Remove the stale "clearing the current module scope ..." line at
  ChangeLog:60.
• Per the sources skill, do not alter DESCRIPTION, add to ChangeLog, or
  add to inst/NEWS.Rd unless explicitly instructed.

Verification

1. R CMD INSTALL /home/edd/tmp/rcpp/Rcpp into /home/edd/tmp/lib/ -- the macro
   compiles and headers resolve on top of the updated turn exceptions during module initialization into r errors #1526.
2. RunAllRcppTests=yes RunVerboseRcppTests=yes -> test_module.R (expect the
   full file to pass, including the new repeated-load assertions), then
   test_modref.R, test_expose_class.R, test_module_client_package.R.
3. each Module() call re-runs the module body, duplicating constructors and methods #1527 reprex: loop Module("demoModule", ..., mustStart = TRUE) and confirm
   the counts stay at one constructor and one greet() overload on every
   iteration (2, 3, 4... on master / on turn exceptions during module initialization into r errors #1526 alone).
4. Confirm the new assertions fail without the once-only flag
   (Expected '1', got '3'), then pass with it.
5. Sanity-check that the run the module body only once per dll #1528 test still loads the module correctly when the
   demo module was built by sourceCpp("cpp/Module.cpp") earlier in the same
   test run (the getLoadedDLLs() lookup assumes exactly one DLL exports the
   symbol).

Open questions

• Whether the maintainer wants the stale ChangeLog line cleaned up.
• Whether to keep the combined test in test_module.R or split the run the module body only once per dll #1528
  repeated-load check into its own file.

[eddelbuettel]

I made the suggested one-line change to ChangeLog, and also remove two redundant 'date' paragraph headers.