Fix segfault in variables tests #641
Conversation
crates/ark/src/variables/variable.rs
Outdated
| env.bind("x".into(), harp::parse_eval_base(code).unwrap()); | ||
| let value = harp::parse_eval_base(code).unwrap(); | ||
| env.bind("x".into(), value.sexp); |
There was a problem hiding this comment.
I think what was happening is that with:
env.bind("x".into(), harp::parse_eval_base(code).unwrap());
harp::parse_eval_base(code).unwrap()creates aRObjectwhich does protectionenv.bind()consumesvalue: impl Into<SEXP>, and callsvalue.into()onvalue. This consumes theRObject(dropping and unprotecting it) and returns the unprotectedSEXP- The unprotected
SEXPis then used inRf_defineVar()
In theory it seems like Rf_defineVar() shouldn't do anything to trigger a reallocation, but in practice we see this crash.
More broadly I think this suggests a typing issue with RObject. I feel like its probably quite bad that we allow the underlying SEXP to escape the lifetime of RObject so easily. It's like we need to tie the lifetime of RObject to the lifetime of underlying SEXP unless you call something super explicit like unsafe into_sexp() which has a SAFETY note that you are taking protection ownership over the SEXP from here on out.
There was a problem hiding this comment.
Removing the impl Into<SEXP> wouldn't work because we could just write temporary().sexp right? hmm....
I wonder if .sexp should return a 'SEXP instead of a SEXP.
I was also thinking that view() should do the same to tie the lifetimes.
There was a problem hiding this comment.
Removing impl Into<SEXP> seems to fix the problem as it seems temporary() is only deleted after the function execution. At least that what I can tell from something like:
crates/ark/src/variables/variable.rs
Outdated
| env.bind("x".into(), harp::parse_eval_base(code).unwrap()); | ||
| let value = harp::parse_eval_base(code).unwrap(); | ||
| env.bind("x".into(), value.sexp); |
There was a problem hiding this comment.
Removing the impl Into<SEXP> wouldn't work because we could just write temporary().sexp right? hmm....
I wonder if .sexp should return a 'SEXP instead of a SEXP.
I was also thinking that view() should do the same to tie the lifetimes.
crates/harp/src/environment.rs
Outdated
| @@ -91,13 +91,13 @@ impl Environment { | |||
| std::iter::successors(Some(self.clone()), |p| p.parent()) | |||
| } | |||
|
|
|||
| pub fn bind(&self, name: RSymbol, value: impl Into<SEXP>) { | |||
| pub fn bind(&self, name: RSymbol, value: SEXP) { | |||
There was a problem hiding this comment.
hmm or maybe make it take an &RObject?
With current main, running:
Will eventually fail with:
While I'm not entirely sure what's happenning, I believe this is a protection issue, and this PR seems to fix the problem.