Fix handling of active bindings defined in packages

[klmr]

Currently, pkgload::load_all() fails with an error when the package defines active bindings with certain side effects. In other cases it might not fail but still cause undesirable side effects.

For instance, I have the following active binding defined in one of my packages, which triggered this error (I’m using it during development to define function stubs):

makeActiveBinding('not_implemented', \() stop('Function not implemented'), environment())

Of course it’s trivial to work around the limitation in this particular case. But many other conceivable active bindings would also cause issues. The proposed fix replaces eapply() with a loop that iterated over the names defined in a package and tests whether they are active bindings before evaluating them.

[lionel-]

Thanks for looking into this. Could you measure the performance cost of this change on large packages please? E.g. vctrs?

[klmr]

Not easy to measure since I can’t easily interleave the benchmarks so I’m performing them in isolated R sessions. Here are some numbers, but the median value varies by ±20ms between executions in both scenarios. I think it’s fair to say that, if there’s a difference at all, it’s minuscule.

packageVersion('pkgload')
# [1] ‘1.3.3’
pkgload::load_all()
# ℹ Loading vctrs

bench::mark(pkgload::load_all(), iterations = 10L)
# ℹ Loading vctrs
# # A tibble: 1 × 13
#   expression               min   median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result           memory time            gc
#   <bch:expr>          <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list>           <list> <list>          <list>
# 1 pkgload::load_all()    425ms    425ms      2.35        NA     33.0     1    14      425ms <named list [4]> <NULL> <bench_tm [10]> <tibble [10 × 3]>

pkgload::load_all(Sys.getenv('PKGLOAD_DEV_PATH'), attach = FALSE)
# ℹ Loading pkgload
packageVersion('pkgload')
# [1] ‘1.3.3.9000’

pkgload::load_all()
# ℹ Loading vctrs

bench::mark(pkgload::load_all(), iterations = 10L)
# ℹ Loading vctrs
# # A tibble: 1 × 13
#   expression               min   median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result           memory time            gc
#   <bch:expr>          <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list>           <list> <list>          <list>
# 1 pkgload::load_all()    406ms    422ms      2.13        NA     2.34    10    11       4.7s <named list [4]> <NULL> <bench_tm [10]> <tibble [10 × 3]>

Anyway, I’m not sure this benchmark is informative: we’d be benchmarking an extreme case (= ‘vctrs’) on an operation that, while important, shouldn’t be performance-critical. Even if there was a significant difference, would that really argue against applying a correctness fix?

[lionel-]

Thanks for the benchmark. It's not performance critical, in a way, but it does affect usability. We used to have perf issues with some packages and I think an eapply() loop was involved. I don't remember the details though.

Even if there was a significant difference, would that really argue against applying a correctness fix?

Perhaps so, since the difference would be "significant" in that case? There's lots of places in pkgload that are not "correct" because a trade-off decision was made in favour of concerns like simplicity and amount of work involved.

[lionel-]

Thanks!