Check read_builtin()'s inputs #1361

jennybc · 2022-01-27T01:35:59Z

This is in reponse to a request from CRAN.

Previously this expectation:

expect_error(read_builtin(AirPassengers, "datasets"))

would call utils::data() on malformed input, i.e. intentionally calling with the symbol AirPassengers. Eventually that lead to some condition having length greater than 1. So we got an error, alright, but an error that indicates a programming problem.

I think this means utils::data() might have an input-checking problem, but it's not my problem.

Instead, I'll be more careful to catch obviously bad input in readr itself.

I was able to reproduce what CRAN reported with released R (they used R-devel) with env var settings such as:

_R_CHECK_LENGTH_1_CONDITION_=verbose
_R_CHECK_LENGTH_1_CONDITION_="abort,verbose"

Note that "true" is not enough to surface the issue, because the offending code is inside expect_error().

This is in reponse to a request from CRAN. Previously this expectation: expect_error(read_builtin(AirPassengers, "datasets")) would call `utils::data()` on malformed input. Eventually that lead to some condition having length greater than 1. So we got an error, but an error that indicates a programming problem. I think this means `utils::data()` might have a condition length problem, but it's not my problem. Instead, I'll be more careful to catch obviously bad input in readr itself. I was able to reproduce what CRAN reported with released R (they used R-devel) with env var settings such as: _R_CHECK_LENGTH_1_CONDITION_=verbose _R_CHECK_LENGTH_1_CONDITION_="abort,verbose" Note that "true" is not enough to surface the issue, because the offending code is inside expect_error().

jennybc · 2022-01-27T01:37:27Z

@DavisVaughan I don't think there's anything particularly tricky here, just looking for another set of 👀 since I decided to create a typical check_string() helper and use it in all the other obvious places I could find.

jennybc · 2022-01-27T01:39:09Z

R/read_builtin.R

  tryCatch(
    warning = function(e) warn_to_error(e),
    expr = {
-      res <- utils::data(list = list(x), package = package, envir = environment(), verbose = FALSE)
+      res <- utils::data(list = x, package = package, envir = environment(), verbose = FALSE)


I think the original list(x) was sort of a mistake. The list argument of utils::data() is supposed to be a character vector (despite its confusing name).

Actually I've done more tests now and this really is the mistake, at least on our end. Just fixing this alone would be enough to avoid triggering "the condition has length > 1". But I think we might as well check the inputs and deliver a more informative error message.

I find it a little silly that the argument is named list but wants a character vector 😛

Agreed that this was a bug on our end.

DavisVaughan · 2022-01-27T14:12:56Z

NEWS.md

@@ -4,6 +4,8 @@

 * `show_progress()` uses `rlang::is_interactive()` instead of `base::interactive()` (#1356).

+* `read_builtin()` does more argument checking, so that we catch obviously malformed input before passing along to `utils::data()`.


It is kind of a chicken and egg problem, but if you don't have an issue open for this problem then I tend to open the PR, document the issue in the PR like you have done, then do another commit that would add (#1361) to the NEWS bullet. It is annoying that you can't do that on a first pass, but you don't know the PR number (for sure) until you open it.

DavisVaughan · 2022-01-27T14:24:00Z

R/read_builtin.R

  tryCatch(
    warning = function(e) warn_to_error(e),
    expr = {
-      res <- utils::data(list = list(x), package = package, envir = environment(), verbose = FALSE)
+      res <- utils::data(list = x, package = package, envir = environment(), verbose = FALSE)


I find it a little silly that the argument is named list but wants a character vector 😛

Agreed that this was a bug on our end.

DavisVaughan · 2022-01-27T14:36:08Z

R/utils.R

@@ -190,3 +190,13 @@ readr_enquo <- function(x) {
  }
  x
 }
+
+check_string <- function(x, nm = deparse(substitute(x)), optional = FALSE) {
+  if (rlang::is_string(x)) {


I appreciate that this also catches NA_character_, which the previous checks would have allowed

jennybc · 2022-01-30T23:24:55Z

CRAN has accepted the patch version of readr where we've fixed this multiple ways (v2.1.2).

But, as predicted, I note that base R just gained a check that the list input of utils::data() is "character":

wch/r-source@e223ca2

jennybc · 2022-01-30T23:27:47Z

I noticed this because readr v2.1.1 (the previous, unfixed version) just started to fail on one of CRAN's r-devel flavours. This should self-resolve when CRAN's readr checks pick up the new version (v2.1.2).

Version: 2.1.1
Check: examples
Result: ERROR
    Running examples in ‘readr-Ex.R’ failed
    The error most likely occurred in:

    > ### Name: read_builtin
    > ### Title: Read built-in object from package
    > ### Aliases: read_builtin
    >
    > ### ** Examples
    >
    > read_builtin("mtcars", "datasets")
    Error in utils::data(list = list(x), package = package, envir = environment(), :
     is.character(list) is not TRUE
    Calls: read_builtin ... tryCatchOne -> doTryCatch -> <Anonymous> -> stopifnot
    Execution halted
Flavor: r-devel-linux-x86_64-fedora-clang

Version: 2.1.1
Check: tests
Result: ERROR
     Running ‘first_edition.R’ [45s/45s]
     Running ‘second_edition.R’ [47s/48s]
     Running ‘spelling.R’
    Running the tests in ‘tests/first_edition.R’ failed.
    Complete output:
     > library(testthat)
     > library(readr)

     Attaching package: 'readr'

     The following objects are masked from 'package:testthat':

     edition_get, local_edition

     >
     > local_edition(1)
     Setting deferred event(s) on global environment.
     * Will be run automatically when session ends
     * Execute (and clear) with `withr::deferred_run()`.
     * Clear (without executing) with `withr::deferred_clear()`.
     > test_check("readr")
     [ FAIL 1 | WARN 0 | SKIP 21 | PASS 728 ]

     ══ Skipped tests ═══════════════════════════════════════════════════════════════
     • On CRAN (10)
     • edition_first() is TRUE (11)

     ══ Failed tests ════════════════════════════════════════════════════════════════
     ── Error (test-read-builtin.R:14:3): read_builtin works ────────────────────────
     Error in `utils::data(list = list(x), package = package, envir = environment(),
     verbose = FALSE)`: is.character(list) is not TRUE
     Backtrace:
     ▆
     1. ├─testthat::expect_true(is.data.frame(read_builtin("BOD", "datasets"))) at test-read-builtin.R:14:2
     2. │ └─testthat::quasi_label(enquo(object), label, arg = "object")
     3. │ └─rlang::eval_bare(expr, quo_get_env(quo))
     4. ├─base::is.data.frame(read_builtin("BOD", "datasets"))
     5. └─readr::read_builtin("BOD", "datasets")
     6. ├─base::tryCatch(...)
     7. │ └─base tryCatchList(expr, classes, parentenv, handlers)
     8. │ └─base tryCatchOne(expr, names, parentenv, handlers[[1L]])
     9. │ └─base doTryCatch(return(expr), name, parentenv, handler)
     10. └─utils::data(...)
     11. └─base::stopifnot(is.character(list))

     [ FAIL 1 | WARN 0 | SKIP 21 | PASS 728 ]
     Error: Test failures
     Execution halted
    Running the tests in ‘tests/second_edition.R’ failed.
    Complete output:
     > library(testthat)
     > library(readr)

     Attaching package: 'readr'

     The following objects are masked from 'package:testthat':

     edition_get, local_edition

     >
     > test_check("readr")
     [ FAIL 1 | WARN 0 | SKIP 22 | PASS 724 ]

     ══ Skipped tests ═══════════════════════════════════════════════════════════════
     • On CRAN (8)
     • edition_first() is not TRUE (14)

     ══ Failed tests ════════════════════════════════════════════════════════════════
     ── Error (test-read-builtin.R:14:3): read_builtin works ────────────────────────
     Error in `utils::data(list = list(x), package = package, envir = environment(),
     verbose = FALSE)`: is.character(list) is not TRUE
     Backtrace:
     ▆
     1. ├─testthat::expect_true(is.data.frame(read_builtin("BOD", "datasets"))) at test-read-builtin.R:14:2
     2. │ └─testthat::quasi_label(enquo(object), label, arg = "object")
     3. │ └─rlang::eval_bare(expr, quo_get_env(quo))
     4. ├─base::is.data.frame(read_builtin("BOD", "datasets"))
     5. └─readr::read_builtin("BOD", "datasets")
     6. ├─base::tryCatch(...)
     7. │ └─base tryCatchList(expr, classes, parentenv, handlers)
     8. │ └─base tryCatchOne(expr, names, parentenv, handlers[[1L]])
     9. │ └─base doTryCatch(return(expr), name, parentenv, handler)
     10. └─utils::data(...)
     11. └─base::stopifnot(is.character(list))

     [ FAIL 1 | WARN 0 | SKIP 22 | PASS 724 ]
     Error: Test failures
     Execution halted
Flavor: r-devel-linux-x86_64-fedora-clang

jennybc requested a review from DavisVaughan January 27, 2022 01:36

jennybc commented Jan 27, 2022

View reviewed changes

DavisVaughan approved these changes Jan 27, 2022

View reviewed changes

jennybc added 2 commits January 27, 2022 08:10

Record PR number in NEWS

0edb578

cli::cli_abort() requires cli >= 3.0.0

1afc65d

jennybc merged commit 718c42c into main Jan 27, 2022

jennybc deleted the check-read-builtin-input branch January 27, 2022 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check read_builtin()'s inputs #1361

Check read_builtin()'s inputs #1361

jennybc commented Jan 27, 2022 •

edited

Loading

jennybc commented Jan 27, 2022 •

edited

Loading

jennybc Jan 27, 2022

jennybc Jan 27, 2022 •

edited

Loading

DavisVaughan Jan 27, 2022

DavisVaughan Jan 27, 2022

DavisVaughan Jan 27, 2022

DavisVaughan Jan 27, 2022

jennybc commented Jan 30, 2022

jennybc commented Jan 30, 2022

		@@ -4,6 +4,8 @@

		* `show_progress()` uses `rlang::is_interactive()` instead of `base::interactive()` (#1356).

		* `read_builtin()` does more argument checking, so that we catch obviously malformed input before passing along to `utils::data()`.

Check read_builtin()'s inputs #1361

Check read_builtin()'s inputs #1361

Conversation

jennybc commented Jan 27, 2022 • edited Loading

jennybc commented Jan 27, 2022 • edited Loading

jennybc Jan 27, 2022

Choose a reason for hiding this comment

jennybc Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

DavisVaughan Jan 27, 2022

Choose a reason for hiding this comment

DavisVaughan Jan 27, 2022

Choose a reason for hiding this comment

DavisVaughan Jan 27, 2022

Choose a reason for hiding this comment

DavisVaughan Jan 27, 2022

Choose a reason for hiding this comment

jennybc commented Jan 30, 2022

jennybc commented Jan 30, 2022

jennybc commented Jan 27, 2022 •

edited

Loading

jennybc commented Jan 27, 2022 •

edited

Loading

jennybc Jan 27, 2022 •

edited

Loading