diff --git a/.github/workflows/rchk.yml b/.github/workflows/rchk.yml new file mode 100644 index 00000000..c5a2c898 --- /dev/null +++ b/.github/workflows/rchk.yml @@ -0,0 +1,38 @@ +name: rchk + +on: + workflow_dispatch: + + +jobs: + rchk: + runs-on: ubuntu-latest + steps: + + - uses: actions/checkout@v4 + + - uses: r-lib/actions/setup-pandoc@v2 + - uses: r-lib/actions/setup-r@v2 + - uses: r-lib/actions/setup-r-dependencies@v2 + + - run: R CMD build . + + - run: docker pull kalibera/rchk:latest + + - name: run rchk + run: | + pkgtar=$(ls S7_*.tar.gz) + mkdir -p rchk/packages + mv $pkgtar rchk/packages/ + cd rchk + docker run -v `pwd`/packages:/rchk/packages kalibera/rchk:latest /rchk/packages/$pkgtar > rchk.log 2>&1 + cat rchk.log + + - run: cat rchk.log + working-directory: rchk + + - name: upload rchk log + uses: actions/upload-artifact@v4 + with: + name: rchk-log + path: rchk/rchk.log diff --git a/R/method-dispatch.R b/R/method-dispatch.R index d612d399..a52e4ec1 100644 --- a/R/method-dispatch.R +++ b/R/method-dispatch.R @@ -20,6 +20,5 @@ method_lookup_error_message <- function(name, types) { #' @order 2 #' @export S7_dispatch <- function() { - S7_dispatched_call <- .Call(method_call_, sys.call(-1), sys.function(-1), sys.frame(-1)) - eval(S7_dispatched_call, envir = sys.frame(-1)) + .External2(method_call_, sys.function(-1L), sys.frame(-1L)) } diff --git a/src/init.c b/src/init.c index 63943bbc..796d4f49 100644 --- a/src/init.c +++ b/src/init.c @@ -5,18 +5,25 @@ /* .Call calls */ extern SEXP method_(SEXP, SEXP, SEXP, SEXP); -extern SEXP method_call_(SEXP, SEXP, SEXP); +extern SEXP method_call_(SEXP, SEXP, SEXP, SEXP); +extern SEXP test_call_(SEXP, SEXP, SEXP, SEXP); extern SEXP S7_class_(SEXP, SEXP); extern SEXP S7_object_(void); extern SEXP prop_(SEXP, SEXP); extern SEXP prop_set_(SEXP, SEXP, SEXP, SEXP); +#define CALLDEF(name, n) {#name, (DL_FUNC) &name, n} + 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}, + CALLDEF(method_, 4), + CALLDEF(S7_object_, 0), + CALLDEF(prop_, 2), + CALLDEF(prop_set_, 4), + {NULL, NULL, 0} +}; + +static const R_ExternalMethodDef ExternalEntries[] = { + CALLDEF(method_call_, 2), {NULL, NULL, 0} }; @@ -35,19 +42,25 @@ SEXP sym_getter; SEXP sym_dot_should_validate; SEXP sym_dot_getting_prop; SEXP sym_dot_setting_prop; +SEXP sym_obj_dispatch; +SEXP sym_dispatch_args; +SEXP sym_methods; SEXP fn_base_quote; +SEXP fn_base_missing; SEXP ns_S7; +SEXP R_TRUE, R_FALSE; + void R_init_S7(DllInfo *dll) { - R_registerRoutines(dll, NULL, CallEntries, NULL, NULL); + R_registerRoutines(dll, NULL, CallEntries, NULL, ExternalEntries); R_useDynamicSymbols(dll, FALSE); + sym_ANY = Rf_install("ANY"); sym_S7_class = Rf_install("S7_class"); - sym_name = Rf_install("name"); sym_parent = Rf_install("parent"); sym_package = Rf_install("package"); @@ -59,8 +72,14 @@ void R_init_S7(DllInfo *dll) sym_dot_should_validate = Rf_install(".should_validate"); sym_dot_getting_prop = Rf_install(".getting_prop"); sym_dot_setting_prop = Rf_install(".setting_prop"); + sym_obj_dispatch = Rf_install("obj_dispatch"); + sym_dispatch_args = Rf_install("dispatch_args"); + sym_methods = Rf_install("methods"); fn_base_quote = Rf_eval(Rf_install("quote"), R_BaseEnv); + fn_base_missing = Rf_eval(Rf_install("missing"), R_BaseEnv); - ns_S7 = Rf_findVarInFrame(R_NamespaceRegistry, Rf_install("S7")); + ns_S7 = Rf_eval(Rf_install("S7"), R_NamespaceRegistry); + R_PreserveObject(R_TRUE = Rf_ScalarLogical(1)); + R_PreserveObject(R_FALSE = Rf_ScalarLogical(0)); } diff --git a/src/method-dispatch.c b/src/method-dispatch.c index 9334a01e..f026ba83 100644 --- a/src/method-dispatch.c +++ b/src/method-dispatch.c @@ -4,6 +4,38 @@ extern SEXP parent_sym; extern SEXP sym_ANY; +extern SEXP ns_S7; +extern SEXP sym_obj_dispatch; +extern SEXP sym_dispatch_args; +extern SEXP sym_methods; +extern SEXP fn_base_quote; +extern SEXP fn_base_missing; + +extern SEXP R_TRUE; + + +static inline +void APPEND_NODE(SEXP node, SEXP tag, SEXP val) { + SEXP new_node = Rf_cons(val, R_NilValue); + SETCDR(node, new_node); + SET_TAG(new_node, tag); +} + +// extern Rboolean is_S7_object(SEXP); +// extern Rboolean is_s7_class(SEXP); +// extern void check_is_S7(SEXP object); + + +static inline +SEXP maybe_enquote(SEXP x) { + switch (TYPEOF(x)) { + case SYMSXP: + case LANGSXP: + return Rf_lang2(fn_base_quote, x); + default: + return x; + } +} // Recursively walk through method table to perform iterated dispatch SEXP method_rec(SEXP table, SEXP signature, R_xlen_t signature_itr) { @@ -17,7 +49,9 @@ SEXP method_rec(SEXP table, SEXP signature, R_xlen_t signature_itr) { SEXP klass = Rf_install(CHAR(STRING_ELT(classes, i))); SEXP val = Rf_findVarInFrame(table, klass); if (TYPEOF(val) == ENVSXP) { + PROTECT(val); // no really necessary, but rchk flags spuriously val = method_rec(val, signature, signature_itr + 1); + UNPROTECT(1); } if (TYPEOF(val) == CLOSXP) { return val; @@ -27,7 +61,9 @@ SEXP method_rec(SEXP table, SEXP signature, R_xlen_t signature_itr) { // ANY fallback SEXP val = Rf_findVarInFrame(table, sym_ANY); if (TYPEOF(val) == ENVSXP) { + PROTECT(val); val = method_rec(val, signature, signature_itr + 1); + UNPROTECT(1); } if (TYPEOF(val) == CLOSXP) { return val; @@ -37,48 +73,53 @@ SEXP method_rec(SEXP table, SEXP signature, R_xlen_t signature_itr) { } SEXP generic_args(SEXP generic, SEXP envir) { + // This function is only used to generate an informative message when + // signalling an S7_method_lookup_error, so it doesn't need to be maximally efficient. + // How many arguments are used for dispatch? - SEXP dispatch_args = Rf_getAttrib(generic, Rf_install("dispatch_args")); + SEXP dispatch_args = Rf_getAttrib(generic, sym_dispatch_args); R_xlen_t n_dispatch = Rf_xlength(dispatch_args); // Allocate a list to store the arguments SEXP args = PROTECT(Rf_allocVector(VECSXP, n_dispatch)); + SEXP missing_call = PROTECT(Rf_lang2(fn_base_missing, R_NilValue)); + PROTECT_INDEX pi; + PROTECT_WITH_INDEX(R_NilValue, &pi); + // Find the value of each argument. SEXP formals = FORMALS(generic); for (R_xlen_t i = 0; i < n_dispatch; ++i) { SEXP name = TAG(formals); - SEXP arg = Rf_findVar(name, envir); - if (PRCODE(arg) == R_MissingArg) { + SETCADR(missing_call, name); + SEXP is_missing = Rf_eval(missing_call, envir); + REPROTECT(is_missing, pi); + + if (Rf_asLogical(is_missing)) { SET_VECTOR_ELT(args, i, R_MissingArg); } else { // method_call_() has already done the necessary computation - SET_VECTOR_ELT(args, i, Rf_eval(arg, R_EmptyEnv)); + SET_VECTOR_ELT(args, i, Rf_eval(name, envir)); } formals = CDR(formals); } Rf_setAttrib(args, R_NamesSymbol, dispatch_args); - UNPROTECT(1); + UNPROTECT(3); return args; } __attribute__ ((noreturn)) void S7_method_lookup_error(SEXP generic, SEXP envir) { - SEXP ns = Rf_findVarInFrame(R_NamespaceRegistry, Rf_install("S7")); - static SEXP S7_method_lookup_error_fun = NULL; - if (S7_method_lookup_error_fun == NULL) { - S7_method_lookup_error_fun = Rf_findVarInFrame(ns, Rf_install("method_lookup_error")); - } SEXP name = Rf_getAttrib(generic, R_NameSymbol); SEXP args = generic_args(generic, envir); - SEXP S7_method_lookup_error_call = PROTECT(Rf_lang3(S7_method_lookup_error_fun, name, args)); - Rf_eval(S7_method_lookup_error_call, ns); + SEXP S7_method_lookup_error_call = PROTECT(Rf_lang3(Rf_install("method_lookup_error"), name, args)); + Rf_eval(S7_method_lookup_error_call, ns_S7); while(1); } @@ -88,31 +129,25 @@ SEXP method_(SEXP generic, SEXP signature, SEXP envir, SEXP error_) { return R_NilValue; } - SEXP table = Rf_getAttrib(generic, Rf_install("methods")); + SEXP table = Rf_getAttrib(generic, sym_methods); if (TYPEOF(table) != ENVSXP) { Rf_error("Corrupt S7_generic: @methods isn't an environment"); } SEXP m = method_rec(table, signature, 0); - int error = Rf_asInteger(error_); - if (error && m == R_NilValue) { + if (m == R_NilValue && Rf_asLogical(error_)) { S7_method_lookup_error(generic, envir); } return m; } -SEXP S7_obj_dispatch(SEXP object) { - SEXP ns = Rf_findVarInFrame(R_NamespaceRegistry, Rf_install("S7")); - static SEXP obj_dispatch_fun = NULL; - if (obj_dispatch_fun == NULL) { - obj_dispatch_fun = Rf_findVarInFrame(ns, Rf_install("obj_dispatch")); - } +SEXP S7_obj_dispatch(SEXP object) { - SEXP obj_dispatch_call = PROTECT(Rf_lang2(obj_dispatch_fun, object)); - SEXP res = Rf_eval(obj_dispatch_call, ns); + SEXP obj_dispatch_call = PROTECT(Rf_lang2(sym_obj_dispatch, maybe_enquote(object))); + SEXP res = Rf_eval(obj_dispatch_call, ns_S7); UNPROTECT(1); return res; @@ -126,67 +161,87 @@ SEXP S7_object_(void) { return obj; } -SEXP method_call_(SEXP call, SEXP generic, SEXP envir) { - int n_protect = 0; +SEXP method_call_(SEXP call_, SEXP op_, SEXP args_, SEXP env_) { + args_ = CDR(args_); + SEXP generic = CAR(args_); args_ = CDR(args_); + SEXP envir = CAR(args_); args_ = CDR(args_); // Get the number of arguments to the generic SEXP formals = FORMALS(generic); R_xlen_t n_args = Rf_xlength(formals); // And how many are used for dispatch - SEXP dispatch_args = Rf_getAttrib(generic, Rf_install("dispatch_args")); + SEXP dispatch_args = Rf_getAttrib(generic, sym_dispatch_args); R_xlen_t n_dispatch = Rf_xlength(dispatch_args); // Allocate a list to store the classes for the arguments SEXP dispatch_classes = PROTECT(Rf_allocVector(VECSXP, n_dispatch)); - ++n_protect; // Allocate a pairlist to hold the arguments for when we call the method SEXP mcall = PROTECT(Rf_lcons(R_NilValue, R_NilValue)); - ++n_protect; SEXP mcall_tail = mcall; + PROTECT_INDEX arg_pi, val_pi; + PROTECT_WITH_INDEX(R_NilValue, &arg_pi); + PROTECT_WITH_INDEX(R_NilValue, &val_pi); + // For each of the arguments to the generic for (R_xlen_t i = 0; i < n_args; ++i) { - // Find its name and look up its value (a promise) SEXP name = TAG(formals); - SEXP arg = Rf_findVar(name, envir); if (i < n_dispatch) { - if (PRCODE(arg) != R_MissingArg) { - // Evaluate the original promise so we can look up its class - SEXP val = PROTECT(Rf_eval(arg, R_EmptyEnv)); - if (!Rf_inherits(val, "S7_super")) { + SEXP arg = Rf_findVarInFrame(envir, name); + if (arg == R_MissingArg) { + + APPEND_NODE(mcall_tail, name, arg); + SET_VECTOR_ELT(dispatch_classes, i, Rf_mkString("MISSING")); + + } else { // arg not missing, is a PROMSXP + + // Force the promise so we can look up its class. + // However, we preserve and pass along the promise itself so that + // methods can still call substitute() + // Instead of Rf_eval(arg, R_EmptyEnv), we do Rf_eval(name, envir), so that + // - if TYPEOF(arg) == LANGSXP or SYMSXP, arg doesn't need to be enquoted and + // - if TYPEOF(arg) == PROMSXP, arg is updated in place. + REPROTECT(arg, arg_pi); // not really necessary, but rchk flags spuriously + SEXP val = Rf_eval(name, envir); + REPROTECT(val, val_pi); + + if (Rf_inherits(val, "S7_super")) { + - // If it's a promise, update the value of the promise to avoid - // evaluating it again in the method body - if (TYPEOF(val) == PROMSXP) { - SET_PRVALUE(arg, val); - } + // Put the super() stored value into the method call. + // Note: This means we don't pass along the arg PROMSXP, meaning that + // substitute() in methods does not retrieve the `super()` call. + // If we wanted substitute() to work here too, we could do: + // if (TYPEOF(arg) == PROMSXP) { SET_PRVALUE(arg, true_val); } else { arg = true_val; } + SEXP arg = VECTOR_ELT(val, 0); // true_val used for dispatch + APPEND_NODE(mcall_tail, name, arg); - // Then add to arguments of method call - SETCDR(mcall_tail, Rf_cons(arg, R_NilValue)); + // Put the super() stored class dispatch vector into dispatch_classes + SET_VECTOR_ELT(dispatch_classes, i, VECTOR_ELT(val, 1)); + + } else { // val is not a S7_super, a regular value + + // The PROMSXP arg will have been updated in place by Rf_eval() above. + // Add to arguments of method call + APPEND_NODE(mcall_tail, name, arg); // Determine class string to use for method look up SET_VECTOR_ELT(dispatch_classes, i, S7_obj_dispatch(val)); - } else { - // If it's a superclass, we get the stored value and dispatch class - SEXP true_val = VECTOR_ELT(val, 0); - SET_PRVALUE(arg, true_val); - SETCDR(mcall_tail, Rf_cons(arg, R_NilValue)); - SET_VECTOR_ELT(dispatch_classes, i, VECTOR_ELT(val, 1)); } - UNPROTECT(1); - } else { - SETCDR(mcall_tail, Rf_cons(name, R_NilValue)); - SET_VECTOR_ELT(dispatch_classes, i, Rf_mkString("MISSING")); } } else { // other arguments not used for dispatch - SEXP arg_wrap = Rf_cons(name, R_NilValue); - SET_TAG(arg_wrap, name); - SETCDR(mcall_tail, arg_wrap); + if (name == R_DotsSymbol) { + SETCDR(mcall_tail, Rf_cons(R_DotsSymbol, R_NilValue)); + } else { + // pass along the promise so substitute() works + SEXP arg = Rf_findVarInFrame(envir, name); + APPEND_NODE(mcall_tail, name, arg); + } } mcall_tail = CDR(mcall_tail); @@ -194,9 +249,10 @@ SEXP method_call_(SEXP call, SEXP generic, SEXP envir) { } // Now that we have all the classes, we can look up what method to call - SEXP m = method_(generic, dispatch_classes, envir, Rf_ScalarLogical(1)); + SEXP m = method_(generic, dispatch_classes, envir, R_TRUE); SETCAR(mcall, m); - UNPROTECT(n_protect); - return mcall; + SEXP out = Rf_eval(mcall, envir); + UNPROTECT(4); + return out; } diff --git a/tests/testthat/_snaps/R-lt-4-3/method-dispatch.md b/tests/testthat/_snaps/R-lt-4-3/method-dispatch.md new file mode 100644 index 00000000..cafe44c1 --- /dev/null +++ b/tests/testthat/_snaps/R-lt-4-3/method-dispatch.md @@ -0,0 +1,8 @@ +# method dispatch works for class_missing + + Code + foo_wrapper() + Condition + Error in `S7::S7_dispatch()`: + ! argument "xx" is missing, with no default + diff --git a/tests/testthat/_snaps/method-dispatch.md b/tests/testthat/_snaps/method-dispatch.md index a90016a0..da9ea870 100644 --- a/tests/testthat/_snaps/method-dispatch.md +++ b/tests/testthat/_snaps/method-dispatch.md @@ -49,3 +49,11 @@ - x: - y: +# method dispatch works for class_missing + + Code + foo_wrapper() + Condition + Error in `foo_wrapper()`: + ! argument "xx" is missing, with no default + diff --git a/tests/testthat/test-method-dispatch.R b/tests/testthat/test-method-dispatch.R index dd4160f0..01a1afe0 100644 --- a/tests/testthat/test-method-dispatch.R +++ b/tests/testthat/test-method-dispatch.R @@ -72,9 +72,32 @@ test_that("can substitute() args", { ) expect_equal(foo("x", y = letters), quote(letters)) - # Doesn't work currently - # method(foo, class_character) <- function(x, ..., z = 1) substitute(z) - # expect_equal(foo("x", z = letters), quote(letters)) + suppressMessages( + method(foo, class_character) <- function(x, ..., z = 1) substitute(z) + ) + expect_equal(foo("x", z = letters), quote(letters)) + + suppressMessages( + method(foo, class_character) <- function(x, ..., z = 1) substitute(list(...)) + ) + expect_equal(foo("x", abc = xyz), quote(list(abc = xyz))) + + suppressMessages( + method(foo, class_character) <- function(x, ..., z = 1, y) missing(y) + ) + expect_true(foo("x"), TRUE) + expect_true(foo("x", y =), TRUE) + expect_true(foo("x", y =), TRUE) + + suppressMessages( + method(foo, class_character) <- function(x, ..., z = 1, y) ...length() + ) + + expect_equal(foo("x"), 0) + expect_equal(foo("x", y =), 0) + expect_equal(foo("x", y =, abc), 1) + expect_equal(foo("x", y =, abc = xyz), 1) + expect_equal(foo("x", y =, abc, xyz), 2) }) test_that("methods get values modified in the generic", { @@ -186,3 +209,21 @@ test_that("can dispatch on evaluated arguments", { method(my_generic, class_numeric) <- function(x) 100 expect_equal(my_generic("x"), 100) }) + + +test_that("method dispatch works for class_missing", { + + foo <- new_generic("foo", "x") + method(foo, class_missing) <- function(x) missing(x) + + expect_true(foo()) + + # dispatch on class_missing only works directly in the generic call + foo_wrapper <- function(xx) foo(xx) + expect_snapshot( + error = TRUE, + variant = if (getRversion() < "4.3") "R-lt-4-3", + foo_wrapper() + ) + +})