Rcpp: Possible Rcpp and ALTREP interaction

[eddelbuettel]

Over in this StackOverflow post we see that a sequence like 1:10 get sum()ed in R seemingly on SEXP header notion.

When Rcpp inserts a value, the sequence gets broken _but the bit does not get unset_. Thanks to @rstub to making it plain via .Internal(inspect). In short:

R> v <- 1:10
R> sum(v)
[1] 55
R> .Internal(inspect(v))
@559843d446c0 13 INTSXP g0c0 [NAM(3)]  1 : 10 (compact)
R> 
R> 
R> v[5] <- 100
R> sum(v)
[1] 150
R> .Internal(inspect(v))
@55984953b748 14 REALSXP g0c5 [NAM(1)] (len=10, tl=0) 1,2,3,4,100,...
R> 

In short we do not make the transition from the 1 : 10 (compact) representation to the enumerated materialized set -- yet sum() in R only looks at that representation bit and then returns the wrong result.

So with Rcpp (and the changed code by the OP):

R> Rcpp::cppFunction("IntegerVector modvec(IntegerVector x) { x[4] = 100; return(x); }")
R> v <- 1:10; sum(v); .Internal(inspect(v))
[1] 55
@559846e37070 13 INTSXP g0c0 [NAM(3)]  1 : 10 (compact)
R> 
R> modvec(v)
 [1]   1   2   3   4 100   6   7   8   9  10
R> 
R> sum(v); .Internal(inspect(v))           # so sum() now lies to us as we hide change
[1] 55       
@559846e37070 13 INTSXP g0c0 [NAM(3)]  1 : 10 (expanded)
R> 
R> v[5] <- 101
R> sum(v); .Internal(inspect(v))
[1] 151
@55984618c628 14 REALSXP g0c5 [NAM(1)] (len=10, tl=0) 1,2,3,4,101,...
R> 

@gmbecker If you have quick pointers... I still have your 'well ALTREP should not require user packages changes' ringing in my ear...

[kevinushey]

I'm guessing that the issue is similar to this:

`````

include

using namespace Rcpp;

// [[Rcpp::export]]
SEXP naughty(SEXP data) {
int* ptr = INTEGER(data);
ptr[5] = 100;
return data;
}

/* R
v <- 1:10
naughty(v)
v
sum(v)
*/
`````

which gives me as output:

> Rcpp::sourceCpp('~/scratch/naughty.cpp')
> v <- 1:10
> naughty(v)
 [1]   1   2   3   4   5 100   7   8   9  10
> v
 [1]   1   2   3   4   5 100   7   8   9  10
> sum(v)
[1] 55

In other words, because we modify the data 'directly' through INTEGER() / DATAPTR(), we skip R's normal ALTREP checks and so we end up with an R object in an inconsistent state.

I _think_ ALTREP is normally supposed to force materialization of the underlying vector in these cases though?

[eddelbuettel]

Not just similar, this is the same issue -- but your example is nicely minimal and shows we can also shoot ourselves in the foot using the C API of R.

[rstub]

A quote from the R sources:

/* By default, compact integer sequences are marked as not mutable at
   creation time. Thus even when expanded the expanded data will
   correspond to the original integer sequence (unless it runs into
   mis-behaving C code). If COMPACT_INTSEQ_MUTABLE is defined, then
   the sequence is not marked as not mutable. Once the DATAPTR has
   been requested and releases, the expanded data might be modified by
   an assignment and no longer correspond to the original sequence. */
//#define COMPACT_INTSEQ_MUTABLE

https://github.com/wch/r-source/blob/9b2a6fc19ca853cff5ae09cb0efcfb179c6da3a5/src/main/altclasses.c#L60-L67

And later on:

static SEXP compact_intseq_Sum(SEXP x, Rboolean narm)
{
#ifdef COMPACT_INTSEQ_MUTABLE
    /* If the vector has been expanded it may have been modified. */
    if (COMPACT_SEQ_EXPANDED(x) != R_NilValue) 
    return NULL;
#endif
...

https://github.com/wch/r-source/blob/9b2a6fc19ca853cff5ae09cb0efcfb179c6da3a5/src/main/altclasses.c#L258-L264

Somehow this looks like intended behavior?!

I do not understand, though, why it does work as expected when called from R:

        int *px = INTEGER(x);
        VECTOR_ASSIGN_LOOP(px[ii] = INTEGER_ELT(y, iny););

https://github.com/wch/r-source/blob/9b2a6fc19ca853cff5ae09cb0efcfb179c6da3a5/src/main/subassign.c#L714-L715

Or am I looking at the wrong place?

[eddelbuettel]

We may need a second (and third) set of eyes. I'll email @gmbecker @kalibera @ltierney later with a link to this and the StackOverflow issue referenced above.

[kalibera]

Please make sure you are not violating the value semantics of R when changing the object. See WRE 5.9.10.

[eddelbuettel]

@kalibera I am not sure I see how 5.9.10 applies. This is about modifcation in place, ie

SEXP naughty(SEXP data) {
  int* ptr = INTEGER(data);
  ptr[5] = 100;
  return data;
}

This code appears to be too minimal. How should be done in an ALTREP world?

[kalibera]

Well you can only modify "data" when it is not referenced (!MAYBE_REFERENCED, NAMED value is 0), otherwise you should duplicate and modify the copy - and then also return the copy from the function. That part is independent of ALTREP but also ALTREP relies on that these rules are followed, so it would be best to have reproducible example that follows them, too (and does not use any non-base packages, including rcpp). Other than that, you can now use SET_INTEGER_ELT instead of INTEGER/DATAPTR but that should only matter for performance, and you still need to check for references and copy accordingly.

[eddelbuettel]

Thanks, that was more helpful though I unsure where you see non base R in the now-twice-quoted C++ snippet. We use Rcpp for convenience; I am apparently unable to call a symbol from a DLL loaded after R CMD SHLIB and dyn.load -- the .Call() complains about an unreg symbol. Anyway...

When I use SET_INTEGER_ELT I can still fool the ALTREP-aware sum() in R:

R> Rcpp::cppFunction("SEXP nice(SEXP x) { SET_INTEGER_ELT(x, 4, 100); return x; }")
R> x <- 1:10
R> sum(x)
[1] 55
R> x <- nice(x)
R> sum(x)
[1] 55
R> x
 [1]   1   2   3   4 100   6   7   8   9  10
R> 

I hope you forgive for use Rcpp to materialize the code. What matters is the use of SET_INTEGER_ELT. It alone seem insufficient for (re-)setting the right bits even though the status changes from 'compact' to 'expanded':

R> .Internal(inspect(x))                # same x as above with 100 hiding in the middle
@1570ee8 13 INTSXP g0c0 [NAM(3)]  1 : 10 (expanded)
R> .Internal(inspect(1:10))           # where sequence is compact
@13caa88 13 INTSXP g0c0 [NAM(3)]  1 : 10 (compact)
R> 

[ltierney]

Rcpp::cppFunction("SEXP nice(SEXP x) { if (MAYBE_SHARED(x)) Rf_error(\"you are being naughty\"); INTEGER(x)[4] = 100; return x;}")
nice(1)
Error in nice(1) : you are being naughty
nice(1:10)
Error in nice(1:10) : you are being naughty
nice(1 + 0)
Error in nice(1 + 0) : you are being naughty

DO NOT MODIFY AN OBJECT IF MAYBE_SHARED RETURNS TRUE!!!

Your nice convenient wrapper produces a closure. At least with the
current NAMED framework MAYBE_SHARED will ALWAYS return TRUE for
arguments passed into your C/C++ code this way. So:

DO NOT MODIFY OBJECTS PASSED TO YOUR C/C++ code this way!!!

If you do { x <- ...; .Call(foo, x) } then your 'foo' MAY see an
object with MAYBE_SHARED(X) FALSE; it may be OK to modify that one.

Compact sequences are always marked as immutable -- they have to be
duplicated before modifying. This could have been done differently,
and may change, but this is how it is now, and if you follow the rules
it is not a problem.

If an ALTREP object that is marked as immutable returns a data pointer
into a read-only memory segment (as a memory-mapped file object might)
and you don't follow the rules and write anyway then you know what
will happen.

So follow the rules.

Best,

luke

On Thu, 28 Feb 2019, Dirk Eddelbuettel wrote:

Thanks, that was more helpful (though I unsure where you see non base R in
the now-twice-quoted C++ snippet. We use Rcpp for convenience; I am
apparently unable to call a symbol from a DLL loaded after R CMD SHLIB and
dyn.load -- the .Call() complains about an unreg symbol. Anyway...

When I use SET_INTEGER_ELT I can still fool the ALTREP-aware sum() in R:

R> Rcpp::cppFunction("SEXP nice(SEXP x) { SET_INTEGER_ELT(x, 4, 100); return
x; }")
R> x <- 1:10
R> sum(x)
[1] 55
R> x <- nice(x)
R> sum(x)
[1] 55
R> x
[1] 1 2 3 4 100 6 7 8 9 10
R>

I hope you forgive for use Rcpp to materialize the code. What matters is the
use of SET_INTEGER_ELT. It alone seem insufficient for (re-)setting the
right bits even though the status changes from 'compact' to 'expanded':

R> .Internal(inspect(x)) # same x as above with 100 hiding in
the middle
@1570ee8 13 INTSXP g0c0 [NAM(3)] 1 : 10 (expanded)
R> .Internal(inspect(1:10)) # where sequence is compact
@13caa88 13 INTSXP g0c0 [NAM(3)] 1 : 10 (compact)
R>

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the
thread.[ADvaVOY2mjbw4o8nc81Y9bYg_tU7ogR8ks5vR_5lgaJpZM4bUh2K.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: [email protected]
Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

[eddelbuettel]

Thanks, @ltierney. I think Rcpp may generally be a code review step behind the enhanced reference counting that provided the 'base layer' for ALTREP. We are getting by because the pattern at the start of this more recent thread (ie in the StackOverflow question) is rare. We don't often get sequence as input and output whil also being modified. But the overall need for paying more attention to MAYBE_SHARED(x) is clear. The devil, however, is in the detail as it is not clear how to operate on a FALSE that can be ignored (as in your last example).

[ltierney]

Just to be clear: this is now new with ALTREP. The MAYBE_SHARED test
is a bit more recent, still well before ALTREP, but the basic idea
what C code should not modify objects that might be shared has been
true since day 1. Violations were more likely to go unnoticed in the
days when R did more duplicating. But as otherwise unnecessary
duplicate() calls have been removed, more of these issues are
unmasked.

On Thu, 28 Feb 2019, Dirk Eddelbuettel wrote:

Thanks, @ltierney. I think Rcpp may generally be a code review step behind
the enhanced reference counting that provided the 'base layer' for ALTREP.
We are getting by because the pattern at the start of this more recent
thread (ie in the StackOverflow question) is rare. We don't often get
sequence as input and output whil also being modified. But the overall need
for paying more attention to MAYBE_SHARED(x) is clear. The devil, however,
is in the detail as it is not clear how to operate on a FALSE that can be
ignored (as in your last example).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the
thread.[ADvaVACT1Nk1fpwpP9qLiE24rDxr2EQ9ks5vSAzmgaJpZM4bUh2K.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: [email protected]
Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

[eddelbuettel]

Sure. "Empirically speaking", everybody seems to get by because the main usage patterns appears to mostly be "functional" in the sense that incoming variable are not modified, and newly created variable are returned. But as has been pointed out before, the internal organization of the code could be stricter in enforcing that. I would be extremely receptive to constructive help but I understand that everybody is busy. So are we, so no ETA for when this may get improved...

[gmbecker]

@eddelbuettel et al,

So there's a couple of things here, most of which @ltierney and/or @kalibera alluded to but which perhaps can benefit from more concrete possible coding patterns.

The crux of the matter here is inline modifying the payload of a SEXP by writing to elements of its DATAPTR-addressed memory. Is it ever ok to do (yes), can it be safe to do without confirming/being guaranteed its ok to do (no, never). And for objects created in the same chunk of C/C++n code, you may have such a priori guarantees, but you're not going to for objects passed down from R.

To be concrete, I don't know of any times it would be ok to write to the pointer returned by INTEGER() on a SEXP that lives in R-space without first checking MAYBE_SHARED(). If MAYBE_SHARED(x) returns FALSE, then it's ok, and you can proceed writing to the pointer, exactly as your code did.

If MAYBE_SHARED(x) == TRUE, then you need to duplicate, operate on the copy, and then return that. When you're in C/C++ code, at the level of grabbing data pointers to things, then you, your code needs to explicitly cause that duplication, protect the new duplicated result, etc.

Now the reason that this particular thing is happening is in the compact sequence case is that, unless R itself is build a specific non-default way, compact sequences ALWAYS have NAMED(x) == MAXNAMED (edit: ie 7, not 2 as I previously stated), from the point of creation. As Luke pointed out, this could change, but is currently by design. So even in a .Call situation where the closure wasn't forcing the NAMED count up, compact sequences will always need duplication before inline modification. And while this is a choice we could have made different in the compact sequence case, Luke's point about other ALTREPs is more important.

There may be ALTREP SEXPs where the memory pointed to by the pointer returned by, e.g., INTEGER is _literally not writable memory_ for one reason or another. The way that those ALTREP classes will declare that is by doing the same thing the compact sequences do, by marking themselves as "IMMUTABLE", ie by doing MARK_NOT_MUTABLE(x) (which currently sets NAMED to MAXNAMED, but which is future-proof against eventual change to reference counting). This declares the contract that the SEXP _must_ be duplicated before any code that grabs the datapointer and writes to it.

Ultimately, I agree this is a really weird unexpected behavior, but its due to a failure to meet the contract which has always been there. It _may_ have been safe(ish, and I still doubt it) to ignore/be lax about in certain cases in the past, but with the advent of ALTREP it now must always be followed for the reasons outlined in this thread.

So all all all code thats going to grab a dataptr from a pre-existing (R-level) SEXP and write to it must follow a pattern along the lines of (or equivalently careful to):

SEXP awesomefun(SEXP x) 
{
    int nprot = 0;
    if(MAYBE_SHARED(x)) {
        PROTECT(x = duplicate(x)); nprot++; 
    }
    /* do awesome things to x that modify it inline
        protect other things as necessary but always increment nprot when you do, 
        decrement nprot if you ever unprotect */
    if(nprot) UNPROTECT(nprot);
    return x;
}

Any code which writes to datapointers retrieved from SEXPs it didn't itself create (ie anything that comes down from the R side) without doing this was already violating the C-API contract but is now also fatally ALTREP unsafe, as the motivating example showed.

And, one more time, remember, compact sequences could behave differently so that that code happened to work, but other ALTREP classes (Luke mentions memory-mapped-file backed ALTREPS) couldn't, so the behavior of compact sequences isn't actually the issue here.

I hope that is helpful and makes things clearer.

Best

[gmbecker]

Small point of order correction, I spoke too quickly. NAMEDMAX is not 2, its 7 in modern versions of R. sorry for that.

[HervePerdry]

Just a quick, unhelpful comment, but I just can't stay silent: as a Rcpp user I (strongly) prefer the current behavior (don't check if mutable, don't create new object, allow in-place modification), even if sum have a problem with it.

We all know that y <- x; nice(y) modifies both x and y and we deal with it (some of us may even like it). We just need to know which functions are affected. I tried prod and diff, which where the most obvious candidates to me, they don’t rely on the compact form. It would be nice to have a list.

[kalibera]

I understand you would like mutable objects in R, but you cannot get them this way. Violating the value semantics can have global effects (in the small example you give, it is because x could already have been used elsewhere, etc), you could not isolate them by listing several functions. One example I spent many days debugging was that someone mutating objects in place managed to change all 0 (constant literals) in an R function to 1. The R runtime has many layers of code that legitimately depend that on the rules that are documented are also followed, and thanks to that it provides the functionality and performance we have. If the rules are not followed, the results are unpredictable and the problems are certainly not local to user "functions" that violate them.

Indeed, one can have a mutable vector outside of R heap. Also, one can allocate in a C function a vector off the R heap and keep modifying it before passing it to R (so ensuring there are no R references to it). Also, R environments are mutable (details in Writing R Extensions).

[HervePerdry]

All right, I think I get your point.

Just to be sure, from the R point of view, the nice() example (a Rcpp function that modifies its argument) is an abomination "in general", for any integer vector, not only when applied to vectors with a compact form?

[kalibera]

You are right, it is always wrong, not just when applied to vectors in a compact form. These rules have been in place for many years, long before any compact forms existed. One needs to check, now best using MAYBE_SHARED, and if true, duplicate. A compact form is just one of many causes of MAYBE_SHARED returning true. So, old code complying with the rules will still work correctly with compact forms (even though in some cases less efficiently than it could).

[eddelbuettel]

But per Luke's example and point, the Rcpp closure _always_ returns a "wrong" MAYBE_SHARED.

The following (in C) "works" for the last two example and (correctly) suggests copies for the 1st:

R> setwd("~/git/rcpp_shared/")
R> system("cat works.c")

#include <R.h>
#include <Rinternals.h>

SEXP isitC(SEXP x) {
    if (MAYBE_SHARED(x)) {
        Rprintf("Maybe shared\n");
    }
    return(x);
}
R> system("R CMD SHLIB works.c")
ccache gcc -I"/usr/share/R/include" -DNDEBUG      -fpic  -g -O3 -Wall -pipe   -std=gnu99 -c works.c -o works.o
ccache gcc -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -Wl,-z,relro -o works.so works.o -L/usr/lib/R/lib -lR
R> dyn.load("works.so")
R> .Call("isitC", 1L)
Maybe shared
[1] 1
R> .Call("isitC", c(1L,2L,3L))
[1] 1 2 3
R> x <- rnorm(4); .Call("isitC", x)
[1]  1.389025 -0.910219  0.916383  0.386044
R> 

But because of the wrapping closure from sourceCpp() (and the other Rcpp construct) we get a constant result in Rcpp:

R> system("cat works.cpp")
#include <Rcpp.h>

// [[Rcpp::export]]
SEXP isitCpp(SEXP x) {
    if (MAYBE_SHARED(x)) {
        Rprintf("Maybe shared\n");
    }
    return(x);
}
R> Rcpp::sourceCpp("works.cpp")
R> isitCpp(1L)
Maybe shared
[1] 1
R> isitCpp(c(1L,2L,3L))
Maybe shared
[1] 1 2 3
R> x <- rnorm(4); isitCpp(x)
Maybe shared
[1]  0.2437037  0.0357416 -0.5986804  0.5094981
R> 

Three for three. Not correct, or at least not what the C version had.

The answer cannot possibly be to copy every incoming object that Rcpp receives.

[ltierney]

On Mon, 4 Mar 2019, Dirk Eddelbuettel wrote:

But per Luke's example and point, the Rcpp closure always returns a "wrong"
MAYBE_SHARED.

Not wrong: conservative. Has to be under NAMED as far as I can
see. REFCNT can be less conservative; you can build R with USE_REFCNT
defined to see what happens there. Hopefully we can make a push to
REFCNT in the net cycle.

The following (in C) "works" for the last two example and (correctly)
suggests copies for the 1st:

R> setwd("~/git/rcpp_shared/")
R> system("cat works.c")

include

include

SEXP isitC(SEXP x) {
if (MAYBE_SHARED(x)) {
Rprintf("Maybe shared\n");
}
return(x);
}
R> system("R CMD SHLIB works.c")
ccache gcc -I"/usr/share/R/include" -DNDEBUG -fpic -g -O3 -Wall -pipe
-std=gnu99 -c works.c -o works.o
ccache gcc -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -Wl,-z,r
elro -o works.so works.o -L/usr/lib/R/lib -lR
R> dyn.load("works.so")
R> .Call("isitC", 1L)
Maybe shared
[1] 1
R> .Call("isitC", c(1L,2L,3L))
[1] 1 2 3
R> x <- rnorm(4); .Call("isitC", x)
[1] 1.389025 -0.910219 0.916383 0.386044
R>

But because of the wrapping closure from sourceCpp() (and the other Rcpp
construct) we get a constant result in Rcpp:

R> system("cat works.cpp")

include

// [[Rcpp::export]]
SEXP isitCpp(SEXP x) {
if (MAYBE_SHARED(x)) {
Rprintf("Maybe shared\n");
}
return(x);
}
R> Rcpp::sourceCpp("works.cpp")
R> isitCpp(1L)
Maybe shared
[1] 1
R> isitCpp(c(1L,2L,3L))
Maybe shared
[1] 1 2 3
R> x <- rnorm(4); isitCpp(x)
Maybe shared
[1] 0.2437037 0.0357416 -0.5986804 0.5094981
R>

Three for three. Not correct, or at least not what the C version had.

The answer cannot possibly to copy every incoming object that Rcpp receives.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the
thread.[ADvaVDpvA24AqHt2qXnLHFBz03cIlRt6ks5vTQ5lgaJpZM4bUh2K.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: [email protected]
Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

[kevinushey]

The problem is that client packages of Rcpp have relied on "knowing better" than R in these cases to mutate objects in place, and have done so for quite some time now. Changing Rcpp to follow the rules now would almost certainly break a large number of Rcpp-using packages currently on CRAN, since one could easily expect that a number of these packages rely on these semantics to modify 'vanilla' R objects in-place, even if R was telling them no, don't do that.

In other words, Rcpp made it possible for users to "break the rules" when they "knew better" than R as to whether an object really was shared. We (AFAIR) documented that this can and does break things if one is not careful, and effectively placed the onus on users to fix things if their attempts to modify in-place caused issues.

This strategy is obviously not sustainable going forward but I see the following paths forward that minimize breakage on CRAN:

1. Force any ALTREP objects that are used by Rcpp to become materialized to their "plain" R object form; ie, convert compact integer sequences to regular integer vectors and so on;

2. Wait until true reference counting becomes the default in R, and then switch to appropriately using MAYBE_SHARED() when constructing Rcpp objects from R objects (since hopefully at that point R will no longer tag arguments to closures as shared and so Rcpp could safely modify these objects in place as appropriate)

This is where Rcpp does its 'copies' (read: copies the pointers, not the underlying memory):

https://github.com/RcppCore/Rcpp/blob/08344ae80da376eb7a153467364ea0bb8c2ce4ba/inst/include/Rcpp/storage/PreserveStorage.h#L35-L41

In theory, checking MAYBE_SHARED() and duplicating there will help resolve the issue, but there's almost certainly other places in the codebase where we would need to adequately check MAYBE_SHARED().

[eddelbuettel]

1.a. Or add smarter ALTREP functions. As the point of ALTREP it to go beyond the old 'materialize whatever it is in memory' to allow read-only (the mmap example) or 'yuge' (out of memory) or ... vectors is something we'll support. Eventually. Maybe when we have true reference counting.

Until then we'll see about not letting too much damage happen by materializing some more. @kevinushey 's suggestion is an excellent first step.

[kalibera]

Any code that modifies an object in place without checking if it is shared is wrong and needs to be fixed. It would be natural if Rcpp, having access to C++ features and having provided classes for individual SEXPs, made it harder (not easier!) to write such buggy code. It would be really helpful if Rcpp turned this into an error. This is not related to ALTREP, only in that ALTREP increases the chances that these errors will get noticed (as they materialize as crashes etc). Of course, for a transition, such checks could be first optional, if too many packages needed to be fixed.

There is no point in waiting for reference counting, the cases when objects are over-conservatively marked shared due to having been passed to a function but not escaped, are extremely rare and users have no chance identifying them correctly: code has to be written anyway to check if the object is shared and copy if it is (or possibly throw (assertion) error if shared and the author believes it cannot be shared...). Rcpp could provide means to make this easier. More bugs like this are in the packages, harder it is to maintain R or to change anything in it (and of course that includes reference counting).

The few cases I've seen so far that suffered from the problem that arguments to closures become shared could have been solved by allocating the R objects in C code and modifying them before they are passed to the R interpreter.

[eddelbuettel]

But one can write malicious code in any language -- see the example above which takes the memory pointer and reassigns. In C code. Is R Core going to outlaw that in base R?

I happen to think that Rcpp is an alternative approach to extending R, and one which may actually make it _simpler_ to write safe code (no PROTECT errors). Avoiding the closure miscount may be possible, but we, just like R Core, have limited resources.

[eddelbuettel]

This is an old discussion but it occurred to me recently that
a) we have the closure problem with biasing what MAYBE_SHARED(x) sees
b) but we do have the underlying DLL so maybe we can come up with a way identify the entry point differently ?

Here is the same C++ example as above again built via sourceCpp() but this time with verbose=TRUE to see what the (generated) entry point would be. Then:

 R> .Call("sourceCpp_7_isitCpp", 7L)
 Maybe shared
 [1] 7
 R> .Call("sourceCpp_7_isitCpp", c(8,9,10))
 [1]  8  9 10
 R>

Now the behaviour is the same as in the C example -- and as expected.

[s-u]

@kevinushey, @eddelbuettel maybe it's time to revisit ~and avoid bugs~? In R 4.0.0 we now have

> .Call("foo", 1+1)
[1] 2
> f=function(x) .Call("foo", x)
> f(1+1)
[1] 2

ass opposed to R 3.6.3:

> .Call("foo", 1+1)
[1] 2
> f=function(x) .Call("foo", x)
> f(1+1)
Maybe shared
[1] 2

so adhering to the rules is less conservative now.

[eddelbuettel]

I had coming back to this issue on the docket as well but hadn't gotten there yet. Thanks for re-running the example.

[waynelapierre]

any updates?