Skip to content

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

Open
kevinushey wants to merge 1 commit into
masterfrom
bugfix/module-class-map-collision
Open

kevinushey wants to merge 1 commit 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

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.

1 participant