Use API setter/unsetter for S4 objects

[MichaelChirico]

Part of #1933.

This passes tests for me. I haven't explored any performance implications (this can induce a copy, right?).

[lionel-]

I haven't explored any performance implications (this can induce a copy, right?).

It looks it only makes a copy when the object might be shared, which seems right.

Do you know what the complete argument does?

[MichaelChirico]

Per ?asS4

complete Optional, logical: whether conversion to S3 is completed. Not usually needed, but see the details section.

asS3 uses the value of complete to control whether an attempt is made to transform object into a valid object of the implied S3 class. If complete is TRUE, then an object from an S4 class extending an S3 class will be transformed into an S3 object with the corresponding S3 class (see S3Part). This includes classes extending the pseudo-classes array and matrix: such objects will have their class attribute set to NULL.

Can't say I fully understand the meaning there, nor does the source mean much to me:

https://github.com/r-devel/r-svn/blob/a068a0f52b9ae871212779b2c1571a999f23ef81/src/main/objects.c#L1853-L1869

Here's the three callsites in r-devel:

https://github.com/r-devel/r-svn/blob/a068a0f52b9ae871212779b2c1571a999f23ef81/src/main/arithmetic.c#L701
https://github.com/r-devel/r-svn/blob/a068a0f52b9ae871212779b2c1571a999f23ef81/src/main/objects.c#L1819
https://github.com/r-devel/r-svn/blob/a068a0f52b9ae871212779b2c1571a999f23ef81/src/library/base/R/lazyload.R#L83

I also only see a handful of usages on CRAN, one of them sets /*complete=*/FALSE with a comment that this was replacing SET_S4_OBJECT():

https://github.com/search?q=org%3Acran+%2FasS4%5C%28%2F+lang%3AC%2B%2B&type=code

[lionel-]

It looks like when complete is set it tries to return an internal slot in R_getS4DataSlot()?

The mark_ functions were meant to mutate the objects. I see that this one returns its input but have you checked the call sites to verify that there is no assumption of mutation?

It seems like setting complete to 0 will be the least disruptive change though?

[MichaelChirico]

have you checked the call sites to verify that there is no assumption of mutation?

I haven't checked at all, sorry :)

I only saw #1933 linked to a related bug on {data.table} and wanted to share how we're approaching removing {UN,}SET_S4_OBJECT; I'll defer to your expertise on how {vctrs} is used for how best to proceed. On {data.table} side, we do not generally work well with S4 objects anyway, so don't need to consider the implications as carefully, IIUC.