-
Notifications
You must be signed in to change notification settings - Fork 9
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.
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
Use common interface to iterate over environments #538
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
?
crates/harp/src/size.rs
Outdated
// in a immediate binding. More info in: | ||
// https://github.com/posit-dev/positron/issues/4686#issuecomment-2352427239 | ||
#[test] | ||
fn test_shinytest2_size() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! I'm happy to remove this test. I unfortunatelly can't find a way of constructing these inlined values in a simple way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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();
@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. A tricky part is that it has to work the same way on all R versions. |
@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. |
I updated the PR to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for fixing!
// This also means we won't count the size of internal implementation details of environments, | ||
// such as the pre-allocated size of hash tables, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
// `object_size_tree` is aware of promise objects. | ||
// The environment iterator will automatically return `PRVALUE` as | ||
// a `Standard` binding though. So this is only seeing unevaluated promises. | ||
// For evaluated promises, we are not counting the size of `PRCODE`, but hopefully |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
CAR
Co-authored-by: Lionel Henry <[email protected]>
bf67732
to
df4e7ca
Compare
Addresses: posit-dev/positron#4700
also addresses crashes from:
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.