is__module_object leads to error when checking non-module object

[tqchen]

#include <Rcpp.h>

using namespace Rcpp;

class MyClass{
};

RCPP_EXPORT_CALSS(MyClass)

// [[Rcpp::export]]
bool check(RObject m)
{
    return is<MyClass>(m);
}

RCPP_MODULE(test) {
     class_<MyClass>("MyClass");
}

If we call check from R's side by passing in a non-module object. It will report error "External pointer is expected", instead of returning false.

The problem is here

Rcpp/inst/include/Rcpp/api/meat/is.h

Line 153 in 0a43747

XPtr<class_Base> xp( env.get(".cppclass"));

Maybe need a explicit check of ext ptr type first before converting to XPtr

[thirdwing]

if (TYPEOF(m) == EXTPTRSXP)

[tqchen]

Directly check won't work, since that object is a S4, and I guess we need to check type of env.get(".cppclass")

[thirdwing]

See the code below, I think it is what you want.

#include <Rcpp.h>                                                               

// [[Rcpp::export]]                                                                                                 
void check(Rcpp::RObject m) { 
    SEXP attrs = ATTRIB(m);                     
    Rcpp::List lst(attrs);                                                      
    std::vector<std::string> names = lst.names();                               
    for (size_t i = 0; i < names.size(); i++) {                                 
        Rcpp::Rcout << names[i] << std::endl;                                   
    }                                                                           

    Rcpp::List lst2 = Rcpp::List(lst[1]);                                       

    for (size_t i = 0; i < lst2.size(); i++) {  
        Rcpp::Rcout << Rcpp::as<std::string>(lst2[i]) << std::endl;   
    }                                                                           
}       

> x = as.array(c(1,2,3))
> mat = mx.nd.array(x, mx.cpu(0))
> check(mat)
.xData
class
Rcpp_MXNDArray

[tqchen]

Yap :) However, I think this may be something that should be solved by Rcpp::is<NDArray>(mat)
in the future

[eddelbuettel]

But

edd@don:~/git/rcpp(master)$ ag NDArray
edd@don:~/git/rcpp(master)$ 

ie NDArray is not a type we know, define or support. How could we have a proper is<> for it then?

[tqchen]

Sorry for confusion, NDArray is a class declared by RCPP_EXPORT_CLASS.

Discussed with @thirdwing , seems it is indeed a logical error in current is. Can be fixed by either

check if object is S4 here

Rcpp/inst/include/Rcpp/is.h

Lines 38 to 43 in 0a43747

	inline bool is__dispatch( SEXP x, Rcpp::traits::false_type ){
	return is__simple<T>( x ) ;
	}
	template <typename T>
	inline bool is__dispatch( SEXP x, Rcpp::traits::true_type ){

check if return value of env.get is R_NilValue here

Rcpp/inst/include/Rcpp/api/meat/is.h

Line 153 in 0a43747

XPtr<class_Base> xp( env.get(".cppclass"));

[tqchen]

In current code, say if MyClass is exported by RCPP_EXPORT_CLASS. And we check

is<MyClass>(some INTEXP )

It dispatches to

Rcpp/inst/include/Rcpp/api/meat/is.h

Line 151 in 0a43747

inline bool is_module_object_internal(SEXP obj, const char* clazz){

And env.get(".cppclass") cannot successfully return a valid external pointer because it does not exist.

So instead of return a false, it throws an Error saying things cannot be converted to XPtr

[tqchen]

To fix this, we can check if the return type of env.get(".cppclass") is External Ptr here, and simply return false if it is not.

[eddelbuettel]

If you want/need special behaviour for your class, can you not write a function in your library to deal with it?

I do not plan to rapid-fire change things in Rcpp without clearer motivation.

[tqchen]

To be clear, this is not special behavior? As the MyClass is exposed by RCPP_MODULE already, and declared by RCPP_EXPORT_CLASS.

I suppose that the check is<MyClass>(sexp) should return false for the normal integer SEXP? I am totally fine not promoting such change in Rcpp, but I do see this is can benefit users of RCPP_MODULE

[eddelbuettel]

I may still misunderstand the issue but I think you expect Rcpp Modules to do more than it does, and apart from you nobody has brought such expectations forward in the five years we used Rcpp Modules.

So my intuitive feeling is that maybe you need to adjust your expectations. Rcpp Modules is not a parser of your class or library code. It does some things well enough, and things that are missing ... will be missing until someone (you ?) contributes them. But posting issue after issue "it should do this or that" will not get us there. Sorry. Again, I may be missing something obvious too.

But maybe you can start by package-local code in your package, and then try to convince us that some generic code is general enough to be moved over into Rcpp itself?

[tqchen]

Thanks Dirk, Yes. I think we will do it in this way. I am opening the issue to suggest solutions(as in my previous post) and see if it makes sense. I guess writing it out will make it more clear.

[tqchen]

created a PR #381 on this

[thirdwing]

Fixed in #381

[eddelbuettel]

Strictly speaking we don't know yet as the tests are not yet done :) But appreciate the close nonetheless. And we will get back to your gist and work down the issue tickets. Promised.

[thirdwing]

Sorry for this.

[Edited] Maybe not very clear in this morning. I thought you have tested before merging.

[eddelbuettel]

It is more work to get a neutral branch to build from so I just merged and am testing now ...

[eddelbuettel]

Looks good -- it just finished and we have what appears to be no new issue. Just the usual problem of tests failing when Suggests: packages are needed for tests and examples as I install only Depends:...