From a63472b6a6beacb83445db1213ae83905ba3721b Mon Sep 17 00:00:00 2001 From: sbearrows Date: Tue, 8 Jun 2021 13:30:18 -0600 Subject: [PATCH 1/5] first pass at fix for issue 127 --- R/register.R | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/R/register.R b/R/register.R index 5c98f37c..8b00905f 100644 --- a/R/register.R +++ b/R/register.R @@ -136,6 +136,11 @@ get_registered_functions <- function(decorations, tag, quiet = FALSE) { } out <- decorations[decorations$decoration == tag, ] + + if (NROW(out) != NROW(decorations)) { + stop("Incorrect cpp11 decorator") + } + out$functions <- lapply(out$context, decor::parse_cpp_function, is_attribute = TRUE) out <- vctrs::vec_cbind(out, vctrs::vec_rbind(!!!out$functions)) From 6e7fb50aaa012f6cafe7de66ef741cc05cbb79f8 Mon Sep 17 00:00:00 2001 From: sbearrows Date: Wed, 9 Jun 2021 11:47:06 -0600 Subject: [PATCH 2/5] adding error message for decorators --- R/register.R | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/R/register.R b/R/register.R index 8b00905f..6e9b87ec 100644 --- a/R/register.R +++ b/R/register.R @@ -137,8 +137,17 @@ get_registered_functions <- function(decorations, tag, quiet = FALSE) { out <- decorations[decorations$decoration == tag, ] - if (NROW(out) != NROW(decorations)) { - stop("Incorrect cpp11 decorator") + if(any(!decorations$decoration %in% c("cpp11::register", "cpp11::init"))) { + errors <- decorations[which(!decorations$decoration %in% c("cpp11::register", "cpp11::init")),] + lines <- errors$line + decors <- errors$decoration + if(length(lines) > 1) { + stop(call. = FALSE, paste0("Can't capture cpp11 decorators on lines ", + paste(lines, sep = " ", collapse = ", "), ".")) + } + stop(call. = FALSE, paste0("Can't capture cpp11 decorator on line ", + lines)) + } out$functions <- lapply(out$context, decor::parse_cpp_function, is_attribute = TRUE) From 81133bc3d1b37e3b7911e687ddcb127a110e506f Mon Sep 17 00:00:00 2001 From: sbearrows Date: Wed, 9 Jun 2021 11:51:26 -0600 Subject: [PATCH 3/5] removed extraneous code --- R/register.R | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/R/register.R b/R/register.R index 6e9b87ec..72e57ba0 100644 --- a/R/register.R +++ b/R/register.R @@ -138,9 +138,7 @@ get_registered_functions <- function(decorations, tag, quiet = FALSE) { out <- decorations[decorations$decoration == tag, ] if(any(!decorations$decoration %in% c("cpp11::register", "cpp11::init"))) { - errors <- decorations[which(!decorations$decoration %in% c("cpp11::register", "cpp11::init")),] - lines <- errors$line - decors <- errors$decoration + lines <- decorations$line[which(!decorations$decoration %in% c("cpp11::register", "cpp11::init"))] if(length(lines) > 1) { stop(call. = FALSE, paste0("Can't capture cpp11 decorators on lines ", paste(lines, sep = " ", collapse = ", "), ".")) From 984545b33e2b945dffeee6cd8aee83ff9efd672b Mon Sep 17 00:00:00 2001 From: sbearrows Date: Thu, 10 Jun 2021 11:40:29 -0600 Subject: [PATCH 4/5] added error message fx for incorrect decorators --- R/register.R | 33 ++++++++++------- R/source.R | 3 ++ tests/testthat/multiple_incorrect.cpp | 5 +++ tests/testthat/single_incorrect.cpp | 1 + tests/testthat/test-register.R | 33 +++++++++++++++++ tests/testthat/test-source.R | 52 +++++++++++++++++++++++++++ 6 files changed, 115 insertions(+), 12 deletions(-) create mode 100644 tests/testthat/multiple_incorrect.cpp create mode 100644 tests/testthat/single_incorrect.cpp diff --git a/R/register.R b/R/register.R index 72e57ba0..f0056af9 100644 --- a/R/register.R +++ b/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")) @@ -136,18 +138,6 @@ get_registered_functions <- function(decorations, tag, quiet = FALSE) { } out <- decorations[decorations$decoration == tag, ] - - if(any(!decorations$decoration %in% c("cpp11::register", "cpp11::init"))) { - lines <- decorations$line[which(!decorations$decoration %in% c("cpp11::register", "cpp11::init"))] - if(length(lines) > 1) { - stop(call. = FALSE, paste0("Can't capture cpp11 decorators on lines ", - paste(lines, sep = " ", collapse = ", "), ".")) - } - stop(call. = FALSE, paste0("Can't capture cpp11 decorator on line ", - lines)) - - } - out$functions <- lapply(out$context, decor::parse_cpp_function, is_attribute = TRUE) out <- vctrs::vec_cbind(out, vctrs::vec_rbind(!!!out$functions)) @@ -286,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) + + } +} diff --git a/R/source.R b/R/source.R index 57c56f01..fcf724fb 100644 --- a/R/source.R +++ b/R/source.R @@ -93,6 +93,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") ) diff --git a/tests/testthat/multiple_incorrect.cpp b/tests/testthat/multiple_incorrect.cpp new file mode 100644 index 00000000..caa100d9 --- /dev/null +++ b/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; } diff --git a/tests/testthat/single_incorrect.cpp b/tests/testthat/single_incorrect.cpp new file mode 100644 index 00000000..7867d53b --- /dev/null +++ b/tests/testthat/single_incorrect.cpp @@ -0,0 +1 @@ +[[cpp11:register]] int foo() { return 1; } diff --git a/tests/testthat/test-register.R b/tests/testthat/test-register.R index 09cdcb65..582d346a 100644 --- a/tests/testthat/test-register.R +++ b/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)) +}) diff --git a/tests/testthat/test-source.R b/tests/testthat/test-source.R index 51c2d6b3..bb9b3013 100644 --- a/tests/testthat/test-source.R +++ b/tests/testthat/test-source.R @@ -101,3 +101,55 @@ test_that("generate_include_paths handles paths with spaces", { expect_equal(generate_include_paths("cpp11"), "-I'/a path with spaces/cpp11'") } }) + +test_that("check_valid_attributes does not return an error if all registers are correct", { + expect_error_free( + cpp11::cpp_source(code = '#include + 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 + 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 + 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; + }')) +}) From ade29ef824951453c383099297ead7561db0b4e6 Mon Sep 17 00:00:00 2001 From: sbearrows Date: Thu, 10 Jun 2021 14:51:00 -0600 Subject: [PATCH 5/5] adding updates on issue #127 to NEWS --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index f6d59df7..d61d6f80 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,6 +4,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.