replace parse() with parse_safe() in geom_text() (#2867)

Closes issue #2864

* use parse_safe() in geom_text()

The built in `parse()` function silently drops items:

    length(parse(text = c("alpha", "", "gamma")))

We expect the length to be 3, but instead it is 2.

So, we add a new function `parse_safe()` that keeps all items.


* update parse_safe()

Use Hadley's code to parse the expressions once instead of twice.

Use `parse(..., keep.source = FALSE)`, as Claus recommended.

Add an example to demonstrate why `parse_safe()` is needed.

* add tests for parse_safe()

* add note about parse(text = NULL)

* use parse_safe() in sf.R

This patch fixes an example that triggers an error:

    library(ggplot2)
    nc <- sf::st_read(system.file("shape/nc.shp", package = "sf"), quiet = TRUE)
    ggplot(nc) +
      geom_sf(aes(fill = AREA)) +
      scale_y_continuous(
        breaks = c(34, 35, 36),
        labels = c("34*degree*N", "", "36*degree*N")
      )
    #> Error in parse(text = x)[[1]]: subscript out of bounds

See #2867 for more details.

* move parse_safe() tests to test-utilities.r

* change parse_safe(text = x) to parse_safe(x)

* modify comment for parse_safe()

add URL to GitHub issue #2864

* fix parsing degree labels in geom_sf()

* add news item about parse_safe()

* fix typo

* move parse() into fixup_graticule_labels()

* change description of parse_safe()

* modify news item about parse_safe()

* add stopifnot() to parse_safe()

* simplify parsing code in sf.R

* improve news item about parse_safe()

* do not test parse_safe(factor(...))

`parse_safe()` takes a character vector, not a factor

Author: slowkow

NEWS.md

@@ -59,6 +59,12 @@
 *   `position_nudge()` is now more robust and nudges only in the direction
     requested. This enables, for example, the horizontal nudging of boxplots
     (@clauswilke, #2733).
+    
+*   `geom_text(..., parse = TRUE)` now correctly renders the expected number of
+    items instead of silently dropping items that are empty expressions, e.g.
+    the empty string "". If an expression spans multiple lines, we take just
+    the first line and drop the rest. This same issue is also fixed for
+    `geom_label()` and the axis labels for `geom_sf()` (@slowkow, #2867).
 
 # ggplot2 3.0.0
 

R/geom-label.R

@@ -63,7 +63,7 @@ GeomLabel <- ggproto("GeomLabel", Geom,
                         label.size = 0.25) {
     lab <- data$label
     if (parse) {
-      lab <- parse(text = as.character(lab))
+      lab <- parse_safe(as.character(lab))
     }
 
     data <- coord$transform(data, panel_params)

R/geom-text.r

@@ -159,7 +159,6 @@ geom_text <- function(mapping = NULL, data = NULL,
   )
 }
 
-
 #' @rdname ggplot2-ggproto
 #' @format NULL
 #' @usage NULL
@@ -176,7 +175,7 @@ GeomText <- ggproto("GeomText", Geom,
                         na.rm = FALSE, check_overlap = FALSE) {
     lab <- data$label
     if (parse) {
-      lab <- parse(text = as.character(lab))
+      lab <- parse_safe(as.character(lab))
     }
 
     data <- coord$transform(data, panel_params)

R/sf.R

@@ -476,9 +476,13 @@ CoordSf <- ggproto("CoordSf", CoordCartesian,
     if (!is.null(graticule$plot12))
       graticule$degree_label[!graticule$plot12] <- NA
 
-    # parse labels into expressions if required
-    if (any(grepl("degree", graticule$degree_label)))
-      graticule$degree_label <- lapply(graticule$degree_label, function(x) parse(text = x)[[1]])
+    # Convert the string 'degree' to the degree symbol
+    has_degree <- grepl("\\bdegree\\b", graticule$degree_label)
+    if (any(has_degree)) {
+      labels <- as.list(graticule$degree_label)
+      labels[has_degree] <- parse_safe(graticule$degree_label[has_degree])
+      graticule$degree_label <- labels
+    }
 
     graticule
   },

R/utilities.r

@@ -430,3 +430,22 @@ is_column_vec <- function(x) {
   dims <- dim(x)
   length(dims) == 2L && dims[[2]] == 1L
 }
+
+# Parse takes a vector of n lines and returns m expressions.
+# See https://github.com/tidyverse/ggplot2/issues/2864 for discussion.
+#
+# parse(text = c("alpha", "", "gamma"))
+# #> expression(alpha, gamma)
+#
+# parse_safe(text = c("alpha", "", "gamma"))
+# #> expression(alpha, NA, gamma)
+#
+parse_safe <- function(text) {
+  stopifnot(is.character(text))
+  out <- vector("expression", length(text))
+  for (i in seq_along(text)) {
+    expr <- parse(text = text[[i]])
+    out[[i]] <- if (length(expr) == 0) NA else expr[[1]]
+  }
+  out
+}

tests/testthat/test-utilities.r

@@ -43,3 +43,47 @@ test_that("find_args behaves correctly", {
   # Defaults are overwritten
   expect_true(test_fun(arg2 = TRUE)$arg2)
 })
+
+test_that("parse_safe works with simple expressions", {
+  expect_equal(
+    parse_safe(c("", " ", "     ")),
+    expression(NA, NA, NA)
+  )
+
+  expect_equal(
+    parse_safe(c("A", "B", "C")),
+    expression(A, B, C)
+  )
+
+  expect_equal(
+    parse_safe(c("alpha", "", "gamma", " ")),
+    expression(alpha, NA, gamma, NA)
+  )
+
+  expect_equal(
+    parse_safe(c(NA, "a", NA, "alpha")),
+    expression(NA, a, NA, alpha)
+  )
+})
+
+test_that("parse_safe works with multi expressions", {
+  expect_equal(
+    parse_safe(c(" \n", "\n ", " \n  \n  ")),
+    expression(NA, NA, NA)
+  )
+
+  expect_equal(
+    parse_safe(c("alpha ~ beta", "beta \n gamma", "")),
+    expression(alpha ~ beta, beta, NA)
+  )
+
+  expect_equal(
+    parse_safe(c("alpha ~ beta", " ", "integral(f(x) * dx, a, b)")),
+    expression(alpha ~ beta, NA, integral(f(x) * dx, a, b))
+  )
+
+  expect_equal(
+    parse_safe(c(NA, 1, 2, "a \n b")),
+    expression(NA, 1, 2, a)
+  )
+})