Skip to content

Conversation

@teunbrand
Copy link
Collaborator

This PR aims to fix #5670.

Briefly, this PR replaces a tryCatch() with a try_fetch(). try_fetch() is better at catching errors that are inherited.

When facet expressions include a function that throws wrapped errors, tryCatch() fails to 'see' the special ggplot2_missing_facet_var error ggplot2 throws in case of missing columns here:

ggplot2/R/facet-.R

Lines 470 to 483 in a4be39d

env <- new_environment(data)
missing_columns <- setdiff(possible_columns, names(data))
undefined_error <- function(e) cli::cli_abort("", class = "ggplot2_missing_facet_var")
bindings <- rep_named(missing_columns, list(undefined_error))
env_bind_active(env, !!!bindings)
# Create a data mask and install a data pronoun manually (see ?new_data_mask)
mask <- new_data_mask(env)
mask$.data <- as_data_pronoun(mask)
tryCatch(
eval_tidy(facet, mask),
ggplot2_missing_facet_var = function(e) NULL
)

We can demonstrate this behaviour like so:

library(ggplot2)

wrapper <- function(x) {
  withCallingHandlers(
    x, error = function(cnd) rlang::abort("Custom error", parent = cnd)
  )
}

ggplot(mpg) +
  geom_point(aes(displ, hwy)) +
  geom_blank(data = data.frame()) + # does not have 'year'
  facet_wrap(~ wrapper(year))
#> Error in `wrapper()`:
#> ! Custom error

Created on 2024-01-31 with reprex v2.1.0

However, try_fetch() is 'aware' that the error is inherited. This restores the correct behaviour of missing columns:

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

wrapper <- function(x) {
  withCallingHandlers(
    x, error = function(cnd) rlang::abort("Custom error", parent = cnd)
  )
}

ggplot(mpg) +
  geom_point(aes(displ, hwy)) +
  geom_blank(data = data.frame()) + # does not have 'year'
  facet_wrap(~ wrapper(year))

Created on 2024-01-31 with reprex v2.1.0

@teunbrand teunbrand requested a review from thomasp85 February 23, 2024 10:06
Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I was sure we had weeded out the remaining tryCatch() calls

@teunbrand teunbrand merged commit cf43adc into tidyverse:main Feb 27, 2024
@teunbrand teunbrand deleted the facet_try_fetch branch February 27, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error: ! Failed to evaluate glue component

2 participants