Skip to content

Dark hackery #198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Dark hackery #198

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion R/signal.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
signal_stage <- function(stage, what, with = NULL, env = caller_env()) {
stage <- arg_match0(stage, c("experimental", "superseded", "deprecated"))
cnd <- new_lifecycle_stage_cnd(stage, what, with, env)
cnd_signal(cnd)
# cnd_signal(cnd)
.Internal(.signalCondition(cnd, "dummy", NULL))
}
Copy link
Member Author

@DavisVaughan DavisVaughan Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also note that cnd_signal() is not 1:1 with signalCondition() because cnd_signal() also adds a non-zero overhead call to withRestarts() to add a rlang_muffle restart. Do we need that? If not, then at the very least we could just call signalCondition() directly to avoid that overhead from cnd_signal() even if we don't do this hackery.

I have tracked the rlang_muffle thing back to this commit in rlang
r-lib/rlang@2c1bd29

Ah okay, so "bare conditions" signaled with signal() or cnd_signal() are mufflable by rlang::cnd_muffle() due to the addition of this rlang_muffle restart, while signalCondition() bare conditions are not. In other words this rlang_muffle restart helps bare conditions be more like warning and message conditions, which are always mufflable due to having muffleWarning and muffleMessage restarts available.

On one hand that feels important to keep around, but on the other hand when would you ever want to muffle this kind of signal which doesn't have any side effects? (unlike messages and warnings that print something)

Here's what just switching cnd_signal() to signalCondition() would look like, which we can do without any hackery. We lose the ability to muffle a lifecycle_signal condition because there won't be a rlang_muffle restart around anymore, but maybe that is totally fine.

cross::bench_branches(\() {
  library(lifecycle)
  
  # trigger the per session warning once
  deprecate_soft("1.1.0", I("my thing"), details = "for this", id = "foo")

  bench::mark(
    deprecate_soft("1.1.0", I("my thing"), details = "for this", id = "foo"),
    iterations = 100000
  )
})
#> # A tibble: 2 × 7
#>   branch                        expression                                   min median `itr/sec` mem_alloc `gc/sec`
#>   <chr>                         <bch:expr>                                <bch:> <bch:>     <dbl> <bch:byt>    <dbl>
#> 1 feature/bare-signal-condition "deprecate_soft(\"1.1.0\", I(\"my thing\… 25.3µs 26.6µs    36226.    8.56KB     71.9
#> 2 main                          "deprecate_soft(\"1.1.0\", I(\"my thing\… 31.7µs   33µs    29478.    8.56KB     67.7

Copy link
Member Author

Choose a reason for hiding this comment

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

I've opened #199 if we want to do this

Copy link
Member

Choose a reason for hiding this comment

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

when would you ever want to muffle this kind of signal which doesn't have any side effects?

Other handlers might have side effects. We have never really used that restart AFAIK, so we can remove it: r-lib/rlang#1836


new_lifecycle_stage_cnd <- function(stage, what, with, env) {
Expand Down
Loading