`modify` fails when `.f` returns NULL

[vspinu]

purrr::modify.default should check explicitly for null values

X <- list(a = 1, b = NULL, c = 3)
purrr:::modify(X, identity)
#> Error in .x[[i]]: subscript out of bounds

^{Created on 2020-03-18 by the reprex package (v0.3.0)}

Likely duplicates #655 and #746. Creating the new issue with simple reprex to create a sense of urgency :) If a simple check for NULL in modify.default is ok with you I can create a PR and add tests.

[vspinu]

@@ -136,7 +136,11 @@ modify.default <- function(.x, .f, ...) {
   .f <- as_mapper(.f, ...)
 
   for (i in seq_along(.x)) {
-    .x[[i]] <- .f(.x[[i]], ...)
+    el <- .f(.x[[i]], ...)
+    if (is.null(el))
+      .x[i] <- list(NULL)
+    else
+      .x[[i]] <- el
   }
 
   .x

[lionel-]

Perhaps use the same strategy as in https://github.com/r-lib/rlang/pull/947, and check if input is a list before using special treatment of NULL? A patch would be great!

[vspinu]

Isn't modify.default called only on recursives anyhow? So special treatment of lists doesn't make much sense here.

BTW, modify doesn't work on environments, might be good to add on this occasion.

[vspinu]

Ok, this is a bit more complex than I though. The only way to assign NULL to a list-like is to use [ ..] <- list(NULL) which I don't want to use as it will break with folks which overload [ like data.table.

Do you have an internal low level assignment in the package?

Also preserving NULL on data.frame is meaningless. Do you want to keep the same-length invariant for data.frames, or it would be ok to remove the column?

I guess the question is, do you want NULL from .f to eliminate the value or to insert NULL. If the latter then the modifier should break for classes for which it doesn't make sense (like data.frame).

[lionel-]

Isn't modify.default called only on recursives anyhow?

It will also be called on classed atomic vectors.

modify doesn't work on environments, might be good to add on this occasion.

Environments are collections but not vectors, so I don't think we should make it work.

The only way to assign NULL to a list-like is to use [ ..] <- list(NULL) which I don't want to use as it will break with folks which overload [ like data.table.

Dispatching on [ and [[ is the main idea. However you make a good point we don't want to use ] <- list(NULL) on all lists, e.g. not on data frames.

Do you have an internal low level assignment in the package?

This will be vctrs. It might be better to wait for the switch to vctrs actually, since there are tricky aspects of type here.

[vspinu]

Environments are collections but not vectors, so I don't think we should make it work.

Dare to elaborate? Why would type play a role for a generic modifier? If it makes sense, then just do it IMO.

It might be better to wait for the switch to vctrs actually, since there are tricky aspects of type here.

I have a simple solution for now. Will PR in a couple of minutes.