fix: isExported is always TRUE after devtools::load_all() (#347)

[bburns632]

Fixes #347.

The bug

FunctionReporter determined which functions were exported by force-attaching the package with require() and then listing the attached environment:

suppressPackageStartupMessages({
    require(self$pkg_name, lib.loc = .libPaths()[1], character.only = TRUE)
})
exported_obj_names <- ls(sprintf("package:%s", self$pkg_name))

devtools::load_all() copies the entire namespace into that attached environment (pkgload:::populate_pkg_env, export_all = TRUE by default), so after load_all() every function looked exported.

The fix

Read the namespace's own export metadata via getNamespaceExports(), using the namespace extract_nodes already loads through private$get_pkg_env(). Thanks @jayqi for pointing at the right function in the issue thread.

Two knock-on effects worth calling out:

1. pkgnet no longer attaches the package it is analyzing as a side effect of building the node table.
2. The Windows covr workaround in calculate_test_coverage() unconditionally detached package:<pkg>, which only worked because require() had attached it. It is now guarded on the package actually being on the search path, and the re-attach is conditioned on the same flag.

Why the test changes are needed

The existing isExported fixtures could not have caught this bug. The only unexported function across all the fake test packages carrying an isExported fixture is milne's .classname — and ls() hides dot-prefixed names by default, so it read FALSE both before and after the fix. Every other fixture function is exported, so TRUE was correct either way. The fixtures were vacuous for this code path.

A real regression test needs two things the suite did not have:

• A package with a non-dot unexported function. inst/silverstein fits: its NAMESPACE exports only Carrots, leaving couplet_1 and couplet_2 unexported. It had no isExported fixture, so nothing needed regenerating.
• An actual load_all(). The bug only manifests through what load_all() does to the attached environment, so it cannot be simulated without invoking it. Hence pkgload in Suggests, guarded with skip_if_not_installed("pkgload") so it degrades gracefully.

The test asserts the precondition (couplet_1 is in the attached env after load_all()) before asserting the fix, so it fails loudly rather than silently passing if load_all()'s behavior ever changes.

One implementation detail: the test calls unloadNamespace("silverstein") first. An earlier test in the same file loads silverstein, and load_all() over an already-loaded namespace takes a ::-patching path that errors on pkgload 1.3.4 against current rlang (env_unlock() is defunct as of rlang 1.1.5). Starting from an unloaded namespace avoids that path entirely, so the test works on both old and new pkgload instead of needing a version floor in Suggests.

Verification

Against inst/silverstein after pkgload::load_all():

node	before	after
couplet_1	TRUE (wrong)	FALSE
couplet_2	TRUE (wrong)	FALSE
Carrots$public_methods$initialize	TRUE	TRUE

The new test was confirmed non-vacuous: against unmodified R/FunctionReporter.R it fails on exactly the two isExported assertions with actual: TRUE.

Full local devtools::test(): 252 pass, 2 fail. Both failures are covr::package_coverage() errors on baseballstats (test-FunctionReporter-class.R:91, test-plotting.R:12). They are pre-existing and unrelated — I checked out main and ran the full suite there, getting the identical two failures, and CI is green on main at 2cd9ea8 (the commit this branches from). Local environment only; deferring to CI.

🤖 Generated with Claude Code