rlang::cnd_entrace(cnd) assumes that 'cnd' can be subsetted with `[[` and `.subset2()`, which I don't think it true

[HenrikBengtsson]

Reading help("condition"), there's nothing saying that a condition object should be a list. For example, there's in nothing in the R documentation that says the following is a invalid condition object:

cnd2 <- structure(42, class = "condition")

My interpretation of the help page is that it is written such that carefully avoids declaring what type a condition object should be. In contrast, other help pages are often explicit about the type, e.g. a "a list" or "a named list", etc. I asked about this in the R-devel thread "Is structure(NA, class = c("def", "condition")) a valid 'condition' object?" on 2025-10-07 (https://stat.ethz.ch/pipermail/r-devel/2025-October/084184.html) - there's only one reply thus far, which seems to agree with my interpretation.

Looking at the implementation of rlang::cnd_entrace(), it appears that it assumes that condition:s are of type list, or possible, environment, because it makes use of [[ and .subset2(). For example, this works:

cnd <- structure(list(), class = "condition")
cnd <- rlang::cnd_entrace(cnd)

but not:

cnd <- structure(42, class = "condition")
cnd <- rlang::cnd_entrace(cnd)
#> Error in x[["trace"]] : subscript out of bounds
#> 
#> 1: rlang::cnd_entrace(cnd)
#> 2: cnd_some(cnd, function(x) !is_null(x[["trace"]]))

Workaround

I think names() is defined for all types in R. This could be used to gate-keep the assumptions made by rlang, e.g.

diff --git a/R/cnd-entrace.R b/R/cnd-entrace.R
index 50ab54ea9..f64188e83 100644
--- a/R/cnd-entrace.R
+++ b/R/cnd-entrace.R
@@ -239,7 +239,7 @@ has_recover <- function() {
 cnd_entrace <- function(cnd, ..., top = NULL, bottom = NULL) {
   check_dots_empty0(...)
 
-  if (cnd_some(cnd, function(x) !is_null(x[["trace"]]))) {
+  if (cnd_some(cnd, function(x) "trace" %in% names(x) && !is_null(x[["trace"]]))) {
     return(cnd)
   }
 
diff --git a/R/cnd.R b/R/cnd.R
index 6b9286020..fb3d88494 100644
--- a/R/cnd.R
+++ b/R/cnd.R
@@ -381,7 +381,8 @@ cnd_some <- function(cnd, fn, ...) {
       return(TRUE)
     }
 
-    inherit <- .subset2(.subset2(cnd, "rlang"), "inherit")
+    inherit <- ("rlang" %in% names(cnd)) &&
+               .subset2(.subset2(cnd, "rlang"), "inherit")
     if (is_false(inherit)) {
       return(FALSE)
     }

There might be more instances in rlang where using $, [[, and .subset2() on condition need to be conditioned as above.

Background

FWIW, my R.oo package has an Exception class that inherits from error and condition is for this discussion effectively defined as:

cnd <- structure(NA, class = c("Exception", "error", "condition"))

This Exception class is how many of the error have been thrown in R.oo, R.utils, R.filesets, aroma.affymetrix, ... since ~2001. I became aware of this problem after a recent issue report (HenrikBengtsson/R.utils#161).

[hadley]

Hmmm, given that the vast majority of conditions do appear to be lists (including all conditions produced by base R and rlang) I'd be tempted to say that you should change your condition object 😄

[lionel-]

For the sake of simplicity, I would like to add my vote for condition objects being lists.

Henrik, could you please provide a motivation for not defining condition objects as a set of named fields? I think that's a reasonable assumption to make, and probably condition objects should be defined as such.

One caveat is that users should not access the message or call fields without using the S3 method accessors, so these fields are "undefined".

In any case, it's probably fine for R.oo to structure their conditions around list()?

[HenrikBengtsson]

I'm not sure if this can be decided by a popular vote - looking at help("conditions"), to me it very much looks like the author, which I guess is Luke T, very deliberately avoided declaring the type or any other behaviors. I think it would be useful to get his feedback to avoid running over his plans. Maybe he don't mind.

I think the condition system in R is yet to flourish - it's so underused and has so many potentials. If I'd speculate around the original design and guessing how conditions could be used, I can imagine supporting also environments in addition to lists might be something we want to keep an option. It would be unfortunately if we closed future doors by assuming lists only, unless really necessary. So, for me this, this issue is more important than just "fixing or working around a problem". I'd like to take R.oo's Exception class out of the discussion. It mostly serves as a revdep use cases that helped to detect this problem, and its two-decade+ track record shows that R's condition minimal API has been working this way for a long time.(*)

One could argue for the condition class should also support S3 methods [[ and [[<-. That wouldn't lock anyone in, and would be less restricted. You have support for that out of the box from lists and environments, and it's easy to extend for other classes. (FWIW, R.oo's Exception class already implements them).

Although it's supported by environments, I think the assumption for being able to use .subset2() might be too strong. (FWIW, if you didn't use that in rlang, then everything would work with R.oo and this issue would never have been noticed in the first place).

Again, it would be useful to get Luke's feedback on this. Could you please reply to my R-devel thread? That would help clarify what the condition API should be.

(*) The reason for me choosing NA in R.oo stems from legacy reasons. R.oo was created before R had namespaces and it even used an .Alias() function that R had at the time. Later that was migrated to use environments, but as an attribute (to allow for clean clones). I can probably change the Exception class to be of type list without breaking anything.

[hadley]

Personal opinion; allowing an S3 object of class "foo" to be of any base type makes it very hard/high-risk to program with and is generally ill-advised (even if classes might have done this, e.g. POSIXt) . Since simpleConditions() yields a list, I think it's reasonable to assume that conditions should be lists. And this doesn't really close the door on any other data structure, since you can always make (e.g) an environment an element of the list.

I would also argue against defining [[ and [[<- methods since no one has documented exactly what other methods need to be compatible (you need at least names() and length(), but what else?). It also adds performance overhead for (IMO) little gain.

[lionel-]

It would be unfortunately if we closed future doors by assuming lists only, unless really necessary

I would prefer to not close the door on condition objects being of predictable storage type ;)

[TimTaylor]

I can raise this as a new issue if preferred (perhaps also in testthat) but it is somewhat related ...

Ignoring the 'type' discussion for the moment, should cnd_entrace() document it is adding a trace field to the condition object to ensure that downstream usage protects user fields if necessary? By this I mean should testthat be doing more here:

library(testthat)

• User has a custom trace field in their condition object

test_that(
    "Testing trace clash",
    expect_true(stop(errorCondition("My error message", trace="hhmmm", class = "foo")))
)
#> ── Error: Testing trace clash ──────────────────────────────────────────────────
#> Error in `if (is.null(x[["trace"]]) || trace_length(x[["trace"]]) == 0L) {     return(x$message) }`: missing value where TRUE/FALSE needed
#> Error:
#> ! Test failed.

• No trace field (what testthat expects)

test_that(
    "Testing no-trace clash",
    expect_true(stop(errorCondition("My error message", class = "foo")))
)
#> ── Error: Testing no-trace clash ───────────────────────────────────────────────
#> <foo/error/condition>
#> Error: My error message
#> Backtrace:
#>     ▆
#>  1. └─testthat::expect_true(...)
#>  2.   └─testthat::quasi_label(enquo(object), label) at testthat/R/expect-constant.R:32:3
#>  3.     ├─testthat:::labelled_value(...) at testthat/R/quasi-label.R:56:3
#>  4.     └─rlang::eval_bare(expr, quo_get_env(quo)) at testthat/R/quasi-label.R:56:3
#> Error:
#> ! Test failed.

To this end would it also be preferable for cnd_entrace() to rely on attributes rather than fields. Accessors and setters of attributes are not generic (to the best of my knowledge) and should work on most objects. Whilst there could still be name clashes with trace I think they would be less likely as errorCondition() / warningCondition() add the ... to fields of the list, not attributes of the object.

Hope this all made sense and apologies for crashing the issue.

[lionel-]

@TimTaylor This discussion made me think about this too and I wondered if cnd_entrace() should wrap the error instead of modifying it.

That said we only modify it if trace doesn't exist, so in principle this is still safe enough.

[TimTaylor]

I wondered if cnd_entrace() should wrap the error instead of modifying it.

actually, yeah that would be much neater 😅

[lionel-]

So much neater that this is actually what we do!

Now that I've reacquainted myself with that API:

• cnd_entrace() is a low level tool that you use on your own conditions
• entrace() is what you use in condition handlers to create an rlang condition, and it does wrap the original condition (more precisely its message and call)

[TimTaylor]

So much neater that this is actually what we do!

Now that I've reacquainted myself with that API:

* `cnd_entrace()` is a low level tool that you use on your own conditions

* `entrace()` is what you use in condition handlers to create an rlang condition, and it does wrap the original condition (more precisely its message and call)

So is this a testthat misuse?

test_that(
    "Testing trace clash",
    expect_true(stop(errorCondition("My error message", trace="hhmmm", class = "foo")))
)
#> ── Error: Testing trace clash ──────────────────────────────────────────────────
#> Error in `if (is.null(x[["trace"]]) || trace_length(x[["trace"]]) == 0L) {     return(x$message) }`: missing value where TRUE/FALSE needed
#> Error:
#> ! Test failed.

The back trace being shown above seems to indicate so?

[hadley]

@TimTaylor Yes, I'd say that's a testthat issue.

[lionel-]

@t-kalinowski reminded me of this thread: #1664