Use common interface to iterate over environments

[dfalbel]

Addresses: posit-dev/positron#4700

also addresses crashes from:

• R session crashed when running a brms-model (Stan) positron#4686
• shinytest2 driver crashes the R session in Positron positron#4741

The approach taken here was to parse the SEXPREC header and check if the extra field is set. extra being set would actually indicate that it's an immediate binding.

[lionel-]

This approach is fine as a stopgap but in the long term we'll want to avoid depending on R internals.

Romain already added support for SxpInfo stuff actually, could you please use the existing Sxpinfo::interpret().is_extra()?

[lionel-]

hmm... I'd rather not have integration tests of this kind here.
I would prefer a unit test. If only because the integration test depends on internals that might change, which could reduce our coverage over time.

What do you think @DavisVaughan?

[dfalbel]

Ok! I'm happy to remove this test. I unfortunatelly can't find a way of constructing these inlined values in a simple way.

[lionel-]

hmm this should do it (although that depends on internals of the compiler):

let env = parse_eval_base("local({ f <- compiler::cmpfun(function() for (i in 1:3) return(environment())) f() })",).unwrap();

[lionel-]

@dfalbel We were planning on removing the dependency on sxpinfo to avoid Ark crashes when internals change. @DavisVaughan would like to go through with that change now: https://github.com/posit-dev/ark/pull/540/files

Could you please rewrite environment traversal so that it uses exposed API? E.g. findVar() (gets you values and promises), R_BindingIsActive(), R_ActiveBindingFunction(), etc.

A tricky part is that it has to work the same way on all R versions.

[lionel-]

@DavisVaughan Agreed, I had no idea we were this close. I guess our new environment iterator implementation went a long way.

I think the right way to approach things here is to create a new environment iterator that provides the binding type along the lines of https://gist.github.com/lionel-/1ebcbd5ec69c0775d514c329522408a3#binding-type. Then you could access this typed binding with specific accessors that we can tweak so they work well on all R versions.

[dfalbel]

I updated the PR to use harp's internal environment iterator, which should safely expand immediate bindings when necessary,and correctly avoids the evaluation of promises and active bindings. We'll no longer account the size of internal implementation details of environments, such as the pre-allocated size of the hash table, or the size each chain uses, etc.

[lionel-]

Looks good, thanks for fixing!

[lionel-]

Good point!

[lionel-]

No need to do now unless you want to, but the iterator should provide PRCODE too for promises. Luke agreed to provide promise accessors, as long as promises themselves are not exposed and instead accessed via the environment.