Figure out what to do about R-devel API changes

[clauswilke]

A major revision of the R headers is underway. See here for an explanation:
https://stat.ethz.ch/pipermail/r-devel/2021-July/080931.html

And this is the first commit making changes:
wch/r-source@86b4a06

Note that many of the deletions are of macros that also exist as functions, so we won't be affected by those. However, we do currently have a problem with freshly generated R bindings for R-devel:
#229 (comment)

 error[E0425]: cannot find function, tuple struct or tuple variant `SET_PRSEEN` in this scope
      --> extendr-api/src/wrapper/promise.rs:27:13
       |
  27   |             SET_PRSEEN(sexp, 0);
       |             ^^^^^^^^^^ help: a function with a similar name exists: `SET_PRENV`
       | 
      ::: /home/runner/work/extendr/extendr/target/debug/build/libR-sys-fe61ba3184160aea/out/bindings.rs:4969:5
       |
  4969 |     pub fn SET_PRENV(x: SEXP, v: SEXP);
       |     ----------------------------------- similarly named function `SET_PRENV` defined here

The email by Luke Tierney encouraged extension developers to see whether the currently available API still works for them or whether additional functions are needed. So this is our opportunity to make our voice heard if something is missing.

@andy-thomason I think you wrote the code that uses SET_PRSEEN(). Could you review what's going on and let us know whether the code you wrote can work with the current API in R-devel?

[clauswilke]

This is the code that uses SET_PRSEEN():

extendr/extendr-api/src/wrapper/promise.rs

Lines 20 to 30 in a889113

	pub fn from_parts(code: Robj, environment: Environment) -> Result<Self> {
	unsafe {
	let sexp = Rf_allocSExp(PROMSXP);
	let robj = new_owned(sexp);
	SET_PRCODE(sexp, code.get());
	SET_PRENV(sexp, environment.robj.get());
	SET_PRVALUE(sexp, R_UnboundValue);
	SET_PRSEEN(sexp, 0);
	Ok(Promise { robj })
	}
	}

@ltierney Is there a specific reason SET_PRSEEN() is missing from Rinternals.h when the other promise setter functions are available?

See here:

void SET_PRENV(SEXP x, SEXP v); // used by dplyr, others
void SET_PRVALUE(SEXP x, SEXP v); // used by dplyr, others
void SET_PRCODE(SEXP x, SEXP v); // used by magrittr, others

(From: https://github.com/wch/r-source/blob/86b4a0641e889808ce204e28dd8bd19bbd1c8cbb/src/include/Rinternals.h#L1196-L1198)

[yutannihilation]

Here's the explanation about PRSEEN in R internals:

Bit 0 is used for PRSEEN, a flag to indicate if a promise has already been seen during the evaluation of the promise (and so to avoid recursive loops).

[yutannihilation]

Reading dplyr's code, it seems setting PRSEEN manually isn't necessary?

https://github.com/tidyverse/dplyr/blob/04454209ea069939d3335c43846c85c725547a89/src/chop.cpp#L17-L28

It sounds this risks a infinite loop if this is exposed to users, so I feel it might be reasonable not to include it in Rinternals.h (but I'm not sure about the details).

[yutannihilation]

Just removing SET_PRSEEN() fixes the build failure, at least.

#236

[ltierney]

On Thu, 22 Jul 2021, Claus Wilke wrote: This is the code that uses SET_PRSEEN(): https://github.com/extendr/extendr/blob/a8891130c216aaa45066880a18d198821d1 72306/extendr-api/src/wrapper/promise.rs#L20-L30 @ltierney Is there a specific reason SET_PRSEEN() is missing from Rinternals.h when the other promise setter functions are available? See here:

As it says a bit higher up in Rinternals.h: /* stuff that probably shouldn't be in the API but is getting used */

void SET_PRENV(SEXP x, SEXP v); // used by dplyr, others void SET_PRVALUE(SEXP x, SEXP v); // used by dplyr, others void SET_PRCODE(SEXP x, SEXP v); // used by magrittr, others

None of these are intended to be in the API. SET_PRSEEN is not used on CRAN/BIOC so was removed now; the others will need some work to wean some packages off of using them. For changes like this I run checks across CRAN and BIOC before committing and at least notify maintainers who might be affected. Longer term I would like to eliminate all direct use of promises outside of base code by moving to higher level concepts, like checking whether a binding has a deferred value. That would allow us to look into alternate implementations that have less overhead for function calls than we currently have. Similar comments apply to internals of environment objects. But this is all longer term. Hope that helps.

…

(From:https://github.com/wch/r-source/blob/86b4a0641e889808ce204e28dd8bd19bbd1c8c bb/src/include/Rinternals.h#L1196-L1198) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, orunsubscribe.[AA55UVFQ6K6TU36K2AGXRTDTY6PL7A5CNFSM5AZHNHI2YY3PNVWWK3TUL52HS4 DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGS5L5EQ.gif]

-- Luke Tierney Ralph E. Wareham Professor of Mathematical Sciences University of Iowa Phone: 319-335-3386 Department of Statistics and Fax: 319-335-3017 Actuarial Science 241 Schaeffer Hall email: ***@***.*** Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

[ltierney]

allocSExp zeros that bit so SET_PRSEEN(x, 0) isn't needed.

…

On Thu, 22 Jul 2021, Hiroaki Yutani wrote: Just removing SET_PRSEEN() fixes the build failure, at least. #236 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, orunsubscribe.[AA55UVB7RI74JZCPKLT6F63TY6VGXA5CNFSM5AZHNHI2YY3PNVWWK3TUL52HS4 DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGS5PJ2I.gif]

-- Luke Tierney Ralph E. Wareham Professor of Mathematical Sciences University of Iowa Phone: 319-335-3386 Department of Statistics and Fax: 319-335-3017 Actuarial Science 241 Schaeffer Hall email: ***@***.*** Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

[clauswilke]

Thanks Luke. We'll remove the call to SET_PRSEEN().

Looks like this was the only issue that popped up in our code base, so all in all not a big issue.

[yutannihilation]

allocSExp zeros that bit so SET_PRSEEN(x, 0) isn't needed.

Thanks for the information!

Longer term I would like to eliminate all direct use of promises outside of base code by moving to higher level concepts

This sounds good to me. It currently feels a bit too low level to handle promises with the C function. I hope we, as users of such unintended interfaces, can somehow contribute to establish the official API in future.

[yutannihilation]

I expected this issue has several more things that we need to handle, but it seems #236 fixes the failure. I'm closing this.