R partial string matching can have unexpected results when theme elements are missing

[clauswilke]

This is related to #2723, but really a distinct issue that I wanted to record separately. If a theme element is missing then R may decide to return a different one whose name partially matches. This can have unexpected results.

library(ggplot2)
t <- theme_void()
t$legend.box
#> NULL

t <- theme_void() + theme(legend.box.background = element_rect(fill = "gray80"))
t$legend.box
#> List of 5
#>  $ fill         : chr "gray80"
#>  $ colour       : NULL
#>  $ size         : NULL
#>  $ linetype     : NULL
#>  $ inherit.blank: logi FALSE
#>  - attr(*, "class")= chr [1:2] "element_rect" "element"

Created on 2018-06-28 by the reprex package (v0.2.0).

[clauswilke]

As proposed by @karawoo here, a solution would be to write t[["legend.box"]] instead of t$legend.box.

I was wondering whether this would affect performance, and it appears to be as good as or better:

library(ggplot2)
t <- theme_void() + theme(legend.box.background = element_rect(fill = "gray80"))
microbenchmark::microbenchmark(
  t$legend.box,
  t[["legend.box"]]
)
#> Unit: microseconds
#>               expr   min     lq    mean median     uq    max neval
#>       t$legend.box 1.257 1.2810 1.51647 1.2965 1.3240 19.462   100
#>  t[["legend.box"]] 1.107 1.1355 1.25123 1.1620 1.1855  8.158   100

So we could consider making this a style guide throughout ggplot2, for more bug-free code at possibly somewhat improved performance.

[baptiste]

Isn't that the standard R behaviour? $ for interactive use (convenient but also dangerous partial matching), and [[ for programatic use (verbose but safe).

[hadley]

We could also override $.theme which would require considerably fewer changes to existing code.

[paleolimbot]

I could definitely be convinced otherwise, but I think that $ should probably never be used on a theme. Instead, calc_element() should be used, as it knows about element inheritance. Any code that isn't internal to the themes that uses $ is usually calculating its own inheritance (or assuming none), which could easily become misaligned with the inheritance defined in the theme code.

[clauswilke]

I agree, and we should move in that direction whenever we can. We're not quite there yet, though, in particular in the guide code which is a mess. Example:

ggplot2/R/guide-colorbar.r

Lines 365 to 369 in e2bdf85

	# We break inheritance for hjust and vjust, because that's more intuitive here; it still allows manual
	# setting of hjust and vjust if desired. The alternative is to ignore hjust and vjust altogether, which
	# seems worse
	if (is.null(guide$label.theme$hjust) && is.null(theme$legend.text$hjust)) label.theme$hjust <- NULL
	if (is.null(guide$label.theme$vjust) && is.null(theme$legend.text$vjust)) label.theme$vjust <- NULL

In any case, this would be a good topic for the missing vignette on internal programming guidelines. 😄