posit-dev / positron

Positron, a next-generation data science IDE
Other
1.59k stars 43 forks source link

Ark: Issue with covr's htmltools output #3813

Open DavisVaughan opened 1 week ago

DavisVaughan commented 1 week ago

Weirdly, if I step through this code one line at a time in the debugger then it works fine and opens in the viewer as expected. So that screams that it is some kind of timing bug.

https://github.com/posit-dev/positron/assets/19150088/36d30949-cc2a-4639-a09e-d9f0ccdf2d30

DavisVaughan commented 1 week ago

Oooh if I load covr completely first, then run the devtools function, it seems to work reliably.

library(covr)
devtools::test_coverage_active_file()

@lionel- is it possible the virtual namespace generation is somehow causing an issue?

Hmm maybe not since this will still fail

options(ark.resource_namespaces = FALSE)
devtools::test_coverage_active_file()
DavisVaughan commented 1 week ago

Pretty hard to get a traceback, but with options(warn = 2, error = recover) I can grab this

Enter a frame number, or 0 to exit   

 1: devtools::test_coverage_active_file()
 2: testthat::with_reporter(reporter, {
    coverage <- covr::environment_coverage(env, test_files, ...)
})
 3: tryCatch(code, testthat_abort_reporter = function(cnd) {
    cat(conditionMessage(cnd), "\n")
    NULL
})
 4: tryCatchList(expr, classes, parentenv, handlers)
 5: tryCatchOne(expr, names, parentenv, handlers[[1]])
 6: doTryCatch(return(expr), name, parentenv, handler)
 7: covr::environment_coverage(env, test_files, ...)
 8: withr::with_envvar(c(R_COVR = "true"), lapply(test_files, sys.source, keep.source = TRUE, envir = exec_env))
 9: force(code)
10: lapply(test_files, sys.source, keep.source = TRUE, envir = exec_env)
11: FUN(X[[i]], ...)
12: readLines(file, warn = FALSE)
13: file(con, "r")
14: getOption("url.method", "default")
15: .Internal(getOption(x)) %||% default
16: ..getNamespace(c("covr", "3.6.4"), "<unknown>")
17: .Internal(getRegisteredNamespace(name)) %||% tryCatch(loadNamespace(name), error = function(e) {
    tr <- Sys.getenv("_R_NO_R
18: .handleSimpleError(function (cnd) 
{
    defer({
        invoke_option_error_handler()
        poke_traceback()
        invokeRest
19: h(simpleError(msg, call))
20: errors.R#46: .ps.is_installed("rlang")
21: package.R#12: system.file(package = pkg)
22: find.package(package, lib.loc, quiet = TRUE)
23: .getNamespaceInfo(asNamespace(pkg), "path")
24: asNamespace(pkg)
25: getNamespace(ns)
26: .Internal(getRegisteredNamespace(name)) %||% loadNamespace(name)
DavisVaughan commented 1 week ago

Minimal reprex of:

Open rlang, then run this all at once

env <- pkgload::load_all()$env
replacements <- lapply(ls(env, all.names = TRUE), covr:::replacement, env = env)
replacements <- covr:::compact(replacements)
lapply(replacements, covr:::replace)

I'm somewhat confident that covr:::replace() is where the problem is, which does do straight up function modification, which could be affecting our namespace generation somehow https://github.com/r-lib/covr/blob/dd5286dd7cb3909456637a52d942f2428fa0a926/src/reassign.c#L8

DavisVaughan commented 1 week ago

Oh some additional testing tells me that this might be very specific to coverage on rlang. Possibly our internal usage of rlang in ark is conflicting with covr trying to alter the functions in the rlang namespace.

Regardless, this probably makes this issue less of a priority now. It seems like coverage for things like vctrs and dplyr work fine.

DavisVaughan commented 6 days ago

After some binary searching I have tracked the reprex down to

env <- pkgload::load_all()$env
replacements <- lapply(ls(env, all.names = TRUE), covr:::replacement, env = env)
replacements <- covr:::compact(replacements)
lapply(replacements[59], covr:::replace)

Where replacements[59] happens to correspond to %||%. Note that that comes from base R!

> replacements[59]
[[1]]
[[1]]$env
<environment: namespace:rlang>

[[1]]$name
`%||%`

[[1]]$orig_value
function (x, y) 
if (is.null(x)) y else x
<bytecode: 0x14637fec8>
<environment: namespace:base>

[[1]]$target_value
function (x, y) 
if (is.null(x)) y else x
<bytecode: 0x14637fec8>
<environment: namespace:base>

[[1]]$new_value
function (x, y) 
if (is.null(x)) y else x
<environment: namespace:base>

And indeed if I remove the rlang code that reexports the %||% from base R, the error goes away. So getting pretty close, I think.


Update:

I am now very convinced that this is a bug at the intersection of:

I say that because if you set do_resource_namespaces() to return false, the problem goes away! You can't just set options(ark.resource_namespaces = FALSE) after R starts up, because we've already populated the virtual namespace for base at this point, so the problem still shows up.

lionel- commented 6 days ago

wow that looks very tricky.

You can't just set options(ark.resource_namespaces = FALSE) after R starts up, because we've already populated the virtual namespace for base at this point, so the problem still shows up.

hmm we could probably check for the option in ns_populate_srcref() inside the idle task.