r-lib / rlang

Low-level API for programming with R
https://rlang.r-lib.org
Other
511 stars 138 forks source link

Error with `rlang::is_string()` in new release? #1354

Closed ddsjoberg closed 2 years ago

ddsjoberg commented 2 years ago

Good Morning!

I received a couple of reports with errors in the gtsummary package related to the rlang v1.0.0 release. I've done some investigating, and I think it's related to the rlang::is_string() function. In gtsummary, I import the function from rlang and use it often internally.

I've printed examples using rlang 1.0.0 and the previous release showing the issue. What do you think the best course of action is? I just released a new version of gtsummary last week. Should I just release another version that depends on rlang >= 1.0.0?

Thank you for taking a look!

Errors with v1.0.0

packageVersion("rlang")
#> [1] '1.0.0'
packageVersion("gtsummary")
#> [1] '1.5.1'
library(gtsummary)

trial %>%
  select(age, marker) %>%
  tbl_summary(
    type = all_continuous() ~ "continuous2",
    statistic = all_continuous() ~ c("{median} ({p25}, {p75})", "{min}, {max}"),
    missing = "no"
  ) %>%
  as_kable()
#> Error in type_check(rhs): object 'rlang_is_string' not found

Created on 2022-01-28 by the reprex package (v2.0.1)

The previous release of rlang works without issue:

packageVersion("rlang")
#> [1] '0.4.12'
packageVersion("gtsummary")
#> [1] '1.5.1'
library(gtsummary)
#> #BlackLivesMatter

trial %>%
  select(age, marker) %>%
  tbl_summary(
    type = all_continuous() ~ "continuous2",
    statistic = all_continuous() ~ c("{median} ({p25}, {p75})", "{min}, {max}"),
    missing = "no"
  ) %>%
  as_kable()
Characteristic N = 200
Age
Median (IQR) 47 (38, 57)
Range 6, 83
Marker Level (ng/mL)
Median (IQR) 0.64 (0.22, 1.39)
Range 0.00, 3.87

Created on 2022-01-28 by the reprex package (v2.0.1)

lionel- commented 2 years ago

The error message shows that is_string() is trying to use internal functions that no longer exist. It sounds like you've copied the whole is_string() function inside your package instead of referring to it with indirection.

Probably here: https://github.com/cran/gtsummary/blob/3c36595541ce68c0ff61607d28982bc9aeb625f1/R/utils-misc.R#L47

The solution is to never copy foreign functions inside your package. Here you could do:

is_string =
      list(msg = "Expecting a string as the passed value.",
           fn = function(...) is_string(...)),

In the meantime people can solve this by reinstalling your package after reinstalling rlang. But the package needs to be rebuilt, and I'm not sure what conditions trigger a rebuild of CRAN binaries. To be sure, they can reinstall it from source, causing a new version of is_string() to be copied inside your package (with the risk of getting out of date in the future again).

lionel- commented 2 years ago

By the way is_named() needs an indirection too: https://github.com/cran/gtsummary/blob/3c36595541ce68c0ff61607d28982bc9aeb625f1/R/utils-misc.R#L66

ddsjoberg commented 2 years ago

Thank you SO much for looking at the source code and identifying the issue! ๐Ÿ™‡๐Ÿผโ€โ™‚๏ธ ๐Ÿ™Œ๐Ÿผ

I would have never guessed there was an important difference between is_string and function(x) is_string(x)

lionel- commented 2 years ago

yup it's a common mistake that is very easy to make. I wish R CMD check would notify us about this.