WIP: Allow expressions in facet specs even when some variables are not available on some layers

[yutannihilation]

Fix #2963

Not all variables in a facet spec are available on all the layers. A solution suggested on r-lib/rlang#888 (comment) is:

bind the same symbols inside each panel, and when the symbols are undefined use an active binding to throw a typed error? And then you'd only catch these particular errors in the tryCatch()

This PR injects such an active bindings on the column names of all the plot data so that we can let eval_facet() fail gracefully.

Note that this PR removes env argument of eval_facet() to avoid confusion. I believe all facet specs are converted to quosures and bare expressions are not allowed, which means env is always ignored on eval_tidy().

devtools::load_all("~/repo/ggplot2/")
#> Loading ggplot2

# works fine with expressions
ggplot(mtcars, aes(mpg, cyl)) +
  geom_point() +
  geom_vline(xintercept = 20) +
  facet_wrap(vars(2 * am))

# works fine with external variables
two <- 2
ggplot(mtcars, aes(mpg, cyl)) +
  geom_point() +
  geom_vline(xintercept = 20) +
  facet_wrap(vars(two * am))

# raises an error when the expression refers to some non-existent variable
ggplot(mtcars, aes(mpg, cyl)) +
  geom_point() +
  geom_vline(xintercept = 20) +
  facet_wrap(vars(no_such_variable * am))
#> Error in eval_tidy(facet, data): object 'no_such_variable' not found

# special case: raises a friendlier error when the expression is a symbol
ggplot(mtcars, aes(mpg, cyl)) +
  geom_point() +
  geom_vline(xintercept = 20) +
  facet_wrap(vars(no_such_variable))
#> Error: At least one layer must contain all faceting variables: `no_such_variable`.
#> * Plot is missing `no_such_variable`
#> * Layer 1 is missing `no_such_variable`
#> * Layer 2 is missing `no_such_variable`

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

---

TODO:

• Add a NEWS bullet
• Consider using warn_for_aes_extract_usage() here as well
• Consider if it's really OK to add a param to Facet (I bet it's OK)

[yutannihilation]

Note that this cannot handle data pronoun at the moment. Probably I did wrong about environment ordering and I should define active bindings on the data mask directly. Will fix...

ggplot(mtcars, aes(mpg, cyl)) +
  geom_point() +
  geom_vline(xintercept = 20) +
  facet_wrap(vars(.data$am))
#> Error: Column `am` not found in `.data`

[yutannihilation]

Now .data works.

devtools::load_all("~/repo/ggplot2/")
#> Loading ggplot2

ggplot(mtcars, aes(mpg, cyl)) +
  geom_point() +
  geom_vline(xintercept = 20) +
  facet_wrap(vars(.data$am))

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

[yutannihilation]

I think this is basically ready for review but, since this is not the one we should have in v3.3.0, let's leave this for a while. Any comments are welcome.

[paleolimbot]

@hadley I know this is likely too big for the upcoming release, but this PR solves (very nicely!) the only qualm I have with recommending .data[[ and .data$ for facet specs: currently if a user does this, reference layers are not possible and there is no workaround except !!sym() (#2963).

With the new strip_dots() (#3746) it is possible to do a less comprehensive fix that solves the most common case of vars(.data$column). Right now broadcasting of reference layers occurs based on quo_is_symbol(facet), and broadcasting based on is_symbol(strip_dots(quo_get_expr(facet))) solves the issue. If that's of interest for the release, I'd be happy to make a PR (that should be overwritten by this, more comprehensive PR after the release).

[hadley]

This seems like an elegant approach to fixing the problem. What are the potential risks to including this in 3.3.0? It feels fairly low risk to me.

[yutannihilation]

Thanks, I just hesitated to add a new one to the milestones while the hard deadline is approaching, but I'd love to finish this PR if there's still some more time.

I don't have any big concern about this change because the most cases will be handled by this existing logic (though I'm not yet sure whether we should keep this or not):

  if (quo_is_symbol(facet)) {
    facet <- as.character(quo_get_expr(facet))

    if (facet %in% names(data)) {
      out <- data[[facet]]
    } else {
      out <- NULL
    }
    return(out)
  }

Note that this means we might still need is_symbol(strip_dots(quo_get_expr(facet))) to issue a consistent warning for the case of .data$no_such_columns as no_such_columns (see the details below).

Details

library(ggplot2)

p <- ggplot(mpg) + geom_bar(aes(cyl))

# friendly error
p + facet_wrap(vars(no_such_column))
#> Error: At least one layer must contain all faceting variables: `no_such_column`.
#> * Plot is missing `no_such_column`
#> * Layer 1 is missing `no_such_column`

# fine, but unfriendly error
p + facet_wrap(vars(.data$no_such_column))
#> Error: Column `no_such_column` not found in `.data`

# not sure if this should be allowed or not
no_such_column <- 1
p + facet_wrap(vars(no_such_column))
#> Error: At least one layer must contain all faceting variables: `no_such_column`.
#> * Plot is missing `no_such_column`
#> * Layer 1 is missing `no_such_column`

@clauswilke @thomasp85
What do you think about including this in 3.3.0?

[clauswilke]

I think it's somewhat risky to introduce this kind of change this late in the release process, even if we're pretty sure it'll be fine. A safe approach would be to add the patch to a branch of the release branch, run a revdep check on that, and only merge into the release branch if no new regressions are found.

[hadley]

Let me give this a thorough review, and I'll contemplate where this might change existing behaviour. I think if it's low risk we can merge, and then re-run the revdeps; if that reveals any problems we can back out and try again for the next version.

[paleolimbot]

I think this PR is safer than what I was proposing, since it keeps the exact same code for the case of a symbol.

[hadley]

@yutannihilation I made a couple of tiny style changes, but otherwise this is lovely

@clauswilke my analysis suggests that this change should only affect a small set of plots that previously errored. I suggest we merge, and then re-run revdepchecks just to be safe.

@thomasp85 does that sound ok with you? I would like to get this into this release so we can over a comprehensive approach to tidyeval that works the same way in ggplot2 and dplyr (1.0.0)

[thomasp85]

This looks good. I’m fine with trying a merge into the rc branch

[yutannihilation]

@hadley
Thanks for your review and improvement!

@thomasp85
I created #3757, a new PR for the rc branch. If you are already working on merging this into master, please feel free to close this.

[clauswilke]

@hadley Sounds good.

[hadley]

I forgot that we can't easily modify this PR to merge on the RC, so lets close this PR. Further discussion can happen on #3757