Repository navigation
wrap returned module objects as the class of the library that returned them - #1532
Open
kevinushey wants to merge 1 commit into
Open
kevinushey wants to merge 1 commit into
kevinushey wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 onlytypeid(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 sametypeidname, 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:internal::module_class_pointer<Class>(): a function-local static that records theclass_<Class>registered by this shared library. The function isattribute_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 predatescpp_object_maker_by_class()(a package built against newer headers), it callscpp_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 byexternalptr_address()).cpp_object_maker_by_class()looks the class up by pointer and creates the object through that class's generator rather thannew(Class, ...), which would resolve the reference class by its (possibly shared) R name. If the pointer isn't registered it defers tocpp_object_maker().cpp_object_maker()itself is unchanged, so packages built against older headers behave exactly as before.Nothing in
Rcpp.soor in the layout of the module classes changes. The fix applies to a package once it is rebuilt.Tests: two small packages,
inst/tinytest/testRcppCollisionAandtestRcppCollisionB, each expose an unrelated class namedCollisionand a function returning one by value.inst/tinytest/test_module_class_collision.Rinstalls both into a temporary library, liketest_module_client_package.Rdoes, and checks that each package's returned object behaves as its own class. It is gated onRunAllRcppTests/RunVerboseRcppTestsand skipped on Windows, like that test.Verification (R 4.6.1, macOS):
collisionB::make_foo()$get()returns2.646569e-312(collisionA'sget()running oncollisionB's object). With this branch, both packages' objects return the right values.testRcppCollisionB::make_collision()$get()gives a single garbage double); on this branch it passes, as dotest_module.R,test_modref.R,test_expose_class.R,test_module_client_package.R,test_wrap.Randtest_rcpp_package_skeleton.RwithRunAllRcppTests=yes RunVerboseRcppTests=yes.Not covered:
sourceCpp()files exposing same-named classes in one session still clash, because both defineRcpp_<name>in.GlobalEnvand the second definition replaces the first. Andnew(pkgB::Foo)still goes through themethodsclass cache by name. Both are part 2 in the issue comment.Checklist
R CMD checkstill passes all tests (module-related test files run locally as above)