From a4baded2f2b37a6655fa7868e545db109cd88723 Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Sat, 20 Aug 2022 15:42:53 +0900 Subject: [PATCH 01/10] Warn on the use of `..var..` and `stat(var)` --- NEWS.md | 3 +++ R/aes-evaluation.r | 30 ++++++++++++++++++++++++------ R/layer.r | 2 +- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/NEWS.md b/NEWS.md index d3d9ceb105..37b66e2c67 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # ggplot2 (development version) +* Warn on the use of the dot-dot notation (`..var..`) and `stat()`, which have + been superseded by `after_stat()` (@yutannihilation, #3693). + * Fix a bug in `position_jitter()` where infinity values were dropped (@javlon, #4790). diff --git a/R/aes-evaluation.r b/R/aes-evaluation.r index 228d42f16f..20af6d64f0 100644 --- a/R/aes-evaluation.r +++ b/R/aes-evaluation.r @@ -92,9 +92,20 @@ is_dotted_var <- function(x) { grepl(match_calculated_aes, x) } +# TODO: if we want to set the frequency "always", we probably need some +# mechanism to generate a frequency ID that is unique per plot build. Otherwise, +# users might face too many warnings. +warn_old_calculated_aes <- function() { + # TODO: move to cli_warn() + cli::cli_inform(c( + "The dot-dot notation ({.var ..var..}) and {.fun stat} have been superseded", + "i" = "To access calculated variables, please use {.fun after_stat} instead" + ), .frequency = "regularly", .frequency_id = "ggplot2-old-calculated-aes") +} + # Determine if aesthetic is calculated -is_calculated_aes <- function(aesthetics) { - vapply(aesthetics, is_calculated, logical(1), USE.NAMES = FALSE) +is_calculated_aes <- function(aesthetics, warn = FALSE) { + vapply(aesthetics, is_calculated, warn = warn, logical(1), USE.NAMES = FALSE) } is_scaled_aes <- function(aesthetics) { vapply(aesthetics, is_scaled, logical(1), USE.NAMES = FALSE) @@ -102,7 +113,7 @@ is_scaled_aes <- function(aesthetics) { is_staged_aes <- function(aesthetics) { vapply(aesthetics, is_staged, logical(1), USE.NAMES = FALSE) } -is_calculated <- function(x) { +is_calculated <- function(x, warn = FALSE) { if (is_call(get_expr(x), "after_stat")) { return(TRUE) } @@ -110,14 +121,21 @@ is_calculated <- function(x) { if (is.atomic(x)) { FALSE } else if (is.symbol(x)) { - is_dotted_var(as.character(x)) + res <- is_dotted_var(as.character(x)) + if (res && warn) { + warn_old_calculated_aes() + } + res } else if (is_quosure(x)) { - is_calculated(quo_get_expr(x)) + is_calculated(quo_get_expr(x), warn = warn) } else if (is.call(x)) { if (identical(x[[1]], quote(stat))) { + if (warn) { + warn_old_calculated_aes() + } TRUE } else { - any(vapply(x, is_calculated, logical(1))) + any(vapply(x, is_calculated, warn = warn, logical(1))) } } else if (is.pairlist(x)) { FALSE diff --git a/R/layer.r b/R/layer.r index 1b043fece2..8c79463d78 100644 --- a/R/layer.r +++ b/R/layer.r @@ -267,7 +267,7 @@ Layer <- ggproto("Layer", NULL, # Drop aesthetics that are set or calculated set <- names(aesthetics) %in% names(self$aes_params) - calculated <- is_calculated_aes(aesthetics) + calculated <- is_calculated_aes(aesthetics, warn = TRUE) modifiers <- is_scaled_aes(aesthetics) aesthetics <- aesthetics[!set & !calculated & !modifiers] From 6d1ad31cb93645ba8f9245df487dd13e96d0a747 Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Sat, 20 Aug 2022 15:53:53 +0900 Subject: [PATCH 02/10] Tweak --- R/aes-evaluation.r | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/aes-evaluation.r b/R/aes-evaluation.r index 20af6d64f0..359234acfe 100644 --- a/R/aes-evaluation.r +++ b/R/aes-evaluation.r @@ -96,9 +96,9 @@ is_dotted_var <- function(x) { # mechanism to generate a frequency ID that is unique per plot build. Otherwise, # users might face too many warnings. warn_old_calculated_aes <- function() { - # TODO: move to cli_warn() + # TODO: move to cli_warn()? cli::cli_inform(c( - "The dot-dot notation ({.var ..var..}) and {.fun stat} have been superseded", + "The dot-dot notation ({.var ..var..}) and {.fun stat} have been superseded as of ggplot2 3.3.0", "i" = "To access calculated variables, please use {.fun after_stat} instead" ), .frequency = "regularly", .frequency_id = "ggplot2-old-calculated-aes") } From 926dd9e00de2cb3096fa8ea9d4588581bc1b4100 Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Wed, 24 Aug 2022 18:53:29 +0900 Subject: [PATCH 03/10] Tweak --- NEWS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index c52117d02a..1055701665 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,7 +1,7 @@ # ggplot2 (development version) -* Warn on the use of the dot-dot notation (`..var..`) and `stat()`, which have - been superseded by `after_stat()` (@yutannihilation, #3693). +* The dot-dot notation (`..var..`) and `stat()`, which have been superseded by + `after_stat()`, are now formally deprecated (@yutannihilation, #3693). * `geom_col()` and `geom_bar()` gain a new `just` argument. This is set to `0.5` by default; use `just = 0`/`just = 1` to place columns on the left/right From 2d4c62d90e32dfd8500067f42b5c9c0b681b5403 Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Wed, 24 Aug 2022 19:07:48 +0900 Subject: [PATCH 04/10] Use the dev verion of lifecycle --- DESCRIPTION | 4 +++- R/aes-evaluation.r | 19 ++++++------------- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 407b190ada..5cdac2974c 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -36,7 +36,7 @@ Imports: grid, gtable (>= 0.1.1), isoband, - lifecycle, + lifecycle (> 1.0.1), MASS, mgcv, rlang (>= 1.0.0), @@ -71,6 +71,8 @@ Suggests: testthat (>= 3.1.2), vdiffr (>= 1.0.0), xml2 +Remotes: + r-lib/lifecycle Enhances: sp VignetteBuilder: diff --git a/R/aes-evaluation.r b/R/aes-evaluation.r index 359234acfe..af935325fa 100644 --- a/R/aes-evaluation.r +++ b/R/aes-evaluation.r @@ -92,17 +92,6 @@ is_dotted_var <- function(x) { grepl(match_calculated_aes, x) } -# TODO: if we want to set the frequency "always", we probably need some -# mechanism to generate a frequency ID that is unique per plot build. Otherwise, -# users might face too many warnings. -warn_old_calculated_aes <- function() { - # TODO: move to cli_warn()? - cli::cli_inform(c( - "The dot-dot notation ({.var ..var..}) and {.fun stat} have been superseded as of ggplot2 3.3.0", - "i" = "To access calculated variables, please use {.fun after_stat} instead" - ), .frequency = "regularly", .frequency_id = "ggplot2-old-calculated-aes") -} - # Determine if aesthetic is calculated is_calculated_aes <- function(aesthetics, warn = FALSE) { vapply(aesthetics, is_calculated, warn = warn, logical(1), USE.NAMES = FALSE) @@ -123,7 +112,9 @@ is_calculated <- function(x, warn = FALSE) { } else if (is.symbol(x)) { res <- is_dotted_var(as.character(x)) if (res && warn) { - warn_old_calculated_aes() + # TODO: if we want to set always = TRUE, we probably need some mechanism + # to limit the warning only once per plot build + lifecycle::deprecate_warn("3.4.0", I("The dot-dot notation (`..var..`)"), "after_stat()") } res } else if (is_quosure(x)) { @@ -131,7 +122,9 @@ is_calculated <- function(x, warn = FALSE) { } else if (is.call(x)) { if (identical(x[[1]], quote(stat))) { if (warn) { - warn_old_calculated_aes() + # TODO: if we want to set always = TRUE, we probably need some mechanism + # to limit the warning only once per plot build + lifecycle::deprecate_warn("3.4.0", "stat()", "after_stat()") } TRUE } else { From a6ecabeb5f5a59d5890aaabbf5c413853ed0c3ce Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Wed, 24 Aug 2022 19:20:16 +0900 Subject: [PATCH 05/10] use lifecycle::deprecate_warn() for size aesthetic for lines --- R/layer.r | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/R/layer.r b/R/layer.r index 8c79463d78..c14fb32cb0 100644 --- a/R/layer.r +++ b/R/layer.r @@ -129,11 +129,7 @@ layer <- function(geom = NULL, stat = NULL, if (geom$rename_size && "size" %in% extra_param && !"linewidth" %in% mapped_aesthetics(mapping)) { aes_params <- c(aes_params, params["size"]) extra_param <- setdiff(extra_param, "size") - # TODO: move to cli_warn() - cli::cli_inform(c( - "{.field size} aesthetic has been deprecated for use with lines as of ggplot2 3.4.0", - "i" = "Please use {.field linewidth} aesthetic instead" - ), .frequency = "regularly", .frequency_id = "ggplot-size-linewidth") + lifecycle::deprecate_warn("3.4.0", I("Using `size` aesthetic for lines"), I("`linewidth`")) } if (check.param && length(extra_param) > 0) { cli::cli_warn("Ignoring unknown parameters: {.arg {extra_param}}", call = call_env) @@ -146,11 +142,7 @@ layer <- function(geom = NULL, stat = NULL, # Take care of size->linewidth aes renaming if (geom$rename_size && "size" %in% extra_aes && !"linewidth" %in% mapped_aesthetics(mapping)) { extra_aes <- setdiff(extra_aes, "size") - # TODO: move to cli_warn() - cli::cli_inform(c( - "{.field size} aesthetic has been deprecated for use with lines as of ggplot2 3.4.0", - "i" = "Please use {.field linewidth} aesthetic instead" - ), .frequency = "regularly", .frequency_id = "ggplot-size-linewidth") + lifecycle::deprecate_warn("3.4.0", I("Using `size` aesthetic for lines"), I("`linewidth`")) } if (check.aes && length(extra_aes) > 0) { cli::cli_warn("Ignoring unknown aesthetics: {.field {extra_aes}}", call = call_env) @@ -247,11 +239,7 @@ Layer <- ggproto("Layer", NULL, !"linewidth" %in% names(self$computed_mapping) && "linewidth" %in% self$geom$aesthetics()) { self$computed_mapping$size <- plot$mapping$size - # TODO: move to cli_warn() - cli::cli_inform(c( - "{.field size} aesthetic has been deprecated for use with lines as of ggplot2 3.4.0", - "i" = "Please use {.field linewidth} aesthetic instead" - ), .frequency = "regularly", .frequency_id = "ggplot-size-linewidth") + lifecycle::deprecate_warn("3.4.0", I("Using `size` aesthetic for lines"), I("`linewidth`")) } # defaults() strips class, but it needs to be preserved for now class(self$computed_mapping) <- "uneval" From ff5bbce92fd07a162a396c3bc61eb9f6ab71940d Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Wed, 24 Aug 2022 19:20:26 +0900 Subject: [PATCH 06/10] Tweak tests --- tests/testthat/_snaps/layer.md | 2 +- tests/testthat/test-layer.r | 4 ++-- tests/testthat/test-stat-sf-coordinates.R | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/testthat/_snaps/layer.md b/tests/testthat/_snaps/layer.md index 9e7388aebf..89024b4a6c 100644 --- a/tests/testthat/_snaps/layer.md +++ b/tests/testthat/_snaps/layer.md @@ -47,7 +47,7 @@ x `fill = after_stat(data)` i Did you map your stat in the wrong layer? -# function aesthetics are wrapped with stat() +# function aesthetics are wrapped with after_stat() Problem while computing aesthetics. i Error occurred in the 1st layer. diff --git a/tests/testthat/test-layer.r b/tests/testthat/test-layer.r index 298532fe88..e4100c2682 100644 --- a/tests/testthat/test-layer.r +++ b/tests/testthat/test-layer.r @@ -55,7 +55,7 @@ test_that("missing aesthetics trigger informative error", { ) }) -test_that("function aesthetics are wrapped with stat()", { +test_that("function aesthetics are wrapped with after_stat()", { df <- data_frame(x = 1:10) expect_snapshot_error( ggplot_build(ggplot(df, aes(colour = density, fill = density)) + geom_point()) @@ -65,7 +65,7 @@ test_that("function aesthetics are wrapped with stat()", { test_that("computed stats are in appropriate layer", { df <- data_frame(x = 1:10) expect_snapshot_error( - ggplot_build(ggplot(df, aes(colour = stat(density), fill = stat(density))) + geom_point()) + ggplot_build(ggplot(df, aes(colour = after_stat(density), fill = after_stat(density))) + geom_point()) ) }) diff --git a/tests/testthat/test-stat-sf-coordinates.R b/tests/testthat/test-stat-sf-coordinates.R index e7fa887eb5..80307cfa3a 100644 --- a/tests/testthat/test-stat-sf-coordinates.R +++ b/tests/testthat/test-stat-sf-coordinates.R @@ -28,7 +28,7 @@ test_that("stat_sf_coordinates() retrieves coordinates from sf objects", { # computed variables (x and y) df_point <- sf::st_sf(geometry = sf::st_sfc(sf::st_point(c(1, 2)))) expect_identical( - comp_sf_coord(df_point, aes(x = stat(x) + 10, y = stat(y) * 10))[, c("x", "y")], + comp_sf_coord(df_point, aes(x = after_stat(x) + 10, y = after_stat(y) * 10))[, c("x", "y")], data_frame(x = 11, y = 20) ) }) From 0d8eb26327af49513a14cc6c98828db6ebe1d068 Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Wed, 24 Aug 2022 20:56:49 +0900 Subject: [PATCH 07/10] Fix error --- tests/testthat/test-stat-contour.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-stat-contour.R b/tests/testthat/test-stat-contour.R index c155c6c27a..8464b9d197 100644 --- a/tests/testthat/test-stat-contour.R +++ b/tests/testthat/test-stat-contour.R @@ -65,7 +65,7 @@ test_that("geom_contour() and stat_contour() result in identical layer data", { test_that("basic stat_contour() plot builds", { p <- ggplot(faithfuld, aes(waiting, eruptions)) + - geom_contour(aes(z = density, col = factor(stat(level)))) + geom_contour(aes(z = density, col = factor(after_stat(level)))) # stat_contour() visual tests are unstable due to the # implementation in isoband From a3ac178e2aa03d2cbab568e5574db416ebadf69c Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Wed, 24 Aug 2022 20:59:25 +0900 Subject: [PATCH 08/10] Avoid a warning of #4960 --- tests/testthat/test-geom-sf.R | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/testthat/test-geom-sf.R b/tests/testthat/test-geom-sf.R index 78ede2e477..d9b717ab58 100644 --- a/tests/testthat/test-geom-sf.R +++ b/tests/testthat/test-geom-sf.R @@ -99,18 +99,18 @@ test_that("geom_sf() removes rows containing missing aes", { colour = c("red", NA) ) - p <- ggplot(pts) + geom_sf() + p <- ggplot(pts) expect_warning( - expect_identical(grob_xy_length(p + aes(size = size)), c(1L, 1L)), + expect_identical(grob_xy_length(p + geom_sf(aes(size = size))), c(1L, 1L)), "Removed 1 row containing missing values" ) expect_warning( - expect_identical(grob_xy_length(p + aes(shape = shape)), c(1L, 1L)), + expect_identical(grob_xy_length(p + geom_sf(aes(shape = shape))), c(1L, 1L)), "Removed 1 row containing missing values" ) # default colour scale maps a colour even to a NA, so identity scale is needed to see if NA is removed expect_warning( - expect_identical(grob_xy_length(p + aes(colour = colour) + scale_colour_identity()), + expect_identical(grob_xy_length(p + geom_sf(aes(colour = colour)) + scale_colour_identity()), c(1L, 1L)), "Removed 1 row containing missing values" ) From 382bfbd168a1840cc956329da23ff91ae2c8ae81 Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Wed, 24 Aug 2022 21:02:45 +0900 Subject: [PATCH 09/10] Remove comments --- R/aes-evaluation.r | 4 ---- 1 file changed, 4 deletions(-) diff --git a/R/aes-evaluation.r b/R/aes-evaluation.r index af935325fa..7c8fef5c68 100644 --- a/R/aes-evaluation.r +++ b/R/aes-evaluation.r @@ -112,8 +112,6 @@ is_calculated <- function(x, warn = FALSE) { } else if (is.symbol(x)) { res <- is_dotted_var(as.character(x)) if (res && warn) { - # TODO: if we want to set always = TRUE, we probably need some mechanism - # to limit the warning only once per plot build lifecycle::deprecate_warn("3.4.0", I("The dot-dot notation (`..var..`)"), "after_stat()") } res @@ -122,8 +120,6 @@ is_calculated <- function(x, warn = FALSE) { } else if (is.call(x)) { if (identical(x[[1]], quote(stat))) { if (warn) { - # TODO: if we want to set always = TRUE, we probably need some mechanism - # to limit the warning only once per plot build lifecycle::deprecate_warn("3.4.0", "stat()", "after_stat()") } TRUE From 934872c4d8264867114ffc610ba864c5db191e10 Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Thu, 25 Aug 2022 09:31:36 +0900 Subject: [PATCH 10/10] Interpolation and snapshot tests --- R/aes-evaluation.r | 10 ++++++++-- tests/testthat/_snaps/aes-calculated.md | 10 ++++++++++ tests/testthat/test-aes-calculated.r | 8 ++++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/R/aes-evaluation.r b/R/aes-evaluation.r index 7c8fef5c68..f68dae587c 100644 --- a/R/aes-evaluation.r +++ b/R/aes-evaluation.r @@ -112,7 +112,10 @@ is_calculated <- function(x, warn = FALSE) { } else if (is.symbol(x)) { res <- is_dotted_var(as.character(x)) if (res && warn) { - lifecycle::deprecate_warn("3.4.0", I("The dot-dot notation (`..var..`)"), "after_stat()") + what <- I(glue("The dot-dot notation (`{x}`)")) + var <- gsub(match_calculated_aes, "\\1", as.character(x)) + with <- I(glue("`after_stat({var})`")) + lifecycle::deprecate_warn("3.4.0", what, with, id = "ggplot-warn-aes-dot-dot") } res } else if (is_quosure(x)) { @@ -120,7 +123,10 @@ is_calculated <- function(x, warn = FALSE) { } else if (is.call(x)) { if (identical(x[[1]], quote(stat))) { if (warn) { - lifecycle::deprecate_warn("3.4.0", "stat()", "after_stat()") + what <- I(glue("`{expr_deparse(x)}`")) + x[[1]] <- quote(after_stat) + with <- I(glue("`{expr_deparse(x)}`")) + lifecycle::deprecate_warn("3.4.0", what, with, id = "ggplot-warn-aes-stat") } TRUE } else { diff --git a/tests/testthat/_snaps/aes-calculated.md b/tests/testthat/_snaps/aes-calculated.md index 87450bfb04..9e0d778121 100644 --- a/tests/testthat/_snaps/aes-calculated.md +++ b/tests/testthat/_snaps/aes-calculated.md @@ -6,3 +6,13 @@ Duplicated aesthetics after name standardisation: colour +# A deprecated warning is issued when stat(var) or ..var.. is used + + `stat(foo)` was deprecated in ggplot2 3.4.0. + Please use `after_stat(foo)` instead. + +--- + + The dot-dot notation (`..bar..`) was deprecated in ggplot2 3.4.0. + Please use `after_stat(bar)` instead. + diff --git a/tests/testthat/test-aes-calculated.r b/tests/testthat/test-aes-calculated.r index eacf21b884..6366634580 100644 --- a/tests/testthat/test-aes-calculated.r +++ b/tests/testthat/test-aes-calculated.r @@ -69,3 +69,11 @@ test_that("staged aesthetics warn appropriately for duplicated names", { # One warning in building due to `stage()`/`after_scale()` expect_snapshot_warning(ggplot_build(p)) }) + +test_that("A deprecated warning is issued when stat(var) or ..var.. is used", { + p1 <- ggplot(NULL, aes(stat(foo))) + expect_snapshot_warning(b1 <- ggplot_build(p1)) + + p2 <- ggplot(NULL, aes(..bar..)) + expect_snapshot_warning(b2 <- ggplot_build(p2)) +})