Skip to content

wrap returned module objects as the class of the library that returned them - #1532

Merged
kevinushey merged 2 commits into
masterfrom
bugfix/module-class-map-collision
Nov 4, 2026
Merged

kevinushey merged 2 commits into
masterfrom
bugfix/module-class-map-collision

Conversation

@kevinushey

Copy link
Copy Markdown
Contributor

Fixes the C++ side of #1531 (part 1 in this comment; part 2, R-level reference class name collisions, is not addressed here).

When C++ returns an object of a module class, make_new_object<Class>() asked R for the reference object with only typeid(Class).name(). R looked that up in a process-wide map holding whichever module class with that name was loaded last. Unrelated classes with the same unqualified name in different packages have the same typeid name, so one package's objects could be wrapped as the other's class, and its methods then ran on the wrong C++ type.

Changes:

  • inst/include/Rcpp/Module.h:
    • New internal::module_class_pointer<Class>(): a function-local static that records the class_<Class> registered by this shared library. The function is attribute_hidden, so each library gets its own instance; with default visibility the dynamic linker can merge vague-linkage statics across libraries. Where the attribute isn't available, it degrades to today's behavior.
    • make_new_object() passes that class pointer to a new R function, cpp_object_maker_by_class(). If the record is empty (e.g. a library returning a class it doesn't expose itself), or the installed Rcpp predates cpp_object_maker_by_class() (a package built against newer headers), it calls cpp_object_maker() as before.
  • inst/include/Rcpp/module/class.h (class_::get_instance): fill in the record when the class is registered.
  • R/Module.R:
    • Module() also registers each class by its pointer (.classes_by_pointer, keyed by externalptr_address()).
    • cpp_object_maker_by_class() looks the class up by pointer and creates the object through that class's generator rather than new(Class, ...), which would resolve the reference class by its (possibly shared) R name. If the pointer isn't registered it defers to cpp_object_maker().
    • cpp_object_maker() itself is unchanged, so packages built against older headers behave exactly as before.

Nothing in Rcpp.so or in the layout of the module classes changes. The fix applies to a package once it is rebuilt.

Tests: two small packages, inst/tinytest/testRcppCollisionA and testRcppCollisionB, each expose an unrelated class named Collision and a function returning one by value. inst/tinytest/test_module_class_collision.R installs both into a temporary library, like test_module_client_package.R does, and checks that each package's returned object behaves as its own class. It is gated on RunAllRcppTests/RunVerboseRcppTests and skipped on Windows, like that test.

Verification (R 4.6.1, macOS):

  • Two packages built from the objects returned from c++ can be wrapped as another library's module class with the same name #1531 example (see the comment): on master collisionB::make_foo()$get() returns 2.646569e-312 (collisionA's get() running on collisionB's object). With this branch, both packages' objects return the right values.
  • Against master the new test fails (testRcppCollisionB::make_collision()$get() gives a single garbage double); on this branch it passes, as do test_module.R, test_modref.R, test_expose_class.R, test_module_client_package.R, test_wrap.R and test_rcpp_package_skeleton.R with RunAllRcppTests=yes RunVerboseRcppTests=yes.

Not covered: sourceCpp() files exposing same-named classes in one session still clash, because both define Rcpp_<name> in .GlobalEnv and the second definition replaces the first. And new(pkgB::Foo) still goes through the methods class cache by name. Both are part 2 in the issue comment.

Checklist

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

@eddelbuettel
eddelbuettel force-pushed the bugfix/module-class-map-collision branch from cf8816b to 1a8d907 Compare November 4, 2026 14:15
@eddelbuettel

Copy link
Copy Markdown
Member

(Trivially) rebased (as the web editor did not cope well enough).

@eddelbuettel eddelbuettel left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaned onto the LLM again to validate and all good.

Nice to see new tests too.

(Whoops. And GH UI was laggy so sent this twice.)

eddelbuettel

This comment was marked as duplicate.

@kevinushey
kevinushey merged commit 561ddc5 into master Nov 4, 2026
28 checks passed
@eddelbuettel
eddelbuettel deleted the bugfix/module-class-map-collision branch November 4, 2026 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants