Move error signalling to rlang::abort()

[thomasp85]

This PR changes all calls to stop() to abort() from rlang. It further removes the last calls to stopifnot() in favour of if (...) abort(...) which should hopefully result in some better error messages (as well as consistent error tracing across all exceptions)

[yutannihilation]

Basically looks good to me! Two minor comments.

1. According to 2.4.1 Inline statements of The tidyverse style guide,

Function calls that affect control flow (like return(), stop() or continue) should always go in their own {} block

So I think all abort() after a if statement should go after a line break.

2. Do you want to prohibit the internal use of stop() and stopifnot() in the same way as data.frame()? If so, we might need a better mechanism to check the code statically (it would be beyond this PR, though).

[clauswilke]

And one comment from my side: Should all the new error messages in code that replaces stopifnot() follow the tidyverse style, with a statement of what is wrong and a suggestion of what might have happened?

[thomasp85]

I’ll fix 1. As for 2. I discussed this with Hadley and my feeling is that it is too likely that something will slip through and the user will be getting a don’t use stop() error message instead of the intended one. I think we just have to rely on good reviews

[clauswilke]

I've read over all changes. I didn't notice any issues except the minor typos I indicated.

[yutannihilation]

it is too likely that something will slip through and the user will be getting a don’t use stop() error message instead of the intended one

Yes. I meant, so we might need to check the code programmatically on CI. For example:

library(testthat)

get_stop <- function(f) {
  d <- getParseData(parse(f))
  d[d$token == "SYMBOL_FUNCTION_CALL" & d$text == "stop", ]
}


test_that("do not use stop()", {
  stops <- purrr::map_dfr(list.files("R", full.names = TRUE), get_stop, .id = "file")
  
  expect_equal(nrow(stops), 0)  
})
#> Error: Test failed: 'do not use stop()'
#> * nrow(stops) not equal to 0.
#> 1/1 mismatches
#> [1] 173 - 0 == 173

[thomasp85]

I'm putting this on ice until the next rlang release (@lionel- said it would be soon) that will include an immediate. argument (or equivalent) in warn()

[yutannihilation]

I'm putting this on ice until the next rlang release

Is this the version?

https://github.com/r-lib/rlang/releases/tag/v0.4.1

[thomasp85]

yeah, but immediate printing of warning turns out to not be possible to implement with the slang conditions... don't know if it is really that necessary

[lionel-]

It's impossible with any condition object, not just rlang. It's possibly a bug in base::warning().

[thomasp85]

fair - didn't meant to call rlang out

[lionel-]

Bug report submitted at https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17646

[thomasp85]

Team (@hadley, @clauswilke, @yutannihilation, @karawoo, @paleolimbot). With the merge of this PR ggplot2 now exclusively uses abort() and warn() from rlang for condition signalling. Unit tests have been set up that scans the R folder for usage of stop() and warning() and will fail if it finds any. message() remains for now (i.e. we do not use inform() from rlang), but that is not set in stone for the future.

Further, the style now is to rely on glue to compose the messages inside conditions, to the extent possible... It is not checked, so be aware of this when reviewing PRs