rlang icon indicating copy to clipboard operation
rlang copied to clipboard

An rlang-ish equivalent to stopifnot()

Open jennybc opened this issue 3 years ago • 8 comments

Summarizing a slack conversation with @lionel-

I'd be interested in an rlang substitute for stopifnot(). Recently, I've switched all conditions in reprex to rlang. Well, except for places where I have a whole raft of simple stopifnot()s, which are quick, basic checks I don't expect to be triggered by real users very often.

Reasons I'd like to adapt the stopifnot()s to a fictional abort_if_not():

  1. Consistency within the package
  2. Backtraces
  3. A place to get some message-making help à la abort()'s auto bullets or arg_match0(). Even stopifnot() has recently gained some features around this.

I realize, in general, we strive to make better error messages than any stopifnot() can do. But I still think there's a role for simple checks that are sort of pro forma, but that are still useful and are unlikely to be encountered by actual users.

Previous related discussion in #37

jennybc avatar Feb 04 '21 16:02 jennybc

We've been trying to figure out best development practices regarding the use of rlang::abort, assertthat::assert_that, and stopifnot in our internal package development while keeping outside dependencies to a minimum, so something along these lines would be really helpful, since we use rlang for all kinds of stuff and love abort for condition catching. For simpler / rarer error checking, though, it feels heavy-handed to import assertthat, even though we like the improved messaging. It has been somewhat difficult to navigate the role / advantages / disadvantages of different error handling contexts and appropriate strategies. Thanks!

nerutenbeck avatar Feb 18 '21 18:02 nerutenbeck

To throw another idea in the mix, I personally use a short-circuiting or, e.g.

assertion || abort(...) 

I think it's concise and readable. The drawbacks I can see is that you can get weird stuff when assertion is not a scalar boolean, there are no auto-generated error messages, and it's also marginally slower than an if. As completely crazy idea, how about something like a "magical assertion operator", e.g.

assertion %?% abort(...)

that would validate assertion as well as inject a formatted message into abort if no message was given?

tzakharko avatar Feb 25 '21 11:02 tzakharko

One thing we should definitely copy is the (IIRC relatively new ability) to customise the error message:

stopifnot("m must be symmetric"= m == t(m))

hadley avatar Apr 14 '21 22:04 hadley

In case it helps anyone here, I recently started using {checkmate} instead of {assertthat}.

Pros:

  • Each check gets 3 forms: test_ (return boolean), assert_ (raise error), and expect_ (testthat expectation)
  • Messages can be tailored to what went wrong
  • Checks can be customized (e.g. arguments to allow/disallow NAs and NULLs)
  • Extension system for new checks
  • Lightweight dependencies

Cons:

  • A lot of functions exported (bloated namespace?)
    • Especially since they support snake case and camel case
  • assert_ functions use stop() instead of abort()

davidchall avatar Sep 22 '21 15:09 davidchall

I would not dislike it if we didn't miss the opportunity not to use the negative form, which I find confusing :).

abort_if_not(!is.numeric(x)) # -> not not, I need a couple seconds to parse this
assert(!is.numeric(x)) # -> I understand right away

or provide an abort_if() counterpart for these cases

moodymudskipper avatar Jul 04 '22 12:07 moodymudskipper

I have recently uploaded another attempt (currently experimental) at rolling an assertion package (modelled after opinionated assertions in Swift), which might be relevant for this discussion. It uses abort() if rlang is installed, but also offers a type of assertion (sanity check) that will immediately stop the script bypassing any error handling if failed.

https://github.com/tzakharko/precondition

One feature of note is that parts of the assertion expression can be embraced which will print the relevant value as part of the assertion error. E.g.

x <- -5
precondition({{x}} > 0}

`x > 0` is not TRUE
x = dbl -5

tzakharko avatar Jul 04 '22 12:07 tzakharko

I stumbled across this issue last night after searching for examples of any abort_if_not helper functions and put together a basic version that works all right. Curious to hear from more experienced rlang users or contributors about what other approaches are possible to achieve this result using rlang!

library(rlang)

abort_if_not <- function(...,
                         message = NULL,
                         class = NULL,
                         not = TRUE,
                         call = caller_env()) {
  params <- list2(...)

  if (length(params) > 1) {
    for (x in seq(params)) {
      abort_if_not(
        params[[x]],
        message = names(params)[[x]],
        class = class,
        call = call
      )
    }

    invisible(return(NULL))
  }

  condition <- params[[1]]

  if (is.logical(condition) && ((!condition && not) | (condition && !not))) {
    if (is_named(params)) {
      message <- message %||% names(params)
    }

    abort(
      message = message,
      class = class,
      call = call
    )
  }

  invisible(return(NULL))
}


abort_if_not(
  "pass if true" = TRUE
)
#> NULL

abort_if_not(
  "abort if false" = FALSE
)
#> Error:
#> ! abort if false

abort_if_not(
  "pass first if true" = TRUE,
  "pass second if true" = TRUE,
  "abort third if false" = FALSE
)
#> Error:
#> ! abort third if false

abort_if_not(
  "equivalent to abort_if" = TRUE,
  not = FALSE
)
#> Error:
#> ! equivalent to abort_if

Created on 2022-07-14 by the reprex package (v2.0.1)

elipousson avatar Jul 14 '22 14:07 elipousson

One thing we should definitely copy is the (IIRC relatively new ability) to customise the error message:

just for posterity, yes, this was part of R 4.0.0.

and just as a side note to add a little more value with this comment: consider internationalization with this interface, which stopifnot() itself does not yet support

MichaelChirico avatar Feb 21 '24 20:02 MichaelChirico