Skip to content

Expand @import directives to explicit importFrom() - #1898

Open
lionel- wants to merge 4 commits into
mainfrom
expand-import
Open

Expand @import directives to explicit importFrom()#1898
lionel- wants to merge 4 commits into
mainfrom
expand-import

Conversation

@lionel-

@lionel- lionel- commented Jun 30, 2026

Copy link
Copy Markdown
Member

Branched from #1892

This PR expands @import pkg directives to explicit imports in an importFrom(pkg, sym1, sym2, ...) expression. This pins the set of imported symbols between rounds of document(), and prevents users from getting issues at load time when a package update creates a conflict. Instead, new conflicts are deferred until the next time the maintainer documents, where the conflict causes an error that forces the maintainer to resolve it with an except = clause.

Ideally we'd detect symbols actually used by the package with a mechanism like findGlobals(). Until we can do it reliably, possibly with the help of Oak and annotations, I've opted to just import every exports.

@hadley

hadley commented Jun 30, 2026

Copy link
Copy Markdown
Member

What if we introduced a new tag for this? Like @importAll or maybe @importPattern ?

@lionel-

lionel- commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

That would be reasonable, especially as a first step to gain practical experience with the approach, but it would not feel like we're creating a pit of success for our users to fall into :)
I.e. the safe option would not be the default one.

@hadley

hadley commented Jul 1, 2026

Copy link
Copy Markdown
Member

Right, I think we'd still want to change long term, but I worry that it's too big of a change to push through now (even in a minor release). If we used a new tag, I'm happy to merge this and encourage people to use it. (It would also be an interesting place to try and claude code automated patching system)

@lionel-
lionel- changed the base branch from merge-import-from to main July 3, 2026 07:25
@lionel-
lionel- force-pushed the expand-import branch 2 times, most recently from 40c0cc5 to 3aa4100 Compare July 3, 2026 07:58
@lionel-

lionel- commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

@hadley I moved the expansion to a new @importAllFrom tag.

Unlike @import where you can pass arbitrary arguments that get included verbatim in the import() call (e.g. except = c(foo, bar)), we need to actually parse exclusions since we're applying them.

I went for an exclusion syntax that is consistent with the existing @inheritDotParams foo -x -y.

#' @importAllFrom dplyr -filter -lag
#' @importAllFrom rlang -":="

We need the imported package to be installed to figure out the exports. Other features in roxygen2 like inherits degrade to a no-op or fallback when a required package is not installed. Here I preferred to throw a hard error. We also warn when an excluded symbol is not exported.

@lionel-
lionel- requested a review from hadley July 17, 2026 08:14
Comment thread R/namespace.R Outdated
Comment thread R/namespace.R Outdated
Comment thread R/namespace.R Outdated
Comment thread R/namespace.R
Comment thread R/namespace.R Outdated
# Splits an `@importAllFrom` value into the package to expand and the symbols to
# leave out. An exclusion is a word with a `-` prefix, e.g. `-abort`. Exclusions
# may be quoted: `-"-.Date"`, ``-`-.Date` ``, or `-'-.Date'`.
parse_import_all_from <- function(vals) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be better to use select_args_text?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

See last commit:

  • Now parses tags with tag_two_part()
  • Then parses arguments with select_args_text()
  • New class argument threaded through roxy_warn and used to handle failures

Comment thread tests/testthat/test-namespace.R Outdated
@lionel-
lionel- requested a review from hadley July 21, 2026 09:07
Comment thread R/namespace.R
#' #' @importAllFrom rlang
#'
#' # This results in the following lines:
#' # importFrom(rlang,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It occurs to me that the downside of this approach is that now if rlang removes a symbol, this package will fail to load. With @import (as long as you didn't actually use the symbol) that isn't a problem.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

oooh that's a very good point. This pretty much kills the approach.

The alternative is that once Oak symbol resolution is robust enough, we wrap it in an R package and then use that the way you'd use {globals} to figure out from usage the minimum set of symbols to import.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants