enforce: like doAssert for for catchableExceptions + allows customizing exception type

[timotheecour]

Too many times I see assert or doAssert being used in a place where a catchableException should've been used, because:

• the error is recoverable
• it's related to user input validation, not internal logic

The current approach is to use:

if not condition:
  raise newException(msg, SomeException)

while this works, it has following drawbacks:

• more verbose/less readable than the assert counterpart: assert condition or assert condition, msg
• in practice, this encourages people to use assert/doAssert instead of raising a CatchableException
• it gives less informative errors when exception is un-caught or caught and displayed, in particular because condition isn't rendered

proposal

• introduce enforce, which works like doAssert, but by default raises a CatchableException; the exception type can be customized
• see PR where I implemented this add std/asserts containing enforce; (catchable + customizable doAssert ) Nim#15606
• it addresses all the above drawbacks

example

enforce a == 1
enforce a == 1, $(a, someOtherContext)
enforce a == 1, $(a, someOtherContext), ValueError
enforce a == 1, typ=ValueError

precedent

in D, they also use both assert and enforce: https://dlang.org/library/std/exception/enforce.html

[Clyybber]

The if statement is clear and readable, "encourages" writing proper error messages and is not something people will have to search for in the documentation.

I don't think the stdlib is the right place for these tiny, highly specific one-char-savers that will never be used in practice because nobody searches the stdlib for "enforce" trying to save themselves from writing a perfectly normal if statement.

[timotheecour]

how is that a 1-char saver?

if not condition:
  raise newException(msg, SomeException)

vs
# in simplest form: still produces informative msg: `(pathto/file(2,3): "a == 1" failed [CatchableError]`
enforce a == 1

# or with optional msg (when `a == 1` isn't enough) and typ (when the default `CatchableError` isn't desirable), as shown in top post

and you're missing this point:

it gives less informative errors when exception is un-caught or caught and displayed, in particular because condition isn't rendered

[Araq]

I like it except for the idea that the error message is produced automatically from the source code. Tying the error message to a particular implementation is amateurish, error messages that a system produces should be stable so that people can search the internet for them, sometimes they are translated into other languages etc.

[mratsim]

I think this should be designed with DrNim preCondition/postCondition, ascertain/ensure in mind so that we don't have too many "contract" words.

Otherwise I agree, the if + raise is quite long.

[timotheecour]

I like it except for the idea that the error message is produced automatically from the source code

it's no different from doAssert, assert, or unittest.check, unittest.require etc: you'll get "a == 1 failed" + an optional msg if msg is provided. I'm fairly sure people would complain if that "a == 1 failed" part of the message were removed in doAssert/assert/check/require messages.

ascertain/ensure

I can change to ensure if there's consensus; ascertain is not as good (harder to remember, less common etc)

[mratsim]

DrNim uses ensures with a different meaning already: https://nim-lang.org/docs/drnim.html#preminus-postconditions-and-invariants-ensures

The "keywords" used are:

• requires -> precondition
• ensures -> postcondition
• invariant -> loop invariant
• assume -> unsafe assumption

[Araq]

it's no different from doAssert, assert, or unittest.check, unittest.require etc:

These are all not supposed to be error messages for end-users!

[timotheecour]

These are all not supposed to be error messages for end-users!

you could argue uncaught exceptions also shouldn't reach end-users. The reality is that we routinely have to read both exceptions and assertions, whether during development or in realeased software.

The condition is almost always useful to show and not showing it would just lead one to paraphrase the condition and translating to english, often loosing in precision.

eg from os.nim:

  if file.isNil:
    raise newException(IOError, "File is nil")

with condition rendered and message omitted:

  # shows: 'file.isNil' failed [IOError]
  enforce file.isNil, typ=IOError

this is simpler to read/write the code and results in error message that is no worse than the following where the condition is not rendered and message compulsory:

  enforce file.isNil, "File is nil", IOError

A good msg should not paraphrase the condition in english (that's redundant when condition is rendered) but offer complementary information, when needed, such as showing runtime values for the relevant variables in context, and when the rendered condition isn't clear, offer additional explanation.

Most of the examples are similar:

fmt"{var=} is often all you need:

if not root.isAbsolute:
  raise newException(ValueError, "The specified root is not absolute: " & root)
=>
# simpler: 
enforce root.isAbsolute, fmt"{root=}, ValueError

which shows: 'root.isAbsolute' failed; root="main.nim" [ValueError]

  if dir.len == 0: # treating "" as "." is error prone
    raise newException(ValueError, "dest is empty")
=>
  enforce dir.len == 0, typ=ValueError

here the autogenerated message 'dir.len == 0' is enough since the context wouldn't add any useful information

[Araq]

Well you said it yourself:

 "The specified root is not absolute: " & root)

vs.

'root.isAbsolute' failed; root="main.nim" [ValueError]

Which one do you think can be translated into other languages?

you could argue uncaught exceptions also shouldn't reach end-users. The reality is that we routinely have to read both exceptions and assertions, whether during development or in realeased software.

Agreed, which is why we should nudge programmers into clear error messages and not seduce them to save keystrokes, user-experience be damned.

[juancarlospaco]

This looks like an ad-hoc workaround for a proper DbC module, I would prefer a proper DbC module on stdlib.
People will still prefer assert because it produces no code when -d:danger.

[timotheecour]

• assert/doAssert is for validating internal logic and is not supposed to be recoverable in normal circumstances.
• enforce is for validating external input and is supposed to be recoverable; it's part of the normal execution flow to correctly handle the exceptional case that can occur.

if you change if not cond: raise newException(...) to assert(cond) it will change the semantics of valid programs (that expect copyFile + friends can occasionally raise) into undefined behavior.

Exceptions (or at least return codes) are fundamental to correctness, eg try writing copyFile or setCurrentDir with pre-conditions (eg validating user input), it will not be correct because of race conditions or because you can't possibly predict how things can fail in a cross platform way.

Furthermore, the overhead of any condition checking in things like os.nim will be dwarfed by the cost of the actual system call, so it's a false saving.

Hence my comment here: nim-lang/fusion#19 (comment) regarding assert question.len > 0, "Question must not be empty" being wrong

note regarding external vs internal data

external input for a publicly exposed proc covers input arguments and depending on cases other global data (eg environment variables accessed) since it could be accessed by any client.
For a private proc, assert may be adequate to validate input arguments since you can validate user input in other exposed procs.

[juancarlospaco]

I am not saying the idea is bad, I am just saying why not make a dbc module with enforce,
something like assert but takes varargs, something like doAssert but takes varargs,

assert a > b
assert b > 0
assert a > 0
assert a < int.high
assert b < int.high

Something like:

enforce a > b, b > 0, a > 0, a < int.high, b < int.high

I would prefer the naming precondition, postcondition to give more meaning to the code,
but use enforce or whatever if you want. It can also be "macro unrolled" into the 1st form.

[disruptek]

Looking forward to seeing this in your next library, @timotheecour.

[github-actions]

This RFC is stale because it has been open for 1095 days with no activity. Contribute a fix or comment on the issue, or it will be closed in 30 days.