Open timotheecour opened 3 years ago
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.
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
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.
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.
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)
DrNim uses ensures
with a different meaning already: https://nim-lang.org/docs/drnim.html#preminus-postconditions-and-invariants-ensures
The "keywords" used are:
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!
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.
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
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.
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
.
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: https://github.com/nim-lang/fusion/pull/19#discussion_r508135593 regarding assert question.len > 0, "Question must not be empty"
being wrong
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.
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.
Looking forward to seeing this in your next library, @timotheecour.
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.
Too many times I see assert or doAssert being used in a place where a catchableException should've been used, because:
The current approach is to use:
while this works, it has following drawbacks:
assert condition
orassert condition, msg
CatchableException
condition
isn't renderedproposal
enforce
, which works likedoAssert
, but by default raises a CatchableException; the exception type can be customizedexample
precedent
in D, they also use both
assert
andenforce
: https://dlang.org/library/std/exception/enforce.html