Simplify group context detection #6552
Conversation
| "dplyr:::error_incompatible_combine", | ||
| "dplyr:::mutate_mixed_null", | ||
| "dplyr:::mutate_constant_recycle_error", | ||
| "dplyr:::summarise_mixed_null", |
There was a problem hiding this comment.
These 4 error classes are still needed to dispatch off for things like error bullet generation, but we no longer need to special case them here
src/summarise.cpp
Outdated
| dplyr::stop_summarise_mixed_null(); | ||
| const SEXP* p_chunks = VECTOR_PTR_RO(chunks); | ||
|
|
||
| for (R_xlen_t i = 0; i < ngroups; i++) { | ||
| if (p_chunks[i] == R_NilValue) { | ||
| // Find out the first time the group was `NULL` | ||
| // so that the error will be associated with this group | ||
| DPLYR_MASK_SET_GROUP(i); | ||
| dplyr::stop_summarise_mixed_null(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the error path that occurs when you happen to return a NULL from one group and a non-NULL value in another group
In mutate() we already do something similar (report the group with the first problematic NULL), so this is just us being consistent with that now
| i In argument `..1 = (if_any())`. | ||
| i In group 1: `g = 1`. |
There was a problem hiding this comment.
Minor improvement
We used to always skip adding the group context if we saw a warning_across_missing_cols_deprecated warning class. But during the evaluation path (i.e. not expansion) we actually do have the group context, so we can report it
| @@ -164,6 +164,18 @@ | |||
| <error/dplyr:::mutate_error> | |||
| Error in `mutate()`: | |||
| i In argument: `..1 = if (a == 1) NULL else "foo"`. | |||
| i In group 1: `a = 1`. | |||
There was a problem hiding this comment.
Another small improvement
Again, we used to always skip adding the group context if mutate_mixed_null was the error class, but we actually do set the group value explicitly in the C++ code for this case, and we can now let that bubble up
| times_two <- function(x) x * 2 | ||
|
|
||
| # Expansion path | ||
| expect_snapshot(out <- mutate(df, z = across())) | ||
| expect_identical(out$z, df) | ||
| expect_snapshot(out <- mutate(gdf, z = across())) | ||
| expect_identical(out$z, df[c("x", "y")]) | ||
| expect_snapshot(out <- mutate(df, across(.fns = times_two))) |
There was a problem hiding this comment.
I realized this wasn't actually doing the "expansion path" of across() because it was a named call to across(), i.e. the z = , so I fixed that here. It ends up tweaking the warning in the snapshot test a little
| "dplyr:::error_incompatible_combine", | ||
| "dplyr:::mutate_mixed_null", | ||
| "dplyr:::mutate_constant_recycle_error", | ||
| "dplyr:::summarise_mixed_null", |
DavisVaughan
force-pushed
the
fix/group-context-errors
branch
2 times, most recently
from
cf27e06
to
ac57d3d
Compare
Previously we never actually used the expansion path because named across calls went through the evaluation path
It is `0L` before any group evaluation, and we now ensure that it is `0L` on the way out too, through `DPLYR_MASK_FINALISE()` This ensures that we can continue to use `$set_current_group()` on the fly right before an `abort()` call to force a group location
We already do this for `mutate()`, so this is just us being consistent
DavisVaughan
force-pushed
the
fix/group-context-errors
branch
from
ac57d3d
to
c79e5ac
Compare
Closes #6534
We now detect whether or not we need to add group context by checking if the
mask$get_current_group()value is0or not. If it is0, we are either before or after the actual computation of the group results. Otherwise we must be inside some kind of group computation, or we have hardcoded the group usingmask$set_current_group(), which we do in a few places.The most important change was probably resetting the group index to
0inDPLYR_MASK_FINALISE()so that it is0after the group computations too.This allowed me to remove the special error wrapping in
pick()and in theacross()deprecation.