Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw an error for @importFrom unknown export #1458

Merged
merged 11 commits into from Nov 10, 2023
2 changes: 2 additions & 0 deletions NEWS.md
@@ -1,5 +1,7 @@
# roxygen2 (development version)

* A friendlier error is thrown when attempting to import non-existing
functions with `@importFrom` (#1409, @MichaelChirico).
* authors in `DESCRIPTION` can now have multiple email addresses (@jmbarbone, #1487).

* The `ROXYGEN_PKG` environment variable is now set up while roxygen
Expand Down
9 changes: 9 additions & 0 deletions R/namespace.R
Expand Up @@ -238,6 +238,15 @@ roxy_tag_parse.roxy_tag_importFrom <- function(x) {
}
#' @export
roxy_tag_ns.roxy_tag_importFrom <- function(x, block, env, import_only = FALSE) {
pkg <- x$val[1L]
if (requireNamespace(pkg, quietly = TRUE)) {
importing <- x$val[-1L]
unknown_idx <- !importing %in% getNamespaceExports(pkg)
Copy link
Contributor Author

@MichaelChirico MichaelChirico Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two issues with this PR in retrospect as surfaced by #1570:

  1. importing can be like "%>%" or `%>%` or '%>%' but getNamespaceExports() will always just be the plain symbol (%>%)
  2. importing[!unknown_idx] might drop everything --> output becomes importFrom($PKG,)

How would you recommend approaching these?

  1. Is a bit tricky because we have to normalize the quotes, I guess we can do gsub("^(`|'|\")(.*)\\1$", "\\1", importing). This could be a helper alongside has_quotes() Initially, I was worried about collision where you try and import a really weirdly-named object like as.name("'%>%'"), but I guess {roxygen2} has always made that difficult. And a quick search doesn't turn up any usage of this anyway: https://github.com/search?q=lang%3AR+%2F%5Cn%5Cs*%28%22%27.*%27%22%7C%27%22.*%22%27%7C%60%22.*%22%60%7C%60%27.*%27%60%7C%22%60.*%60%22%7C%27%60.*%60%27%29%5Cs*%3C-%2F+-path%3A.Rd&type=code
  2. Should this method just return(NULL) if all(unknown_idx)?

Copy link
Member

@hadley hadley Jan 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yeah, I think strip_quotes() seems reasonable.
  2. Yes, I think so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! that's basically how I did #1574 so let's continue there.

if (any(unknown_idx)) {
warn_roxy_tag(x, "Excluding unknown {cli::qty(sum(unknown_idx))} export{?s} in from {.package {pkg}}: {.code {importing[unknown_idx]}}")
x$val <- c(pkg, importing[!unknown_idx])
}
}
repeat_first_ignore_current("importFrom", x$val)
}

Expand Down
35 changes: 35 additions & 0 deletions tests/testthat/test-namespace.R
Expand Up @@ -378,3 +378,38 @@ test_that("can extract non-imports from namespace preserving source", {
path <- withr::local_tempfile(lines = lines)
expect_equal(namespace_exports(path), lines[c(1:3, 5)])
})

test_that("Invalid imports throw a helpful error", {
expect_warning(
expect_equal(
roc_proc_text(namespace_roclet(), "
#' @importFrom utils head InvalidUtilsFunction
NULL
"),
"importFrom(utils,head)"
),
"Excluding unknown export",
fixed = TRUE
)

# pluralization
expect_warning(
expect_equal(
roc_proc_text(namespace_roclet(), "
#' @importFrom utils head InvalidUtilsFunction1 InvalidUtilsFunction2
NULL
"),
"importFrom(utils,head)"
),
"Excluding unknown exports"
)

# If the package is not available at roxygenize() run time, nothing we can do
expect_equal(
roc_proc_text(namespace_roclet(), "
#' @importFrom AnUnknownUnavailablePackage Unchecked
NULL
"),
"importFrom(AnUnknownUnavailablePackage,Unchecked)"
)
})