Adding Rcpp signature attribute

[eddelbuettel]

This PR extends / revisits #1183 which got close for technical reasons. See #1182 for discussion motivating it.

Checklist

• Code compiles correctly
• R CMD check still passes all tests
• Prefereably, new tests were added which fail without the change
• Document the changes by file in ChangeLog

[Enchufa2]

Some comments:

• @traversc mentioned in Additional Rcpp attributes #1182 that there were a bunch of tests. Are they gonna be part of this PR? It's a nice functionality, but I think it's important we have a number of tests torturing it.
• For example, how does this currently play with the code that transforms C++ default arguments to R arguments?
  For instance, I see that the signature takes precedence here and verbose is FALSE by default:

Rcpp::sourceCpp(code='
#include <Rcpp.h>
using namespace Rcpp;
// [[Rcpp::export( rng = false, signature = {x = list("{A}", "B"), verbose=FALSE} )]]
int test_inline(List x, bool verbose=true) {
  Rcout << "The verbose paramter is set to: ";
  Rcout << verbose << std::endl;
  if(x.size() > 0) {
    CharacterVector first_element = x[0];
    Rcout << "The first element of list x is: ";
    Rcout << first_element << std::endl;
  }
  return x.size();
}')
test_inline()

• What should happen with partial signature specification? This currently compiles, but the execution segfaults:

Rcpp::sourceCpp(code='
#include <Rcpp.h>
using namespace Rcpp;
// [[Rcpp::export( rng = false, signature = {x = list("{A}", "B")} )]]
int test_inline(List x, bool verbose=true) {
  Rcout << "The verbose paramter is set to: ";
  Rcout << verbose << std::endl;
  if(x.size() > 0) {
    CharacterVector first_element = x[0];
    Rcout << "The first element of list x is: ";
    Rcout << first_element << std::endl;
  }
  return x.size();
}')
test_inline()

• Block parsing is a bit fragile. This fails because it takes the } in "{A}" as the end of the block:

Rcpp::sourceCpp(code='
#include <Rcpp.h>
using namespace Rcpp;
// [[Rcpp::export( rng = false, signature = {x = list("{A}", "B"), verbose=FALSE )]]
int test_inline(List x, bool verbose) {
  Rcout << "The verbose paramter is set to: ";
  Rcout << verbose << std::endl;
  if(x.size() > 0) {
    CharacterVector first_element = x[0];
    Rcout << "The first element of list x is: ";
    Rcout << first_element << std::endl;
  }
  return x.size();
}')
test_inline()

• However, this succeeds even if the closing brace is missing:

Rcpp::sourceCpp(code='
#include <Rcpp.h>
using namespace Rcpp;
// [[Rcpp::export( rng = false, signature = {x = list("A", "B"), verbose=FALSE )]]
int test_inline(List x, bool verbose) {
  Rcout << "The verbose paramter is set to: ";
  Rcout << verbose << std::endl;
  if(x.size() > 0) {
    CharacterVector first_element = x[0];
    Rcout << "The first element of list x is: ";
    Rcout << first_element << std::endl;
  }
  return x.size();
}')
test_inline()

• Should we check that the code inside the signature block is valid R?

[traversc]

Should we check that the code inside the signature block is valid R?

How about something like this? (prototyping in R, but could be converted to C++)

check_r_signature <- function(required_args, args) {
  func_sig <- paste0("function(", args, ") {}")
  parsed_args <- formalArgs(eval(parse(text = func_sig)))
  all(required_args %in% parsed_args)
}

required_args <- c("x", "verbose")

check_r_signature(required_args, args='x = list("{A}", "B"), verbose=FALSE')
# TRUE

check_r_signature(required_args, args='x = list("{A}", "B")')
# FALSE

check_r_signature(required_args, args='x = list("{A}",#%), verbose=FALSE')
# error -- could be wrapped in tryCatch?

This checks that all arguments required by the Rcpp function exist in the R signature (so at least it won't segfault in your second example).

Broader question: how much do you need to protect against user error / typos / mistakes?

[Enchufa2]

Broader question: how much do you need to protect against user error / typos / mistakes?

My two cents. Avoiding segfaults is a must have. :) Then, Rcpp::exportAttribute's documentation claims that the syntax is "compatible with the new generalized attributes feature of the C++11 standard". So, ideally, code should compile under a recent standard after removing the comment (i.e., no syntax errors such as missing closing braces). Checking that the signature is valid R code helps with that, and it's probably easier than developing a standard-grade parser. Having specific error messages for specific user errors / typos / mistakes is a nice-to-have feature, but not critical, I'd say, if this feature is well-documented in Rcpp::exportAttribute and a generic error is triggered pointing to such documentation.

[eddelbuettel]

So as expected, the reverse dependency check did not reveal new issues caused by this PR. It is a new code path, it alters an existing access pattern that already is extremely lightly to not all used in standard tests. So no issue there.

But @Enchufa2 makes good points above. We should probably add a bit more unit testing to this before going any further. @Travers, do you think you can make a few tests operational on this?

[eddelbuettel]

I rebased this but apparently made a hash out of things (concurrent edits to ChangeLog can get in the way). So this branch too may become an orphan and I may start over again with the (squashed) commit by @Travers, my follow-up on config.h and maybe just stick the codecov.io yaml update into master first. All cleanup up; lacked tracking for a moment.

[eddelbuettel]

@jjallaire when you have a few moments over a fresh cup of joe for this PR some time this week we would love to hear your input. With a supplementary PR merged into this PR we should be in (generally) good shape.

[eddelbuettel]

Hi @traversc any chance you could comment on the points by @kevinushey ?

[traversc]

Sorry, I didn't think those questions were directed to me.

The first comment on checking sig.empty makes sense.

The question on -1 vs. std::string::npos is stylistic. I think you'd want to make it consistent with the previous line.

The question on error proagation: I believe an error from Rcpp::function propagates correctly, as expected. But how errors from Rcpp::function work is not 100% clear to me.

Let me know what you guys decide and I can write another pull request.

[eddelbuettel]

As for errors: we throw in other parts of src/attributes.cpp too. Same here?

[traversc]

I understand the throwing part, but I'm not sure of the type of error thrown by Rcpp::function is.

The following works:

  Rcpp::Function parse = Rcpp::Environment::base_env()["parse"];
  Rcpp::Function eval = Rcpp::Environment::base_env()["eval"];
  Rcpp::Function formalArgs = Rcpp::Environment::namespace_env("methods")["formalArgs"];
  CharacterVector pargs_cv;


  try {
    pargs_cv = formalArgs(eval(parse(_["text"] = args)));
  } catch(const std::exception & ex) { // what is the correct error type we should catch here?
    std::string err_msg = "error parsing signature attribute\n";
    err_msg += std::string(ex.what());
    throw Rcpp::exception(err_msg.c_str());
  }

But is this right? I don't understand how an R error gets converted into a std::exception. Maybe it is simple and I am overthinking it.

[kevinushey]

My question re: errors was just to confirm that errors (e.g. from invalid signatures) are appropriately handled; it sounds like you've confirmed that's the case in testing. (If that's indeed the case then I don't think any changes are necessary)

[eddelbuettel]

Looks like we're getting really close. @traversc could you maybe add one more tiny PR cleaning up the nudges above?

[eddelbuettel]

No issue arose in the reverse-depends check.

So ... I guess we're all kewl with this now? @kevinushey @jjallaire @Enchufa2 ? Any final glances ?

[Enchufa2]

LGTM!

[kevinushey]

👍

[eddelbuettel]

In it goes --thanks for this, @traversc, and for your patience in dealing with Team Rcpp.