Apply suggestions from code review

Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
Co-authored-by: Davis Vaughan <davis@rstudio.com>

R/bind.R

@@ -84,7 +84,7 @@
 #'
 #' - [base::c()]
 #'
-#' If columns to combine inherit from a common class hierarchy,
+#' If columns to combine inherit from a common class,
 #' `vec_rbind()` falls back to `base::c()` if there exists a `c()`
 #' method implemented for this class hierarchy.
 #'

man/faq/developer/reference-compatibility.Rmd

@@ -10,7 +10,7 @@ All vctrs operations are based on four primitive generics described in the next
 
 - [vec_slice()] falls back to the base `[` generic if no [vec_proxy()] method is implemented. This way foreign classes  that do not implement [vec_restore()] can restore attributes based on the new subsetted contents.
 
-- [vec_c()] and [vec_rbind()] fall back to [base::c()] if the inputs have a common class hierarchy for which a `c()` method is implemented but no self-to-self [vec_ptype2()] method is implemented.
+- [vec_c()] and [vec_rbind()] now fall back to [base::c()] if the inputs have a common parent class with a `c()` method (only if they have no self-to-self `vec_ptype2()` method).
 
   vctrs works hard to make your `c()` method success in various situations (with `NULL` and `NA` inputs, even as first input which would normally prevent dispatch to your method). The main downside compared to using vctrs primitives is that you can't combine vectors of different classes since there is no extensible mechanism of coercion in `c()`, and it is less efficient in some cases.
 

src/c.c

@@ -173,7 +173,7 @@ static inline bool vec_implements_base_c(SEXP x);
 
 // [[ include("c.h") ]]
 bool needs_vec_c_fallback(SEXP ptype) {
-  if (!Rf_inherits(ptype, c_strs_vctrs_common_class_fallback)) {
+  if (!vec_is_common_class_fallback(ptype)) {
     return false;
   }
 

src/cast.c

@@ -183,7 +183,7 @@ SEXP vec_cast_dispatch_s3(const struct cast_opts* opts) {
   PROTECT(method);
 
   if (method == R_NilValue) {
-    SEXP out = vec_cast_default(x, to, x_arg_obj, to_arg_obj, &opts->fallback);
+    SEXP out = vec_cast_default(x, to, x_arg_obj, to_arg_obj, &(opts->fallback));
     UNPROTECT(3);
     return out;
   }

src/type-data-frame.c

@@ -437,7 +437,7 @@ SEXP df_ptype2_loop(const struct ptype2_opts* opts,
 // [[ register() ]]
 SEXP vctrs_df_cast_opts(SEXP x, SEXP to, SEXP opts, SEXP x_arg, SEXP to_arg) {
   struct vctrs_arg c_x_arg = vec_as_arg(x_arg);
-  struct vctrs_arg c_to_arg = vec_as_arg(to_arg);;
+  struct vctrs_arg c_to_arg = vec_as_arg(to_arg);
 
   const struct cast_opts c_opts = new_cast_opts(x, to, &c_x_arg, &c_to_arg, opts);
 

src/type2.c

@@ -158,7 +158,7 @@ SEXP vctrs_is_coercible(SEXP x,
                         SEXP x_arg,
                         SEXP y_arg) {
   struct vctrs_arg c_x_arg = vec_as_arg(x_arg);
-  struct vctrs_arg c_y_arg = vec_as_arg(y_arg);;
+  struct vctrs_arg c_y_arg = vec_as_arg(y_arg);
 
   const struct ptype2_opts c_opts = new_ptype2_opts(x, y, &c_x_arg, &c_y_arg, opts);
 

tests/testthat/test-bind.R

@@ -726,6 +726,8 @@ test_that("vec_rbind() falls back to c() if S3 method is available", {
   )
   expect_identical(out, data_frame(x = quux(c(1, 2))))
 
+  # Fallback is used with data frame subclasses, with or without
+  # ptype2 method
   foo_df <- foobaz(x_df)
   bar_df <- foobaz(y_df)
 
@@ -758,7 +760,7 @@ test_that("c() fallback works with unspecified columns", {
   skip("FIXME: c() fallback with unspecified columns")
 })
 
-test_that("vec_rbind() falls back to c() if S4 method is available", {
+test_that("vec_rbind() falls back to c() if S3 method is available for S4 class", {
   joe <- data_frame(x = .Counts(c(1L, 2L), name = "Joe"))
   jane <- data_frame(x = .Counts(3L, name = "Jane"))
 

tests/testthat/test-cast.R

@@ -182,7 +182,7 @@ test_that("vec_ptype_common() optionally falls back to base class", {
   expect_identical(common, list(x = x_df, y = y_df))
 })
 
-test_that("vec_ptype_common() collects common type", {
+test_that("vec_ptype_common_fallback() collects common type", {
   x <- foobar(1, foo = 1, class = c("quux", "baz"))
   y <- foobar(2, bar = 2, class = "baz")