From d5e8e1d44d54a35b74f93e24e0db42faa2c4c047 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 3 Jul 2026 12:32:26 +0000 Subject: [PATCH] Fix S3 dispatch, multi-package loading, and export processing load_fast() replicated R's namespace machinery but omitted several pieces that real packages and multi-package sessions depend on: - S3 methods declared via S3method() were never registered, so the namespace's .__S3MethodsTable__. stayed empty and S3 dispatch fell through to *.default / the base method (even for print/format). Now calls registerS3methods() like base::loadNamespace(), and rebuilds the table on incremental reloads so re-sourced methods take effect. - The namespace info was missing the `lazydata` field, so `pkg::name` for any name not in exports threw "object 'lazydata' not found" (the documented multi-package bug). Now seeds `lazydata` and `nativeRoutines` at namespace creation, matching makeNamespace(). - Only export() was processed. exportPattern(), exportClasses(), and exportMethods() were ignored, so exportPattern packages exported nothing (pkg::fn failed) and cross-package importClassesFrom() / importMethodsFrom() errored. Now computes exports the way loadNamespace() does, including the internal-name stoplist. - .onAttach was never run. Now runs on full load after attach. Adds an S3 fixture to devpackage and test stages covering S3 dispatch (single- and cross-package), incremental S3 refresh, exportPattern / exportClasses / exportMethods, `::` resolution, and .onAttach. Suite is 259 passing. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_0133UhzJyq9xA2BBSA6B1LTd --- AGENTS.md | 23 +- R/loadfast.R | 165 ++++++++++++- README.md | 22 +- TECHNICAL_DEBT.md | 25 +- devpackage/NAMESPACE | 7 + devpackage/R/s3_classes.R | 41 ++++ devpackage/tests/testthat/test-base.R | 23 ++ test_loadfast.R | 318 +++++++++++++++++++++++++- 8 files changed, 601 insertions(+), 23 deletions(-) create mode 100644 devpackage/R/s3_classes.R diff --git a/AGENTS.md b/AGENTS.md index cec1c0d..db9ed93 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,10 +17,12 @@ This `AGENT.md` file is read by every agent session. !!!Keep them high-signal!!! - **`test_loadfast.R`** is the single custom test runner. It sources `R/loadfast.R` directly and remains the single command users run. - **`devpackage/`** is the single frozen baseline package snapshot used by the tests. Contains `DESCRIPTION`, `NAMESPACE`, `R/` (source files), and `tests/testthat/` (testthat tests + helpers). Package name is `devpackage`. All code mutations for reload/incremental testing are applied ad-hoc to temp copies at test time. - `R/base.R` — plain functions (`add`, `scale_vector`, `summarize_values`, `mutate_dt`) — `mutate_dt` exercises `data.table`'s `:=` and `as.data.table` via `importFrom` + - `R/s3_classes.R` — S3 constructor (`new_temperature`), a package-defined generic (`describe_s3`), and methods on both base generics (`print`/`format`/`as.character`) and the local generic. Registered via `S3method()` in NAMESPACE, none exported by name — so dispatch depends on the loader populating the S3 methods table. - `R/s4_classes.R` — S4 classes (`Animal`, `Pet`), generics, methods - `R/r6_classes.R` — R6 classes (`Logger`, `Counter`) - `tests/testthat/helper-utils.R` — testthat helper factories (`make_test_animal`, `make_test_logger`) - `tests/testthat/test-base.R` — testthat tests exercising all of the above + - NAMESPACE exercises `export()`, `importFrom()`, `import(methods)`, and `S3method()`. `test_loadfast.R` also builds ad-hoc packages that use `exportPattern()`, `exportClasses()`, and `exportMethods()`. - **`renv/`** and **`renv.lock`** manage the project-local library. Key packages: `testthat`, `R6`, `rlang`, `data.table`. - **`TECHNICAL_DEBT.md`** tracks known loader tradeoffs, risks, and cleanup opportunities. Update it when you identify a non-trivial issue that is worth preserving across sessions. Use reload-registration terminology in docs and guidance rather than cache-invalidation terminology when describing the user-facing behavior. - **`pkgload/`** and **`devtools`** contains the original pkgload & devtools R package source code (moved here for reference). It is NOT used at runtime (and gitignored). Can be initally cloned with `just setup`. @@ -53,15 +55,16 @@ This `AGENT.md` file is read by every agent session. !!!Keep them high-signal!!! - `testthat::test_dir()` runs only once (Stage 1, against the frozen `devpackage/` snapshot). Subsequent stages verify behavior via `check()` assertions, which already cover everything the testthat suite tests. - **`on.exit()` cannot be used at the top level of `test_loadfast.R`** for suite-wide cleanup because the file is executed as a script and the suite manages temp directories explicitly through `.tmp_dirs`. - **Test stages** (all in `test_loadfast.R`): - - **Stage 1**: Load frozen `devpackage/` from its original location. Full checks (namespace, imports, S4, R6, data.table, testthat helpers) + `test_dir()` run. - - **Stage 2**: Copy `devpackage/` to a temp dir, apply mutations via `writeLines()` (changed function behavior, new S4 slots, new R6 methods, updated testthat helpers), load, verify all changed behavior. The mutations change behavior across all three class systems (plain functions, S4, R6) so the reload path is exercised for each. + - **Stage 1**: Load frozen `devpackage/` from its original location. Full checks (namespace, imports, S3, S4, R6, data.table, testthat helpers) + `test_dir()` run. + - **Stage 2**: Copy `devpackage/` to a temp dir, apply mutations via `writeLines()` (changed function behavior, new S4 slots, new R6 methods, updated testthat helpers), load, verify all changed behavior. The mutations change behavior across the class systems (plain functions, S4, R6) so the reload path is exercised for each. - **Stage 3**: Copy `devpackage/` to a second temp dir, test cross-file dependencies: - 3a: `compute()` in `wrappers.R` calls `add()` from `base.R` — change only `base.R`, verify `compute()` output changes. - 3b: `describe_loud()` in `wrappers.R` calls `describe()` generic from `s4_classes.R` — change only the method, verify `describe_loud()` output changes. Also tests `callNextMethod` chain for `Pet`. - 3c: `make_animal()` in `wrappers.R` calls `new("Animal",...)` — add `age` slot to the class in `s4_classes.R`, verify the unchanged constructor returns an object with the new slot. - 3d: add a `Collate` field plus deliberately misordered filenames, then verify the loader respects `Collate` ordering rather than plain alphabetical order. - - **Stage 4**: Copy `devpackage/` to a third temp dir, test incremental-specific behaviors: no-change short-circuit, function removal (stale symbols linger), function addition, function modification, new file, file deletion, explicit file reload registration, failed incremental reload recovery, runtime S4 method patch reload registration, and persistent `renv.lock` change warnings. Verifies that `full = TRUE` properly cleans up stale symbols. -- Stage 2 always triggers a full load (different path from Stage 1 = cache miss). Stage 3 exercises the incremental path for `load_fast()` on a stable temp path while mutating files in place. + - **Stage 4**: Copy `devpackage/` to a third temp dir, test incremental-specific behaviors: no-change short-circuit, function removal (stale symbols linger), function addition, function modification, new file, file deletion, explicit file reload registration, failed incremental reload recovery, runtime S4 method patch reload registration, persistent `renv.lock` change warnings, `.onLoad`/`.onAttach` hook execution (4l), and S3 method table refresh on incremental reload (4m). Verifies that `full = TRUE` properly cleans up stale symbols. + - **Stage 5**: Build ad-hoc packages (distinct class names, so nothing collides) to test cross-package fidelity: `exportPattern()` populating exports and `pkg::fn` resolving (5a), `pkg::missing` giving a proper not-exported error rather than the `lazydata` internal (5b), S3 dispatch across package boundaries for both base and package-defined generics (5c), and `exportClasses()`/`exportMethods()` + `importClassesFrom()`/`importMethodsFrom()` for S4 (5d). +- Stage 2 always triggers a full load (different path from Stage 1 = cache miss). Stage 3 exercises the incremental path for `load_fast()` on a stable temp path while mutating files in place. The multi-package stages (3e, 3h) call `strip_s3_fixture()` on their copies so the shared `temperature` S3 fixture from the baseline does not register colliding methods across renamed packages. ## R namespace machinery gotchas @@ -100,6 +103,18 @@ These base R functions place objects into the **parent** of the namespace env (t ### Imported symbols must be copied to the attached package env `namespaceImportFrom` places symbols in the imports env (parent of ns_env). Code *inside* the namespace finds them via the parent chain, but they are not in `ls(ns_env)`. To make them available for interactive use on the search path (matching `devtools::load_all()` behavior), the loaders also copy all symbols from the imports env into the `package:` env. +### S3 methods must be registered, not just sourced (`.loadfast.register_s3`) +Sourcing `print.foo` into `ns_env` is **not enough** for S3 dispatch, even though the loader also copies it onto the search path. `UseMethod()` resolves a method by searching the calling frame and then the `.__S3MethodsTable__.` of the generic's defining namespace — it does **not** walk the attached search path. So a method registered with `S3method()` but not exported by name (the roxygen default) never dispatches unless the table is populated. The loader calls base's (unexported) `registerS3methods(nsInfo$S3methods, pkg, ns_env)`, exactly like `loadNamespace()`: methods on **local** generics are copied into the package's own table; methods on **foreign** generics (e.g. base `print`) are registered into *that* package's table (or delayed via a load hook). On incremental reload the table is rebuilt (`reset = TRUE`) because re-sourcing replaces the underlying functions and `registerS3methods()` otherwise appends to the `"S3methods"` info matrix on every call. + +### Exports are more than `export()` (`.loadfast.compute_exports`) +`export()` alone misses the common cases. The loader mirrors `loadNamespace()`'s export computation: `exportPattern()` (expanded via `ls(ns_env, pattern=)`), and for S4 packages `exportClasses()`/`exportClassPattern()` (exported as `.__C__` metadata objects) and `exportMethods()` (the generic plus its `.__T__:` method table). `methods::cacheMetaData()` is called for S4 packages. Without this, `exportPattern` packages expose **nothing** (so `pkg::fn` fails) and `importClassesFrom()`/`importMethodsFrom()` from another package error with *"class/method not exported"*. + +### `.__NAMESPACE__.` must include `lazydata` (and `nativeRoutines`) +`makeNamespace()` seeds these fields at namespace creation; the inline replica must too. `pkg::name` goes through `getExportedValue()` → `.Internal(getNamespaceValue())`, which falls back to `getNamespaceInfo(ns, "lazydata")` for any name not in `exports`. If the field is missing, that read throws the infamous *"object 'lazydata' not found"* instead of a proper *"not an exported object"* error. `lazydata` is an env named `lazydata:`; `nativeRoutines` is `list()`. `DLLs` is intentionally omitted (pure-R packages don't have it). + +### `.onAttach` is an attach-time hook +`.onLoad` runs on every (full and incremental) load; `.onAttach` runs only on the full load, right after the package env is attached to the search path — matching `library()`/`load_all()`, since incremental reloads never re-attach. Both go through `.loadfast.run_hook(hook, ...)`. + ## System-specific notes - **OS**: Windows diff --git a/R/loadfast.R b/R/loadfast.R index f7f2854..188d5ad 100644 --- a/R/loadfast.R +++ b/R/loadfast.R @@ -134,6 +134,7 @@ load_fast <- function(path = ".", helpers = TRUE, attach_testthat = NULL, full = old_lock_hash <- if (is.null(cached$lock_hash)) NA_character_ else cached$lock_hash registered_reload_files <- if (is.null(cached$registered_reload_files)) character(0) else cached$registered_reload_files pending_reload_message <- if (is.null(cached$pending_reload_message)) NULL else cached$pending_reload_message + s3_methods_matrix <- if (is.null(cached$s3_methods)) matrix(NA_character_, 0L, 4L) else cached$s3_methods if (!identical(current_lock_hash, old_lock_hash)) { warning( @@ -168,6 +169,7 @@ load_fast <- function(path = ".", helpers = TRUE, attach_testthat = NULL, full = pkg_name = pkg_name, hashes = current_hashes, lock_hash = old_lock_hash, + s3_methods = s3_methods_matrix, registered_reload_files = character(0), pending_reload_message = NULL ) @@ -186,6 +188,12 @@ load_fast <- function(path = ".", helpers = TRUE, attach_testthat = NULL, full = } .timer(paste0("incr source ", length(files_to_source), " files")) + # Re-sourcing a file replaces its function objects in `ns_env`, but the S3 + # methods table holds separate (eagerly copied) references for methods on + # local generics, so it must be rebuilt to point at the fresh definitions. + .loadfast.register_s3(s3_methods_matrix, pkg_name, ns_env, reset = TRUE) + .timer("incr S3 re-registration") + list2env(as.list(ns_env, all.names = FALSE), envir = pkg_env) list2env(as.list(parent.env(ns_env), all.names = TRUE), envir = pkg_env) .timer("incr pkg_env sync") @@ -195,6 +203,7 @@ load_fast <- function(path = ".", helpers = TRUE, attach_testthat = NULL, full = pkg_name = pkg_name, hashes = current_hashes, lock_hash = old_lock_hash, + s3_methods = s3_methods_matrix, registered_reload_files = character(0), pending_reload_message = NULL ) @@ -227,7 +236,7 @@ load_fast <- function(path = ".", helpers = TRUE, attach_testthat = NULL, full = "]" ) - .loadfast.run_onload(ns_env, abs_path, pkg_name) + .loadfast.run_hook(".onLoad", ns_env, abs_path, pkg_name) .timer("incr .onLoad") .loadfast.source_helpers(abs_path, pkg_env, helpers, attach_testthat, pkg_name) @@ -260,9 +269,17 @@ load_fast <- function(path = ".", helpers = TRUE, attach_testthat = NULL, full = ns_env[[".__NAMESPACE__."]] <- info info[["spec"]] <- c(name = pkg_name, version = "0.0.0") setNamespaceInfo(ns_env, "exports", new.env(hash = TRUE, parent = baseenv())) + # `lazydata` must exist even for packages that ship no data: base's + # `getExportedValue()` (used by `pkg::name`) falls through to it for any name + # not in `exports`, and `getNamespaceInfo(ns, "lazydata")` errors with + # "object 'lazydata' not found" if the field is missing. See makeNamespace(). + lazydata_env <- new.env(parent = baseenv(), hash = TRUE) + attr(lazydata_env, "name") <- paste0("lazydata:", pkg_name) + setNamespaceInfo(ns_env, "lazydata", lazydata_env) setNamespaceInfo(ns_env, "imports", list(base = TRUE)) setNamespaceInfo(ns_env, "path", abs_path) setNamespaceInfo(ns_env, "dynlibs", NULL) + setNamespaceInfo(ns_env, "nativeRoutines", list()) setNamespaceInfo(ns_env, "S3methods", matrix(NA_character_, 0L, 4L)) ns_env[[".__S3MethodsTable__."]] <- new.env(hash = TRUE, parent = baseenv()) ns_env[[".__DEVTOOLS__"]] <- new.env(parent = ns_env) @@ -382,12 +399,16 @@ load_fast <- function(path = ".", helpers = TRUE, attach_testthat = NULL, full = } .timer(paste0("source ", length(r_files), " files")) + s3_methods_matrix <- matrix(NA_character_, 0L, 4L) if (file.exists(ns_file)) { - exports <- nsInfo$exports + exports <- .loadfast.compute_exports(ns_env, nsInfo, pkg_name) if (length(exports) > 0L) { namespaceExport(ns_env, exports) } + s3_methods_matrix <- nsInfo$S3methods + .loadfast.register_s3(s3_methods_matrix, pkg_name, ns_env) } + .timer("exports + S3 registration") uses_testthat <- local({ test_dirs <- c( @@ -402,7 +423,7 @@ load_fast <- function(path = ".", helpers = TRUE, attach_testthat = NULL, full = } .timer("attach testthat") - .loadfast.run_onload(ns_env, abs_path, pkg_name) + .loadfast.run_hook(".onLoad", ns_env, abs_path, pkg_name) .timer(".onLoad") pkg_env <- attach(NULL, name = pkg_env_name) @@ -410,6 +431,12 @@ load_fast <- function(path = ".", helpers = TRUE, attach_testthat = NULL, full = list2env(as.list(impenv, all.names = TRUE), envir = pkg_env) .timer("attach pkg to search path") + # `.onAttach` is an attach-time hook (search-path attachment happens only on a + # full load), so it runs here rather than in the incremental path, mirroring + # library()/load_all(). `.onLoad` above covers the load-time hook. + .loadfast.run_hook(".onAttach", ns_env, abs_path, pkg_name) + .timer(".onAttach") + if (isTRUE(helpers) && uses_testthat) { .loadfast.do_source_helpers(abs_path, pkg_env) } @@ -420,6 +447,7 @@ load_fast <- function(path = ".", helpers = TRUE, attach_testthat = NULL, full = pkg_name = pkg_name, hashes = current_hashes, lock_hash = current_lock_hash, + s3_methods = s3_methods_matrix, registered_reload_files = character(0), pending_reload_message = NULL ) @@ -503,12 +531,135 @@ load_fast_register_reload <- function(path = ".", files, reason = NULL) { invisible(TRUE) } -.loadfast.run_onload <- function(ns_env, abs_path, pkg_name) { - if (!exists(".onLoad", envir = ns_env, inherits = FALSE)) return(invisible(NULL)) +# Compute the full set of names to export, mirroring base::loadNamespace(): +# explicit export(), exportPattern() expansion, and (when the package defines S4 +# metadata) exportClasses()/exportClassPattern() and exportMethods(). S4 classes +# are exported as their `.__C__` metadata objects and S4 methods as the +# generic plus its `.__T__:` method table, which is what +# importClassesFrom()/importMethodsFrom() look for in another package. +.loadfast.compute_exports <- function(ns_env, nsInfo, pkg_name) { + exports <- nsInfo$exports + for (p in nsInfo$exportPatterns) { + exports <- c(ls(ns_env, pattern = p, all.names = TRUE), exports) + } + + has_s4 <- isNamespaceLoaded("methods") && + isTRUE(tryCatch(methods:::.hasS4MetaData(ns_env), error = function(e) FALSE)) + if (has_s4 && pkg_name != "methods") { + methods::cacheMetaData(ns_env, TRUE, ns_env) + + for (p in nsInfo$exportPatterns) { + expp <- ls(ns_env, pattern = p, all.names = TRUE) + newEx <- !(expp %in% exports) + if (any(newEx)) exports <- c(expp[newEx], exports) + } + + expClasses <- nsInfo$exportClasses + aClasses <- methods::getClasses(ns_env) + classPatterns <- nsInfo$exportClassPatterns + if (!length(classPatterns)) classPatterns <- nsInfo$exportPatterns + pClasses <- unique(unlist(lapply(classPatterns, grep, aClasses, value = TRUE))) + if (length(pClasses)) { + good <- vapply(pClasses, methods::isClass, NA, where = ns_env) + expClasses <- c(expClasses, pClasses[good]) + } + if (length(expClasses)) { + missing_classes <- !vapply(expClasses, methods::isClass, NA, where = ns_env) + if (any(missing_classes)) { + stop( + "in package ", pkg_name, " classes ", + paste(expClasses[missing_classes], collapse = ", "), + " were specified for export but not defined", + call. = FALSE + ) + } + expClasses <- paste0(methods::classMetaName(""), expClasses) + } + + allGenerics <- unique(c( + methods:::.getGenerics(ns_env), + methods:::.getGenerics(parent.env(ns_env)) + )) + expMethods <- nsInfo$exportMethods + addGenerics <- expMethods[is.na(match(expMethods, exports))] + if (length(addGenerics)) { + have <- vapply( + addGenerics, + function(w) exists(w, mode = "function", envir = ns_env), + NA, USE.NAMES = FALSE + ) + exports <- c(exports, addGenerics[have]) + } + expTables <- character() + if (length(allGenerics)) { + expMethods <- unique(c(expMethods, exports[!is.na(match(exports, allGenerics))])) + tPrefix <- methods:::.TableMetaPrefix() + allMethodTables <- unique(c( + methods:::.getGenerics(ns_env, tPrefix), + methods:::.getGenerics(parent.env(ns_env), tPrefix) + )) + needMethods <- (exports %in% allGenerics) & !(exports %in% expMethods) + if (any(needMethods)) expMethods <- c(expMethods, exports[needMethods]) + # Methods on primitive generics (e.g. `[`, `length`) are exportable even + # when not listed in exportMethods(), so their method tables can reach an + # importing package via importMethodsFrom(). Mirrors loadNamespace(). + pm <- allGenerics[!(allGenerics %in% expMethods)] + if (length(pm)) { + prim <- vapply(pm, function(pmi) { + f <- tryCatch(methods::getFunction(pmi, FALSE, FALSE, ns_env), error = function(e) NULL) + !is.null(f) && is.primitive(f) + }, logical(1L)) + expMethods <- c(expMethods, pm[prim]) + } + for (mi in expMethods) { + if (!(mi %in% exports) && exists(mi, envir = ns_env, mode = "function", inherits = FALSE)) { + exports <- c(exports, mi) + } + ii <- grep(paste0(tPrefix, mi, ":"), allMethodTables, fixed = TRUE) + if (length(ii)) expTables <- c(expTables, allMethodTables[ii[1L]]) + } + } + exports <- c(exports, expClasses, expTables) + } + + # Internal namespace objects must never be exported, even when an + # exportPattern() happens to match them. Mirrors base::loadNamespace()'s + # stoplist, plus loadfast's own `.__DEVTOOLS__` marker. + stoplist <- c( + ".__NAMESPACE__.", ".__S3MethodsTable__.", ".__DEVTOOLS__", ".packageName", + ".First.lib", ".onLoad", ".onAttach", ".conflicts.OK", ".noGenerics" + ) + exports <- exports[!exports %in% stoplist] + exports <- exports[!is.na(exports) & nzchar(exports)] + unique(exports) +} + +# Register S3 methods declared via S3method() in NAMESPACE, mirroring the +# registerS3methods() call inside base::loadNamespace(). Without this, S3 +# dispatch fails for any method not resolvable in the calling frame — notably +# methods on generics defined in other namespaces (e.g. base's `print`) and +# methods declared with S3method() but not exported by name. +.loadfast.register_s3 <- function(s3_methods, pkg_name, ns_env, reset = FALSE) { + # registerS3methods() appends its input to the namespace's "S3methods" info + # matrix each call. On incremental reloads we re-register to refresh the table + # (re-sourced files replace the underlying functions), so reset the matrix + # first to avoid unbounded growth and keep it in sync with what is declared. + if (isTRUE(reset)) { + setNamespaceInfo(ns_env, "S3methods", matrix(NA_character_, 0L, 4L)) + } + if (is.null(s3_methods) || NROW(s3_methods) == 0L) { + return(invisible(NULL)) + } + registerS3methods(s3_methods, pkg_name, ns_env) + invisible(NULL) +} + +.loadfast.run_hook <- function(hook, ns_env, abs_path, pkg_name) { + if (!exists(hook, envir = ns_env, inherits = FALSE)) return(invisible(NULL)) tryCatch( - get(".onLoad", envir = ns_env, inherits = FALSE)(dirname(abs_path), pkg_name), + get(hook, envir = ns_env, inherits = FALSE)(dirname(abs_path), pkg_name), error = function(e) { - warning("Error in .onLoad() for '", pkg_name, "': ", conditionMessage(e), call. = FALSE) + warning("Error in ", hook, "() for '", pkg_name, "': ", conditionMessage(e), call. = FALSE) } ) invisible(NULL) diff --git a/README.md b/README.md index 9aba9c7..c04ce1b 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,7 @@ loadfast::load_fast("path/to/your/package") `load_fast()` reads the package name from `DESCRIPTION`, builds a namespace, processes `NAMESPACE` imports, sources `R/` files, attaches the package to the search path, and optionally sources testthat helpers. -It supports packages that rely on standard R namespace behavior, including imports, S4 classes, and R6 classes. +It supports packages that rely on standard R namespace behavior, including imports, S3 methods (registered via `S3method()`), S4 classes, and R6 classes. Exports declared with `export()`, `exportPattern()`, `exportClasses()`, and `exportMethods()` are all honored, so a `load_fast()`-loaded package can be depended on by another package in the same session. ### Options @@ -63,15 +63,23 @@ After an incremental reload: Use `full = TRUE` after deleting files, removing functions, or whenever you need a clean namespace state. -## Known bug: `lazydata` can fail across multiple loaded packages +## Working across multiple packages -There is a known compatibility bug when multiple packages are loaded in the same R session with `loadfast` and one package calls code from another package. +`load_fast()` can load several interdependent packages in the same session: load +the dependency first, then the packages that import from it. Because each +namespace is built with the same metadata a real installed package has +(including the `lazydata` field, S3 method tables, and full export +processing), cross-package access works — whether through `importFrom()`, +`import()`, `pkg::name`, S3 dispatch, or `importClassesFrom()` / +`importMethodsFrom()`. -In particular, when package `A` is loaded and package `B` is then loaded, calling a function from `B` that depends on `A` can fail with: +As with `devtools::load_all()`, a package imports its dependencies' symbols as a +snapshot taken at its own load time. After editing a dependency, reload the +dependency **and** the packages that import from it so the importers pick up the +change. -```r -Error in function_in_a() : object 'lazydata' not found -``` +> Earlier versions failed here with `Error: object 'lazydata' not found` when a +> loaded package was accessed with `::`. That is fixed. ## Editor setup diff --git a/TECHNICAL_DEBT.md b/TECHNICAL_DEBT.md index 92a44c8..5c338e5 100644 --- a/TECHNICAL_DEBT.md +++ b/TECHNICAL_DEBT.md @@ -7,6 +7,12 @@ This document tracks known implementation debt and conscious tradeoffs in the `l - The package implementation under `R/` passes the current repo test suite. - The loader design is broadly sound for the target use case. - Most debt is in edge-case correctness and maintainability rather than basic functionality. +- Namespace fidelity has been brought in line with `loadNamespace()` for the + areas that affect real packages and multi-package sessions: S3 method + registration (`S3method()`), full export processing (`export()`, + `exportPattern()`, `exportClasses()`, `exportMethods()`), the `lazydata` / + `nativeRoutines` namespace-info fields, and the `.onAttach` hook. See the + namespace-machinery notes in `AGENTS.md`. ## Medium-priority debt @@ -48,8 +54,6 @@ If the `Package:` field in `DESCRIPTION` changes in place for the same directory ## Low-priority debt -## Low-priority debt - ### 3. Testthat detection logic is duplicated **Why this matters** @@ -96,7 +100,22 @@ When `renv.lock` changes, incremental reload continues to warn on later calls un This is intentional and tested. It favors making dependency drift obvious over silently accepting a new lockfile baseline. -### 3. `Collate` support is intentionally narrow +### 3. Incremental reload does not re-read `NAMESPACE` +The incremental path only re-sources changed `R/` files. It does not re-parse +`NAMESPACE`, so imports and the *set* of exports/`S3method()` declarations are +fixed at the initial full load. Editing a function body is picked up +incrementally (and S3 method tables are rebuilt so re-sourced methods take +effect), but **adding** a new `export()`, `S3method()`, or `importFrom()` needs +`full = TRUE`. + +This mirrors the existing treatment of imports and matches the "use `full = +TRUE` after structural changes" guidance. Re-parsing `NAMESPACE` every call was +not worth the cost for the edit-reload loop. + +**Current rule** +- Use `full = TRUE` after editing `NAMESPACE` (new exports, S3 methods, imports) + +### 4. `Collate` support is intentionally narrow The loader now respects the `Collate` field from `DESCRIPTION` when ordering `R/*.R` files, and this behavior is covered by the test suite. This is still intentionally lightweight rather than a full reproduction of every package-loading edge case. Future changes should preserve the current `Collate` behavior without overcomplicating the package implementation. diff --git a/devpackage/NAMESPACE b/devpackage/NAMESPACE index cf20777..99db7d1 100644 --- a/devpackage/NAMESPACE +++ b/devpackage/NAMESPACE @@ -4,3 +4,10 @@ importFrom(R6, R6Class) importFrom(data.table,":=") importFrom(data.table,as.data.table) importFrom(data.table,data.table) +export(new_temperature) +export(describe_s3) +S3method(print, temperature) +S3method(format, temperature) +S3method(as.character, temperature) +S3method(describe_s3, temperature) +S3method(describe_s3, default) diff --git a/devpackage/R/s3_classes.R b/devpackage/R/s3_classes.R new file mode 100644 index 0000000..573bef9 --- /dev/null +++ b/devpackage/R/s3_classes.R @@ -0,0 +1,41 @@ +# S3 classes, generics, and methods for testing loadfast's S3 support. +# +# These exercise the dispatch cases that only work when the namespace S3 methods +# table is populated from the S3method() directives in NAMESPACE: methods on a +# base generic (print/format/as.character) and methods on a generic defined in +# this package (describe_s3). None of the methods are exported by name, so +# dispatch cannot fall back to the search path — it must go through the table. + +new_temperature <- function(celsius) { + structure(list(celsius = celsius), class = "temperature") +} + +# Method on the base `print` generic; registered via S3method(print, temperature). +print.temperature <- function(x, ...) { + cat("Temperature:", x$celsius, "C\n") + invisible(x) +} + +# Method on the base `format` generic. +format.temperature <- function(x, ...) { + paste0(x$celsius, " C") +} + +# Method on the base `as.character` generic; delegates to format() to also +# exercise dispatch from inside another method. +as.character.temperature <- function(x, ...) { + format(x) +} + +# A generic defined in this package plus its methods. +describe_s3 <- function(x, ...) { + UseMethod("describe_s3") +} + +describe_s3.default <- function(x, ...) { + "an unknown object" +} + +describe_s3.temperature <- function(x, ...) { + paste0("a temperature of ", x$celsius, " degrees Celsius") +} diff --git a/devpackage/tests/testthat/test-base.R b/devpackage/tests/testthat/test-base.R index 195123f..5f22f9c 100644 --- a/devpackage/tests/testthat/test-base.R +++ b/devpackage/tests/testthat/test-base.R @@ -84,4 +84,27 @@ test_that("helper make_test_logger() builds a Logger with one entry", { lg <- make_test_logger() expect_equal(lg$size(), 1L) expect_equal(lg$last(), "init") +}) + +test_that("S3 methods on base generics dispatch", { + temp <- new_temperature(21) + expect_output(print(temp), "Temperature: 21 C") + expect_equal(format(temp), "21 C") + expect_equal(as.character(temp), "21 C") + # These tests run inside package:devpackage, where the methods are reachable by + # lexical scope, so dispatch alone would pass even if registration were broken. + # Assert the base generic's S3 table actually saw the method — that is what the + # loader's registerS3methods() call provides and what real dispatch relies on. + base_s3 <- get(".__S3MethodsTable__.", envir = asNamespace("base")) + expect_true(exists("format.temperature", envir = base_s3, inherits = FALSE)) +}) + +test_that("S3 methods on a package-defined generic dispatch", { + expect_equal( + describe_s3(new_temperature(21)), + "a temperature of 21 degrees Celsius" + ) + expect_equal(describe_s3(42), "an unknown object") + dev_s3 <- get(".__S3MethodsTable__.", envir = asNamespace("devpackage")) + expect_true(exists("describe_s3.temperature", envir = dev_s3, inherits = FALSE)) }) \ No newline at end of file diff --git a/test_loadfast.R b/test_loadfast.R index 6ca54e3..025f310 100644 --- a/test_loadfast.R +++ b/test_loadfast.R @@ -124,6 +124,21 @@ rename_package <- function(pkg_path, pkg_name) { } } +# The baseline ships an S3 fixture (s3_classes.R) that defines methods on the +# `temperature` class. When several packages are built from the same baseline in +# one session, they would all register those same methods and collide in R's +# shared S3 tables (an "overwritten" note per reload). Strip the fixture from +# packages that only exist to test cross-package imports so those stages stay +# focused and quiet. Dedicated S3 stages use their own, distinctly named classes. +strip_s3_fixture <- function(pkg_path) { + unlink(file.path(pkg_path, "R", "s3_classes.R")) + ns_path <- file.path(pkg_path, "NAMESPACE") + ns_lines <- readLines(ns_path, warn = FALSE) + drop <- grepl("^S3method\\(", ns_lines) | + grepl("^export\\((new_temperature|describe_s3)\\)$", ns_lines) + writeLines(ns_lines[!drop], ns_path) +} + replace_description_field <- function(desc_path, field, replacement_lines) { lines <- readLines(desc_path, warn = FALSE) start_idx <- grep(paste0("^", field, ":"), lines) @@ -509,6 +524,43 @@ check("Counter does NOT have decrement yet", quote( is.null(ctr1$decrement) )) +# --- S3 classes and methods --- +# The S3 methods are declared with S3method() (not exported by name), so correct +# dispatch depends on load_fast() populating the namespace S3 methods table via +# registerS3methods(). These would silently fall through to *.default (or the +# base method) if the table were left empty. +# A method on a base generic registers in base's S3 table; a method on a +# package-defined generic registers in the package's own S3 table. +check("S3 method for base generic registered in base's table", quote( + exists("print.temperature", envir = get(".__S3MethodsTable__.", envir = asNamespace("base")), inherits = FALSE) +)) + +check("S3 method for package generic registered in package's table", quote( + exists("describe_s3.temperature", envir = get(".__S3MethodsTable__.", envir = ns), inherits = FALSE) +)) + +s3_temp <- get("new_temperature", envir = ns)(21) + +check("S3 dispatch: format() finds the temperature method", quote( + format(s3_temp) == "21 C" +)) + +check("S3 dispatch: as.character() finds the temperature method", quote( + as.character(s3_temp) == "21 C" +)) + +check("S3 dispatch: print() finds the temperature method", quote( + any(grepl("Temperature: 21 C", utils::capture.output(print(s3_temp)), fixed = TRUE)) +)) + +check("S3 dispatch: package-defined generic dispatches to method", quote( + get("describe_s3", envir = ns)(s3_temp) == "a temperature of 21 degrees Celsius" +)) + +check("S3 dispatch: package-defined generic falls back to default", quote( + get("describe_s3", envir = ns)(42) == "an unknown object" +)) + # --- testthat helpers sourced into pkg env --- check("testthat is on the search path", quote( "package:testthat" %in% search() @@ -1170,6 +1222,8 @@ tmp_multi_a <- tempfile("loadfast_multi_a_") tmp_multi_b <- tempfile("loadfast_multi_b_") copy_baseline(tmp_multi_a) copy_baseline(tmp_multi_b) +strip_s3_fixture(tmp_multi_a) +strip_s3_fixture(tmp_multi_b) rename_package(tmp_multi_a, "packagea") rename_package(tmp_multi_b, "packageb") @@ -1338,6 +1392,8 @@ tmp_order_a <- tempfile("loadfast_order_a_") tmp_order_b <- tempfile("loadfast_order_b_") copy_baseline(tmp_order_a) copy_baseline(tmp_order_b) +strip_s3_fixture(tmp_order_a) +strip_s3_fixture(tmp_order_b) rename_package(tmp_order_a, "ordapkg") rename_package(tmp_order_b, "ordbpkg") @@ -2044,8 +2100,8 @@ check("lockfile: full reload resets warning baseline", quote( !any(grepl("renv.lock changed", lock_reload_full$warnings)) )) -# --- 4l: .onLoad hook is called during full and incremental loads --- -cat("\n--- 4l: .onLoad hook execution ---\n\n") +# --- 4l: .onLoad and .onAttach hooks are called during load --- +cat("\n--- 4l: .onLoad / .onAttach hook execution ---\n\n") tmp_onload <- tempfile("loadfast_onload_") copy_baseline(tmp_onload) @@ -2053,10 +2109,15 @@ remove_renv_lock(tmp_onload) writeLines(c( ".onLoad_call_count <- 0L", + ".onAttach_call_count <- 0L", ".onLoad <- function(libname, pkgname) {", " .onLoad_call_count <<- .onLoad_call_count + 1L", " .onLoad_libname <<- libname", " .onLoad_pkgname <<- pkgname", + "}", + ".onAttach <- function(libname, pkgname) {", + " .onAttach_call_count <<- .onAttach_call_count + 1L", + " .onAttach_pkgname <<- pkgname", "}" ), file.path(tmp_onload, "R", "zzz.R")) @@ -2078,6 +2139,14 @@ check(".onLoad: receives dirname(abs_path) as libname", quote( ) )) +check(".onAttach: is called on full load", quote( + get(".onAttach_call_count", envir = ns_onload) == 1L +)) + +check(".onAttach: receives correct pkgname", quote( + get(".onAttach_pkgname", envir = ns_onload) == "devpackage" +)) + # Incremental reload — nothing changed => short-circuit, .onLoad not re-called ns_onload_nochg <- load_fast(tmp_onload, helpers = FALSE, attach_testthat = FALSE) @@ -2085,6 +2154,11 @@ check(".onLoad: not called again on no-change reload", quote( get(".onLoad_call_count", envir = ns_onload_nochg) == 1L )) +# .onAttach is an attach-time hook; incremental reloads do not re-attach. +check(".onAttach: not re-called on incremental reload (no re-attach)", quote( + get(".onAttach_call_count", envir = ns_onload_nochg) == 1L +)) + # Trigger an incremental reload by modifying base.R writeLines(c( "add <- function(a, b) a + b + 9999" @@ -2096,10 +2170,250 @@ check(".onLoad: called again on incremental reload (files changed)", quote( get(".onLoad_call_count", envir = ns_onload_incr) == 2L )) +check(".onAttach: still not re-called after incremental reload with changes", quote( + get(".onAttach_call_count", envir = ns_onload_incr) == 1L +)) + check(".onLoad: add() reflects incremental change", quote( get("add", envir = ns_onload_incr)(1, 2) == 10002 )) +# --- 4m: incremental reload refreshes S3 methods --- +cat("\n--- 4m: incremental S3 method reload ---\n\n") + +tmp_s3incr <- tempfile("loadfast_s3incr_") +copy_baseline(tmp_s3incr) + +ns_s3i <- load_fast(tmp_s3incr, helpers = FALSE, attach_testthat = FALSE, full = TRUE) + +new_temp_s3i <- function(env, x) get("new_temperature", envir = env)(x) + +check("s3-incr: initial package-generic dispatch", quote( + get("describe_s3", envir = ns_s3i)(new_temp_s3i(ns_s3i, 5)) == + "a temperature of 5 degrees Celsius" +)) + +check("s3-incr: initial base-generic dispatch", quote( + format(new_temp_s3i(ns_s3i, 5)) == "5 C" +)) + +# Re-sourcing s3_classes.R replaces the method functions in the namespace; the +# S3 tables (base's for format, the namespace's for describe_s3) must be rebuilt +# to point at the new definitions rather than the stale ones. +writeLines(c( + "new_temperature <- function(celsius) {", + " structure(list(celsius = celsius), class = 'temperature')", + "}", + "print.temperature <- function(x, ...) { cat('T=', x$celsius, '\\n'); invisible(x) }", + "format.temperature <- function(x, ...) paste0('temp(', x$celsius, ')')", + "as.character.temperature <- function(x, ...) format(x)", + "describe_s3 <- function(x, ...) UseMethod('describe_s3')", + "describe_s3.default <- function(x, ...) 'unknown'", + "describe_s3.temperature <- function(x, ...) paste0('CHANGED:', x$celsius)" +), file.path(tmp_s3incr, "R", "s3_classes.R")) + +ns_s3i2 <- load_fast(tmp_s3incr, helpers = FALSE, attach_testthat = FALSE) + +check("s3-incr: package-generic method refreshed after incremental reload", quote( + get("describe_s3", envir = ns_s3i2)(new_temp_s3i(ns_s3i2, 5)) == "CHANGED:5" +)) + +check("s3-incr: base-generic method refreshed after incremental reload", quote( + format(new_temp_s3i(ns_s3i2, 5)) == "temp(5)" +)) + +# ============================================================================ +# STAGE 5: Cross-package fidelity (exports, S3 dispatch, S4 export, `::`) +# These exercise the machinery that only matters once a *second* package +# depends on a load_fast()-loaded one: exportPattern(), S3method registration +# across namespaces, S4 class/method export, and `pkg::name` resolution. +# Distinct class names are used per package so nothing collides. +# ============================================================================ +cat("\n--- Stage 5: cross-package fidelity ---\n\n") + +write_pkg <- function(root, name, desc_extra, ns_lines, r_files) { + dir.create(file.path(root, "R"), recursive = TRUE, showWarnings = FALSE) + writeLines( + c(paste0("Package: ", name), "Title: t", "Version: 0.0.1", + "Description: d.", "License: MIT", desc_extra), + file.path(root, "DESCRIPTION") + ) + writeLines(ns_lines, file.path(root, "NAMESPACE")) + for (fn in names(r_files)) writeLines(r_files[[fn]], file.path(root, "R", fn)) + .tmp_dirs <<- c(.tmp_dirs, root) +} + +# --- 5a: exportPattern() populates exports and `pkg::fn` resolves --- +cat("\n--- 5a: exportPattern and :: resolution ---\n\n") + +tmp_ep <- tempfile("loadfast_exportpattern_") +write_pkg( + tmp_ep, "eppkg", + desc_extra = character(0), + ns_lines = c("exportPattern(\"^[^.]\")"), + r_files = list("a.R" = c( + "ep_visible <- function(x) x * 3", + ".ep_hidden <- function() \"secret\"" + )) +) +ns_ep <- load_fast(tmp_ep, helpers = FALSE, attach_testthat = FALSE) + +check("exportPattern: matched name is exported", quote( + exists("ep_visible", envir = getNamespaceInfo(ns_ep, "exports"), inherits = FALSE) +)) + +check("exportPattern: dotted name is not exported", quote( + !exists(".ep_hidden", envir = getNamespaceInfo(ns_ep, "exports"), inherits = FALSE) +)) + +check("exportPattern: eppkg::ep_visible resolves without a 'lazydata' error", quote( + getExportedValue("eppkg", "ep_visible")(4) == 12 +)) + +# exportPattern(".") matches dotted names too; internal namespace machinery must +# still never be exported (base::loadNamespace applies the same stoplist). +tmp_epall <- tempfile("loadfast_exportall_") +write_pkg( + tmp_epall, "epallpkg", + desc_extra = character(0), + ns_lines = c("exportPattern(\".\")"), + r_files = list("a.R" = c( + "user_fn <- function() 1", + ".user_hidden <- function() 2" + )) +) +ns_epall <- load_fast(tmp_epall, helpers = FALSE, attach_testthat = FALSE) +epall_exports <- getNamespaceExports("epallpkg") + +check("exportPattern('.'): user names (dotted and plain) are exported", quote( + all(c("user_fn", ".user_hidden") %in% epall_exports) +)) + +check("exportPattern('.'): internal namespace objects are not leaked", quote( + !any(c(".__NAMESPACE__.", ".__S3MethodsTable__.", ".__DEVTOOLS__", + ".packageName", ".onLoad") %in% epall_exports) +)) + +# --- 5b: `pkg::missing` gives a proper not-exported error, not 'lazydata' --- +missing_err <- tryCatch( + getExportedValue("eppkg", "does_not_exist"), + error = function(e) conditionMessage(e) +) + +check("colon: missing export reports a not-exported error", quote( + grepl("not an exported object", missing_err, fixed = TRUE) +)) + +check("colon: missing export does not surface the 'lazydata' internal", quote( + !grepl("lazydata", missing_err, fixed = TRUE) +)) + +# --- 5c: S3 dispatch works across package boundaries --- +cat("\n--- 5c: cross-package S3 dispatch ---\n\n") + +tmp_s3prov <- tempfile("loadfast_s3prov_") +tmp_s3cons <- tempfile("loadfast_s3cons_") +write_pkg( + tmp_s3prov, "s3prov", + desc_extra = character(0), + ns_lines = c( + "export(new_gadget)", "export(render)", + "S3method(format, gadget)", "S3method(render, gadget)" + ), + r_files = list("a.R" = c( + "new_gadget <- function(id) structure(list(id = id), class = \"gadget\")", + "format.gadget <- function(x, ...) paste0(\"\")", + "render <- function(x, ...) UseMethod(\"render\")", + "render.default <- function(x, ...) \"default-render\"", + "render.gadget <- function(x, ...) paste0(\"rendered \", format(x))" + )) +) +write_pkg( + tmp_s3cons, "s3cons", + desc_extra = c("Imports:", " s3prov"), + ns_lines = c( + "importFrom(s3prov, new_gadget)", "importFrom(s3prov, render)", + "export(consume_format)", "export(consume_render)" + ), + r_files = list("b.R" = c( + "consume_format <- function(id) format(new_gadget(id))", + "consume_render <- function(id) render(new_gadget(id))" + )) +) +ns_s3prov <- load_fast(tmp_s3prov, helpers = FALSE, attach_testthat = FALSE) +ns_s3cons <- load_fast(tmp_s3cons, helpers = FALSE, attach_testthat = FALSE) + +check("xpkg-s3: consumer dispatches a base generic to the provider's method", quote( + get("consume_format", envir = ns_s3cons)(7) == "" +)) + +check("xpkg-s3: consumer dispatches the provider's own generic", quote( + get("consume_render", envir = ns_s3cons)(7) == "rendered " +)) + +# --- 5d: S4 class + method export/import works across packages --- +cat("\n--- 5d: cross-package S4 export/import ---\n\n") + +tmp_s4prov <- tempfile("loadfast_s4prov_") +tmp_s4cons <- tempfile("loadfast_s4cons_") +# The provider defines a method on the primitive generic `length` but does NOT +# list it in exportMethods(); it should still be exportable/importable. +write_pkg( + tmp_s4prov, "s4prov", + desc_extra = character(0), + ns_lines = c("import(methods)", "exportClasses(Sprocket)", "exportMethods(spin)"), + r_files = list("a.R" = c( + "setClass(\"Sprocket\", representation(teeth = \"numeric\"))", + "setGeneric(\"spin\", function(obj) standardGeneric(\"spin\"))", + "setMethod(\"spin\", \"Sprocket\", function(obj) obj@teeth * 2)", + "setMethod(\"length\", \"Sprocket\", function(x) length(x@teeth))" + )) +) +write_pkg( + tmp_s4cons, "s4cons", + desc_extra = c("Imports:", " s4prov, methods"), + ns_lines = c( + "import(methods)", "importClassesFrom(s4prov, Sprocket)", + "importMethodsFrom(s4prov, spin)", "importMethodsFrom(s4prov, length)", + "export(make_and_spin)", "export(count_teeth)" + ), + r_files = list("b.R" = c( + "make_and_spin <- function(n) spin(new(\"Sprocket\", teeth = n))", + "count_teeth <- function(v) length(new(\"Sprocket\", teeth = v))" + )) +) +ns_s4prov <- load_fast(tmp_s4prov, helpers = FALSE, attach_testthat = FALSE) + +check("xpkg-s4: primitive-generic method table auto-exported without exportMethods", quote( + any(grepl("__T__length", getNamespaceExports("s4prov"), fixed = TRUE)) +)) + +check("xpkg-s4: provider exports the class metadata object", quote( + exists( + paste0(methods::classMetaName(""), "Sprocket"), + envir = getNamespaceInfo(ns_s4prov, "exports"), inherits = FALSE + ) +)) + +s4cons_load <- tryCatch( + load_fast(tmp_s4cons, helpers = FALSE, attach_testthat = FALSE), + error = function(e) e +) + +check("xpkg-s4: consumer importClassesFrom + importMethodsFrom succeeds", quote( + is.environment(s4cons_load) && isNamespace(s4cons_load) +)) + +check("xpkg-s4: consumer uses the imported S4 class and method", quote( + !inherits(s4cons_load, "error") && + get("make_and_spin", envir = s4cons_load)(6) == 12 +)) + +check("xpkg-s4: consumer dispatches an imported primitive-generic S4 method", quote( + !inherits(s4cons_load, "error") && + get("count_teeth", envir = s4cons_load)(c(3, 5, 7)) == 3 +)) + # ============================================================================ # Summary # ============================================================================