Signed-off-by: Drew DeVault <sir@cmpwn.com>
---
language/expressions.tex | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/language/expressions.tex b/language/expressions.tex
index 86b9c4d..e889877 100644
--- a/language/expressions.tex+++ b/language/expressions.tex
@@ -614,7 +614,8 @@ result type of these forms is \terminal{void}.
In the second \terminal{assert} form, and in the \terminal{abort} form if
present, the final \nonterminal{expression} shall have type \terminal{str},
which shall be provided to it as a type hint. The contents of the string shall
-be included in the diagnostic message.+be included in the diagnostic message. The final \nonterminal{expression} shall+utilize the \secref{Translation compatible expression subset}.\specsubsubitem
In the \terminal{abort} form, the execution environment shall unconditionally
--
2.47.1
This is a somewhat useful feature for table-based tests, where one
assert statement accounts for many test cases. Without this, you don't
know which test case triggered the assert.
I think it's all good if we add assert fns to fmt::
fn assert(cond: bool, args: formattable...) void = {
if (cond) {
error(args);
os::exit(255);
};
};
and similar for an assertf. Only problem is the keyword name "assert".
Any suggestions for a better name? "fatalif/fatalfif"?
On Sun Dec 29, 2024 at 1:51 PM CET, Joe Finney wrote:
> and similar for an assertf. Only problem is the keyword name "assert".> Any suggestions for a better name? "fatalif/fatalfif"?
I agree that this is the main use-case, but I think you can easily just
add fmt::println/fmt::errorln liberally to your tests and it works even
better, now that the test harness captures the stdout/stderr of each
test case to display on failure in the test summary.
I didn't add this to the bytes/strings tests I updated in my
corresponding stdlib patch because it would have introduced a dependency
loop.
> I agree that this is the main use-case, but I think you can easily just> add fmt::println/fmt::errorln liberally to your tests and it works even> better, now that the test harness captures the stdout/stderr of each> test case to display on failure in the test summary.
That's fine, I suppose. At some point I would like a more powerful test
harness (possibly a library, possibly improve the built-in one), but
that's a discussion for later.
> I didn't add this to the bytes/strings tests I updated in my> corresponding stdlib patch because it would have introduced a dependency> loop.
This bothered me to no end while I wrote the strconv/ftos test. No good
way around it, alas.
The patch looks good to me, except that "utilize" is an odd word. I
would suggest "shall be limited to". Not at all a big deal though.
On Sun Dec 29, 2024 at 3:32 PM CET, Drew DeVault wrote:
> On Sun Dec 29, 2024 at 1:51 PM CET, Joe Finney wrote:> > and similar for an assertf. Only problem is the keyword name "assert".> > Any suggestions for a better name? "fatalif/fatalfif"?>> I agree that this is the main use-case, but I think you can easily just> add fmt::println/fmt::errorln liberally to your tests and it works even> better, now that the test harness captures the stdout/stderr of each> test case to display on failure in the test summary.>> I didn't add this to the bytes/strings tests I updated in my> corresponding stdlib patch because it would have introduced a dependency> loop.
Ok so I changed my opinion on this way too many times, sorry.
Ultimately, I think this should not be removed. I agree with Joe in that
this is very useful in tests, but I don't agree that replacing the
assert/abort builtin functionality with a function is adequate.
The most important issue is that this is replacing a language feature
with a library call, assuming the library is present and functioning,
and that the user is willing and able to depend on it, and even then,
the message is getting printed without location information.
With this functionality, the implementation promises to magically take
care of displaying whatever message. It can also annotate it with
location information or other things the programmer might care about.
Without this functionality, the implementation is freed of this
(trivial, as can be seen from the removal patch) requirement, and the
burden is shifted to the library, and to the user. And that burden is
much less trivial than the cost of supporting this in the
implementation. I don't want to depend on computing backtraces just so I
can see in which line my test is failing.
There is also the issue of conditional evaluation of the error message,
which is useful, and cannot be done with a regular function, but that's
unlikely to be a big deal in tests.
So in short, this functionality is useful, trivial to support, and hard
to adequately replace. So I think we should just keep it.
I think that it has too many footguns and it isn't that useful. Out of
nearly 4,000 asserts and aborts in the stdlib there were, what, six of
them which needed to be fixed following this change? And I've heard
*several* people shooting themselves in this particular foot before. I'd
rather make the change to simplify it. IMO it's bad practice to spend
cycles and/or memory generating assertion failure messages anyway, since
in theory, you know, assertions aren't supposed to fail and it's a waste
of time and/or resources.
> I think that it has too many footguns and it isn't that useful.> rather make the change to simplify it.
The footgun here was that stack-allocated strings don't work with
assert/abort in tests. Are there any others, and would it be too
expensive to fix the test runner to save the message before the stack is
lost?
> Out of nearly 4,000 asserts and aborts in the stdlib there were, what,> six of them which needed to be fixed following this change?
To be fair, this feature was only recently-ish added.
> The most important issue is that this is replacing a language feature> with a library call, assuming the library is present and functioning,> and that the user is willing and able to depend on it, and even then,> the message is getting printed without location information.
Is there a reason to not print stack traces when tests abort, regardless
of this patch?
On Sun Dec 29, 2024 at 5:07 PM CET, Joe Finney wrote:
> The footgun here was that stack-allocated strings don't work with> assert/abort in tests. Are there any others, and would it be too> expensive to fix the test runner to save the message before the stack is> lost?
The whole "always" discussion came up from something which reduces to
the following iirc:
let x = strings::dup("foobar");
defer free(x);
assert(false, x);
or something like that, idk
I'd rather not dup the message tbh. I still think it's bad practice.
IMO that's a broader problem with defer, not assert. You can replace
the assert with fmt::fatal and it will have the same issue. Your point
still stands though, the problem wouldn't happen with assert after your
patch.
I don't feel very strongly either way wrt this patch, so I'll check out
and let others chime in. It's probably good to note that this is a
breaking change in one of the commit messages.
> > The most important issue is that this is replacing a language feature> > with a library call, assuming the library is present and functioning,> > and that the user is willing and able to depend on it, and even then,> > the message is getting printed without location information.>> Is there a reason to not print stack traces when tests abort, regardless> of this patch?
It doesn't work in absence of debug info (all non-debug builds), and it
needs platform specific code to work. So it should be treated as a
nice-to-have feature that the official stdlib provides for well
supported targets, not something that works everywhere.
And parsing the debug info takes a lot of code - debug:: is among the
bigger components of the stdlib.
> IMO it's bad practice to spend> cycles and/or memory generating assertion failure messages anyway, since> in theory, you know, assertions aren't supposed to fail and it's a waste> of time and/or resources.
Yeah I agree. This is part of why we need the builtin to work like
that - it is the only way to conditionally generate the error message
without resorting to if expressions.
On Sun Dec 29, 2024 at 6:59 PM CET, Bor Grošelj Simić wrote:
>> IMO it's bad practice to spend>> cycles and/or memory generating assertion failure messages anyway, since>> in theory, you know, assertions aren't supposed to fail and it's a waste>> of time and/or resources.>> Yeah I agree. This is part of why we need the builtin to work like> that - it is the only way to conditionally generate the error message> without resorting to if expressions.
I don't think much mind need be paid to the quality of assertion
messages. In a test context, you could add that "if", or you could chase
down the stack trace, or you could add additional logging. In a release
context (that is, -test), assertions should be kept simple and sweet.
On Sun Dec 29, 2024 at 6:45 PM CET, Bor Grošelj Simić wrote:
> It doesn't work in absence of debug info (all non-debug builds), and it> needs platform specific code to work. So it should be treated as a> nice-to-have feature that the official stdlib provides for well> supported targets, not something that works everywhere.
The first thing you should generally do when you hit an assertion
failure (which isn't immediately obvious in its cause) in any systems
language is rebuild with symbols and debug bits and try to reproduce
there.