Use original path names in compiler outputs (#198)

NEWS.md

@@ -2,6 +2,8 @@
 * added `as_double()` and `as_integer()` method to coerce integers to doubles and doubles to integers to doubles (@sbearrows, #46)
 * Removed redundant `.Call calls` in cpp11.cpp file (@sbearrows, #170)
 * Allow cpp11 decorators of the form `cpp11::linking_to` (@sbearrows, #193)
+* Error messages now output original file name rather than the temporary file name (@sbearrows, #194)
+* Fixed bug when running `cpp_source()` on the same file more than once (@sbearrows, #202)
 * Removed internal instances of `cpp11::stop()` and replaced with C++ exceptions (@sbearrows, #203)
 
 # cpp11 0.3.1

R/register.R

@@ -297,14 +297,13 @@ get_cpp_register_needs <- function() {
   strsplit(res, "[[:space:]]*,[[:space:]]*")[[1]]
 }
 
-check_valid_attributes <- function(decorations) {
+check_valid_attributes <- function(decorations, file = decorations$file) {
 
   bad_decor <- startsWith(decorations$decoration, "cpp11::") &
     (!decorations$decoration %in% c("cpp11::register", "cpp11::init", "cpp11::linking_to"))
 
   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")

R/source.R

@@ -84,36 +84,34 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu
     stop("`file` must have a `.cpp` or `.cc` extension")
   }
 
-  base_cpp <- (basename(file) %in% "cpp11.cpp")
+  name <- generate_cpp_name(file)
+  package <- tools::file_path_sans_ext(name)
 
-  if (base_cpp) {
-    name <- set_cpp_name(file)
-    package <- tools::file_path_sans_ext(name)
-  }
-  else {
-    # name and package might be different if cpp_source was called multiple times
-    name <- basename(file)
-    package <- tools::file_path_sans_ext(generate_cpp_name(file))
-  }
+  orig_dir <- normalizePath(dirname(file), winslash = "/")
+  new_dir <- normalizePath(file.path(dir, "src"), winslash = "/")
+
+  # file now points to another location
+  file.copy(file, file.path(new_dir, name))
 
-  # file not points to another location
-  file.copy(file, file.path(dir, "src", name))
   #change variable name to reflect this
-  file <- file.path(dir, "src", name)
+  new_file_path <- file.path(new_dir, name)
+  new_file_name <- basename(new_file_path)
 
+  orig_file_path <- file.path(orig_dir, new_file_name)
 
   suppressWarnings(
     all_decorations <- decor::cpp_decorations(dir, is_attribute = TRUE)
   )
 
-  check_valid_attributes(all_decorations)
+  #provide original path for error messages
+  check_valid_attributes(all_decorations, file = orig_file_path)
 
   cli_suppress(
     funs <- get_registered_functions(all_decorations, "cpp11::register")
   )
   cpp_functions_definitions <- generate_cpp_functions(funs, package = package)
 
-  cpp_path <- file.path(dirname(file), "cpp11.cpp")
+  cpp_path <- file.path(dirname(new_file_path), "cpp11.cpp")
   brio::write_lines(c('#include "cpp11/declarations.hpp"', "using namespace cpp11;", cpp_functions_definitions), cpp_path)
 
   linking_to <- union(get_linking_to(all_decorations), "cpp11")
@@ -128,17 +126,20 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu
 
   makevars_content <- generate_makevars(includes, cxx_std)
 
-  brio::write_lines(makevars_content, file.path(dir, "src", "Makevars"))
+  brio::write_lines(makevars_content, file.path(new_dir, "Makevars"))
 
-  source_files <- normalizePath(c(file, cpp_path), winslash = "/")
-  res <- callr::rcmd("SHLIB", source_files, user_profile = TRUE, show = !quiet, wd = file.path(dir, "src"))
+  source_files <- normalizePath(c(new_file_path, cpp_path), winslash = "/")
+  res <- callr::rcmd("SHLIB", source_files, user_profile = TRUE, show = !quiet, wd = new_dir)
   if (res$status != 0) {
-    cat(res$stderr)
+    error_messages <- res$stderr
+
+    # Substitute temporary file path with original file path
+    error_messages <- gsub(tools::file_path_sans_ext(new_file_path), tools::file_path_sans_ext(orig_file_path), error_messages, fixed = TRUE)
+    cat(error_messages)
     stop("Compilation failed.", call. = FALSE)
   }
 
-
-  shared_lib <- file.path(dir, "src", paste0(tools::file_path_sans_ext(basename(file)), .Platform$dynlib.ext))
+  shared_lib <- file.path(dir, "src", paste0(tools::file_path_sans_ext(new_file_name), .Platform$dynlib.ext))
   r_path <- file.path(dir, "R", "cpp11.R")
   brio::write_lines(r_functions, r_path)
   source(r_path, local = env)
@@ -149,14 +150,6 @@ cpp_source <- function(file, code = NULL, env = parent.frame(), clean = TRUE, qu
 the <- new.env(parent = emptyenv())
 the$count <- 0L
 
-set_cpp_name <- function(name) {
-  ext <- tools::file_ext(name)
-  root <- tools::file_path_sans_ext(basename(name))
-  count <- 2
-  new_name <- sprintf("%s_%i", root, count)
-  sprintf("%s.%s", new_name, ext)
-}
-
 generate_cpp_name <- function(name, loaded_dlls = c("cpp11", names(getLoadedDLLs()))) {
   ext <- tools::file_ext(name)
   root <- tools::file_path_sans_ext(basename(name))

tests/testthat/single_error.cpp

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

tests/testthat/test-source.R

@@ -54,6 +54,17 @@ test_that("cpp_source works with files called `cpp11.cpp`", {
   expect_true(always_true())
 })
 
+test_that("cpp_source returns original file name on error", {
+
+  expect_output(try(cpp_source(test_path("single_error.cpp"), clean = TRUE), silent = TRUE),
+               normalizePath(test_path("single_error.cpp"), winslash = "/"), fixed = TRUE)
+
+  #error generated for incorrect attributes is separate from compilation errors
+  expect_error(cpp_source(test_path("single_incorrect.cpp"), clean = TRUE),
+                normalizePath(test_path("single_incorrect.cpp"), winslash = "/"), fixed = TRUE)
+
+})
+
 test_that("cpp_source lets you set the C++ standard", {
   skip_on_os("solaris")
   skip_on_os("windows") # Older windows toolchains do not support C++14
@@ -104,7 +115,7 @@ 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>
+    cpp11::cpp_source(clean = TRUE, code = '#include <cpp11.hpp>
   using namespace cpp11::literals;
   [[cpp11::register]]
   cpp11::list fn() {
@@ -119,7 +130,7 @@ test_that("check_valid_attributes does not return an error if all registers are
     return x;
   }'))
   expect_error_free(
-    cpp11::cpp_source(
+    cpp11::cpp_source(clean = TRUE,
       code = '#include <cpp11/R.hpp>
               #include <RProgress.h>
 
@@ -181,6 +192,8 @@ test_that("check_valid_attributes returns an error if one or more registers is i
     return x;
   }'))
 
+
+
   expect_error(
     cpp11::cpp_source(
       code = '
@@ -195,6 +208,12 @@ test_that("check_valid_attributes returns an error if one or more registers is i
           pb.tick();
         }
       }
-')
-  )
+'))
+})
+
+test_that("cpp_source(d) functions work after sourcing file more than once", {
+  cpp11::cpp_source(test_path("single.cpp"), clean = TRUE)
+  expect_equal(foo(), 1)
+  cpp11::cpp_source(test_path("single.cpp"), clean = TRUE)
+  expect_equal(foo(), 1)
 })