Let stage() work without after_stat or after_scale

[yutannihilation]

Fix #4873

Not sure how this is useful, but probably stage(x) simply equals to x?

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

d <- data.frame(value = 1:10)
ggplot(d, aes(value, stage(value))) +
  geom_point()

^{Created on 2022-06-25 by the reprex package (v2.0.1)}

[yutannihilation]

While this seems to add several lines, it just collapses the logic as the line would be too long otherwise. The true diff is only adding && length(uq_expr) >= 3 to the if condition.

[yutannihilation]

I couldn't understand why start is used here. IIUC, this means stage(x, start = y) returns y, but I feel it should be x...

[thomasp85]

That seems like a contrived example? if you decidedly moves the first argument behind an unnamed one you are asking for trouble

[yutannihilation]

Yes, I agree this is an unusual usage. I just don't see the intention you put this in the chain of %||% rather than simply letting that bad manner fail. If this isn't your intention in #3534, I think we can just remove this, or use some proper evaluation. Like this:

expr <- rlang::expr(stage(x, start = y))

env <- rlang::env(stage = function(start, after_stat, after_scale) {
  rlang::enexprs(start = start, after_stat = after_stat, after_scale = after_scale)
})

rlang::eval_bare(expr, env)
#> $start
#> y
#> 
#> $after_stat
#> x
#> 
#> $after_scale

^{Created on 2022-06-30 by the reprex package (v2.0.1)}

[yutannihilation]

Anyway, all this pull request is about is just nitpicking. Sorry for the noise...

[thomasp85]

I must admit I can't fully remember why I did what I did when writing these lines... to me it probably didn't make sense to use stage() with a single argument because the purpose is to combine multiple aesthetic evaluations...

I'm not married to that idea so if you feel like changing it I'm fine with it

[yutannihilation]

didn't make sense to use stage() with a single argument

I think there can be some cases when the call is constructed programatically. But I generally agree with you that it's probably rare when this fix will be actually found useful.

With that in mind, I changed my mind not to use the actual evaluation as it seems a bit slow for the gain. Let's just remove this start branch.

library(rlang)

e1 <- expr(stage(x))
e2 <- expr(stage(x, after_stat = y))
e3 <- expr(stage(y, start = x, z))

f1 <- function(uq_expr) {
    sandbox_env <- env(stage = function(start, after_stat, after_scale) {
      expr <- enexprs(start = start, after_stat = after_stat)
      if (!is_missing(expr$after_stat)) {
        expr$after_stat
      } else {
        expr$start
      }
    })

    eval_bare(uq_expr, sandbox_env)
}
f2 <- function(uq_expr) {
  if (!is.null(uq_expr$after_stat))
    return(uq_expr$after_stat)
  if (!is.null(uq_expr$start))
    return(uq_expr$start)
  if (is.null(uq_expr$after_scale) && length(uq_expr) >= 3)
    return(uq_expr[[3]])
  uq_expr[[2]]
}


bench::mark(
  f1(e1),
  f2(e1)
)
#> # A tibble: 2 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 f1(e1)       40.7µs   44.4µs    21684.      96KB     26.1
#> 2 f2(e1)        600ns    800ns   817321.        0B      0

bench::mark(
  f1(e2),
  f2(e2)
)
#> # A tibble: 2 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 f1(e2)       41.3µs   44.8µs    21372.      280B     30.0
#> 2 f2(e2)        300ns    400ns  2098063.        0B      0

bench::mark(
  f1(e3),
  f2(e3),
  check = FALSE
)
#> # A tibble: 2 × 6
#>   expression      min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr> <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 f1(e3)         41µs   44.8µs    21800.      280B     28.4
#> 2 f2(e3)        400ns    500ns  1838032.        0B      0

^{Created on 2022-07-08 by the reprex package (v2.0.1)}