Skip to content
Draft
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion R/class.R
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ new_object <- function(.parent, ...) {
stop(msg)
}

args <- list(...)
args <- list2(...)
nms <- names(args)

# TODO: Some type checking on `.parent`?
Expand Down
3 changes: 3 additions & 0 deletions R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,9 @@ modify_list <- function (x, new_vals) {
x
}

list2 <- function(...)
.Call(collect_dots_skip_missing_, environment(), substitute(list(...)))


# For older versions of R ----------------------------------------------------
deparse1 <- function(expr, collapse = " ", width.cutoff = 500L, ...) {
Expand Down
2 changes: 2 additions & 0 deletions src/init.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ extern SEXP S7_class_(SEXP, SEXP);
extern SEXP S7_object_(void);
extern SEXP prop_(SEXP, SEXP);
extern SEXP prop_set_(SEXP, SEXP, SEXP, SEXP);
extern SEXP collect_dots_skip_missing_(SEXP, SEXP);

static const R_CallMethodDef CallEntries[] = {
{"method_", (DL_FUNC) &method_, 4},
{"method_call_", (DL_FUNC) &method_call_, 3},
{"S7_object_", (DL_FUNC) &S7_object_, 0},
{"prop_", (DL_FUNC) &prop_, 2},
{"prop_set_", (DL_FUNC) &prop_set_, 4},
{"collect_dots_skip_missing_", (DL_FUNC) &collect_dots_skip_missing_, 2},
{NULL, NULL, 0}
};

Expand Down
56 changes: 56 additions & 0 deletions src/object.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#define R_NO_REMAP
#include <R.h>
#include <Rinternals.h>

SEXP collect_dots_skip_missing_(SEXP env, SEXP list_dddExprs_call) {
// This function is equivalent to `base::list(...)`, except it
// silently skips missing arguments. Ideally we could iterate
// over the DOTSXP list of promises directly, but there is currently
// no non-"non-API" way to do this. So we use `base::missing(..i)` to
// test for missingness, and use `substitute(list(...))` to get the
// promise expressions.
static SEXP missing_call = NULL;
if (missing_call == NULL) {
SEXP missing_fun = Rf_eval(Rf_install("missing"), R_BaseEnv);
missing_call = Rf_lang2(missing_fun, R_NilValue);
R_PreserveObject(missing_call);
}
// 14 = 2 for ".." + up to 10 digit number + '\0' + 1 extra for safety
static char ddi_buf[14] = "..";
static char *i_buf = ddi_buf + 2;
ddi_buf[13] = '\0'; // Technically not necessary, but just to be safe

PROTECT_INDEX pi;
PROTECT_WITH_INDEX(R_NilValue, &pi);

{
unsigned int i = 1;
SEXP prev_node = list_dddExprs_call;
SEXP ddExpr_node = CDR(list_dddExprs_call);
for (; ddExpr_node != R_NilValue; i++) {
snprintf(i_buf, sizeof(ddi_buf) - 2, "%u", i);
SEXP ddSym = Rf_install(ddi_buf);
SETCADR(missing_call, ddSym);
SEXP is_missing = Rf_eval(missing_call, env);
REPROTECT(is_missing, pi);

if (Rf_asLogical(is_missing)) {
ddExpr_node = CDR(ddExpr_node);
SETCDR(prev_node, ddExpr_node);
} else {
if (TAG(ddExpr_node) == R_NilValue) {
SEXP val_expr = CAR(ddExpr_node);
if (TYPEOF(val_expr) == SYMSXP) {
SET_TAG(ddExpr_node, val_expr);
}
}
SETCAR(ddExpr_node, ddSym);
prev_node = ddExpr_node;
ddExpr_node = CDR(ddExpr_node);
}
}
}

UNPROTECT(1); // is_missing
return Rf_eval(list_dddExprs_call, env);
}
20 changes: 20 additions & 0 deletions tests/testthat/_snaps/constructor.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,23 @@
new_object(foo(...), y = y)
<environment: 0x0>

# can create constructors with missing or lazy defaults

Code
Person()
Condition
Error:
! <Person> object properties are invalid:
- @first_name must be <character>, not <NULL>
- @last_name must be <MISSING> or <character>, not <NULL>
- @nick_name must be <character>, not <NULL>

---

Code
Person("Alice")
Condition
Error:
! <Person> object properties are invalid:
- @last_name must be <MISSING> or <character>, not <NULL>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit confusing because it is missing?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue happens because person@last_name <- <value> is never set since <value> is missing. Then, when the validator fetches person@last_name, it returns NULL (because attr(x, "does_not_exist") return NULL), which is the wrong type and causes the error.

This whole test probably needs to be rewritten to make sense with the new behavior.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S7/src/prop.c

Lines 260 to 261 in 15a01a3

// This is commented out because we currently have no way to distinguish between
// a prop with a value of NULL, and a prop value that is unset/missing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not set, should we be using quote(expr = )?

Copy link
Copy Markdown
Member Author

@t-kalinowski t-kalinowski Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To do that, we would have to use Rf_setAttrib(object, Rf_install("name"), R_missingArg) in prop<- when the object is first constructed.

quote(expr=) is such a pain to work with at the R level; I'm beginning to think that perhaps we should not create a pattern that requires all object instances to be handled with care, checking with missing() like this.

To make the "deprecate via getter+setter" pattern possible, maybe we should return to what we discussed in #396 (comment) and altogether bypass or not invoke custom setters on initial construction.

We would then need to provide a convenient way to "opt-in" to running the setters without requiring a full custom constructor.

Perhaps we add an argument to new_class(..., initializer = function(self) {}), a function authors can provide. The initializer function will be called with the output of the constructor after the constructor() has returned but before validator() is called. (somewhat analogous to __new__ and __init__ in Python.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may also want to add an initializer arg to new_property, defaulting to setter.

new_property(..., initializer = setter)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just use class_missing as the sentinel value? Or perhaps something similar but named like arg_missing?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we got a bit side-tracked into a much bigger topic than I was thinking. Looking at this with fresh eyes, I think all that I want is for the error message to be:

! <Person> object properties are invalid:
- @last_name must be <character>, not <NULL>


4 changes: 2 additions & 2 deletions tests/testthat/test-constructor.R
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ test_that("can create constructors with missing or lazy defaults", {
birthdate = Sys.Date()
))) # no age

expect_error(Person(), 'argument "first_name" is missing, with no default')
expect_error(Person("Alice"), 'argument "last_name" is missing, with no default')
expect_snapshot(Person(), error = TRUE)
expect_snapshot(Person("Alice"), error = TRUE)

p <- Person("Alice", ,"Smith")

Expand Down
38 changes: 38 additions & 0 deletions tests/testthat/test-utils.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
test_that("list2() works", {
# list2() is equivalent to base::list(), with the following differences:
# - A missing arg value is silently ignored instead of signaling an error.
# - An argument is automatically named if it is unnamed and the value expression is a symbol.

expect_identical(list2(), list())
expect_identical(list2(a = 1), list(a = 1))
expect_identical(list2(a = 1, b = ), list(a = 1))
expect_identical(list2(a = 1, b = , , ), list(a = 1))
expect_identical(list2(, a = 1, b = , , ), list(a = 1))
a <- 1
expect_identical(list2(a), list(a = 1))
expect_identical(list2(a, b = ), list(a = 1))
expect_identical(list2(a, b = , a, ), list(a = 1, a = 1))
expect_identical(list2(a = identity(a)), list(a = 1))

expect_identical(list2((a)), list(1))
expect_identical(list2(identity(a)), list(1))

# make sure all this works if values in `...` are nested promises
f1 <- function(...) list2(...)
f2 <- function(..., b) f1(..., b)
f3 <- function(..., c) f2(..., c)
f4 <- function(..., d) f3(..., d)

a <- 1; b <- 2
for (f in list(f1, f2, f3, f4, list2)) {
expect_identical(f(), list())
expect_mapequal(f(a = 1), list(a = 1))
expect_mapequal(f(a = 1, b =), list(a = 1))
expect_mapequal(f(a = 1, b = 2), list(a = 1, b = 2))
expect_mapequal(f(a, b), list(a = 1, b = 2))
}

expect_identical(list2(a, b, a + b), list(a = 1, b = 2, 3))
expect_identical(list2(a, b, c = a + b), list(a = 1, b = 2, c = 3))
expect_identical(list2((a), b, c = a + b), list( 1, b = 2, c = 3))
})