Feature add error message for incorrect decorators (#180)

Co-authored-by: Jim Hester <[email protected]>

Author: sbearrows

NEWS.md

@@ -5,6 +5,7 @@
 
 # cpp11 0.2.7
 
+* Outputting more informative error message when cpp11 decorators are incorrectly formatted (@sbearrows, #127)
 * Fix spurious diffs from `tools::package_native_routine_registration_skeleton()` by temporarily using C collation (@sbearrows, #171)
 * Fix a transient memory leak for functions that return values from `cpp11::unwind_protect()` and `cpp11::safe` (#154)
 * `cpp_source()` now gets an argument `dir` to allow customized temporary directory to store generated source files.

R/register.R

@@ -46,6 +46,8 @@ cpp_register <- function(path = ".", quiet = FALSE) {
     return(invisible(character()))
   }
 
+  check_valid_attributes(all_decorations)
+
   funs <- get_registered_functions(all_decorations, "cpp11::register", quiet)
 
   package <- desc::desc_get("Package", file = file.path(path, "DESCRIPTION"))
@@ -274,3 +276,22 @@ get_cpp_register_needs <- function() {
   res <- read.dcf(system.file("DESCRIPTION", package = "cpp11"))[, "Config/Needs/cpp11/cpp_register"]
   strsplit(res, "[[:space:]]*,[[:space:]]*")[[1]]
 }
+
+check_valid_attributes <- function(decorations) {
+
+  bad_decor <- !decorations$decoration %in% c("cpp11::register", "cpp11::init")
+
+  if(any(bad_decor)) {
+    lines <- decorations$line[bad_decor]
+    file <- decorations$file[bad_decor]
+    names <- decorations$decoration[bad_decor]
+    bad_lines <- glue::glue_collapse(glue::glue("- Invalid attribute `{names}` on
+                 line {lines} in file '{file}'."), "\n")
+
+    msg <- glue::glue("cpp11 attributes must be either `cpp11::register` or `cpp11::init`:
+      {bad_lines}
+      ")
+    stop(msg, call. = FALSE)
+
+  }
+}

R/source.R

@@ -105,6 +105,9 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu
   suppressWarnings(
     all_decorations <- decor::cpp_decorations(dir, is_attribute = TRUE)
   )
+
+  check_valid_attributes(all_decorations)
+
   cli_suppress(
     funs <- get_registered_functions(all_decorations, "cpp11::register")
   )

tests/testthat/multiple_incorrect.cpp

@@ -0,0 +1,5 @@
+[[cpp11:register]] int foo() { return 1; }
+
+[[cpp11:register]] double bar(bool run) { return 1.0; }
+
+[[cpp11::register]] bool baz(bool run, int value = 0) { return true; }

tests/testthat/single_incorrect.cpp

@@ -0,0 +1 @@
+[[cpp11:register]] int foo() { return 1; }

tests/testthat/test-register.R

@@ -703,3 +703,36 @@ describe("generate_init_functions", {
     expect_equal(generate_init_functions(funs), list(declarations = "\nvoid foo(DllInfo* dll);\nvoid bar(DllInfo* dll);\n", calls = "\n  foo(dll);\n  bar(dll);"))
   })
 })
+
+test_that("check_valid_attributes does not return an error if all registers are correct", {
+  pkg <- local_package()
+  p <- pkg_path(pkg)
+  dir.create(file.path(p, "src"))
+  file.copy(test_path("single.cpp"), file.path(p, "src", "single.cpp"))
+
+  expect_error_free(cpp_register(p))
+
+  pkg <- local_package()
+  p <- pkg_path(pkg)
+  dir.create(file.path(p, "src"))
+  file.copy(test_path("multiple.cpp"), file.path(p, "src", "multiple.cpp"))
+
+  expect_error_free(cpp_register(p))
+})
+
+
+test_that("check_valid_attributes returns an error if one or more registers is incorrect", {
+  pkg <- local_package()
+  p <- pkg_path(pkg)
+  dir.create(file.path(p, "src"))
+  file.copy(test_path("single_incorrect.cpp"), file.path(p, "src", "single_incorrect.cpp"))
+
+  expect_error(cpp_register(p))
+
+  pkg <- local_package()
+  p <- pkg_path(pkg)
+  dir.create(file.path(p, "src"))
+  file.copy(test_path("multiple_incorrect.cpp"), file.path(p, "src", "multiple_incorrect.cpp"))
+
+  expect_error(cpp_register(p))
+})

tests/testthat/test-source.R

@@ -102,3 +102,54 @@ test_that("generate_include_paths handles paths with spaces", {
   }
 })
 
+test_that("check_valid_attributes does not return an error if all registers are correct", {
+  expect_error_free(
+    cpp11::cpp_source(code = '#include <cpp11.hpp>
+  using namespace cpp11::literals;
+  [[cpp11::register]]
+  cpp11::list fn() {
+    cpp11::writable::list x;
+    x.push_back({"foo"_nm = 1});
+    return x;
+  }
+ [[cpp11::register]]
+  cpp11::list fn2() {
+    cpp11::writable::list x;
+    x.push_back({"foo"_nm = 1});
+    return x;
+  }'))
+})
+
+test_that("check_valid_attributes returns an error if one or more registers is incorrect", {
+  expect_error(
+    cpp11::cpp_source(code = '#include <cpp11.hpp>
+  using namespace cpp11::literals;
+  [[cpp11:register]]
+  cpp11::list fn() {
+    cpp11::writable::list x;
+    x.push_back({"foo"_nm = 1});
+    return x;
+  }
+ [[cpp11::register]]
+  cpp11::list fn2() {
+    cpp11::writable::list x;
+    x.push_back({"foo"_nm = 1});
+    return x;
+  }'))
+
+  expect_error(
+    cpp11::cpp_source(code = '#include <cpp11.hpp>
+  using namespace cpp11::literals;
+  [[cpp11:register]]
+  cpp11::list fn() {
+    cpp11::writable::list x;
+    x.push_back({"foo"_nm = 1});
+    return x;
+  }
+ [[cpp11::egister]]
+  cpp11::list fn2() {
+    cpp11::writable::list x;
+    x.push_back({"foo"_nm = 1});
+    return x;
+  }'))
+})