Informative Assert Messages

[wilzbach]

So I presume I probably have to write a DIP for this, but as there are many open question I thought I already start hacking with DMD to see what they are and how potential solutions could look like.

Motivation

Informative assert messages that tell you why sth. failed. This is incredibly useful when the testsuite isn't run on your machine, but on a CI server or the assertion failed in production and you don't have a coredump.

tl;dr:

int a = 1, b = 2;
assert(a == b); // ERROR: 1 != 2 

Also better just assert diagnostics is the number #7 features that people miss according to the recent State of D survey:

Moreover, a better debugging experience (and in fact DIP83) has been part of the bi-annual high level visions since 2017.

Lastly, this is based on top of #7575 which would already solve two very old issues.

What's the status quo?

core.exception.AssertError@onlineapp.d(4): Assertion failure
----------------
??:? _d_assertp [0x79a0dad5]
onlineapp.d:4 _Dmain [0x79a0da08]

https://run.dlang.io/is/b3RHNb

(for the example above)

Previous work:

• Original DIP83
• Improving assert-printing in DMD NG thread from 2015
• Original DMD PR
• Issue 5547 - Improve assert to give information on values given to it when it fails
• assertPred - druntime PR

Main problems pointed out in the past

1. druntime can't use std.conv.to or anything from Phobos.

Solution: some debug output is a lot better than none. All basic types can be formatted by libc, structs could just be recursively iterated over all their basic fields and for classes .to!string only calls .toString anyhow. Even just basic types would already be a huge improvement.

2. Performance / Bloat

As all formatting is done only in the non-release build and the actual string formatting happens lazily when the error is triggered, there's no performance overhead.
However, if a templated _d_assert_fail is chosen, there might be a bit of template bloat when a project uses many asserts with different types.
If this is really a concern, a minimal implementation could only go for basic types or define a extern(C) interface. Though there's already lots of templated stuff in druntime, e.g. destroy, __delete, __cmp, __ArrayEq, __ArrayPostblit, __ArrayDtor, .dup, .idup, __switch, _arrayOp, hashOf, assumeSafeAppend, capacity, _postblitRecurse, update, require, get, values, keys, by{Key,Value,KeyValue}, rehash, ...

3. This is yet another compiler magic

Yes, but as assert is already baked into the language, let's make use of the fact and by a moderately simple change we can vastly improve the debugging/testing experience of D.

Open questions

This PR directly hacks the lazy string message that is provided to the AssertError. This has the advantage of making things easy. There could be a point made this isn't ideal because:

1. it makes lots of code that is currently @nogc, use the garbage collection for string formatting (and is potentially slower than directly printing the warning)

My take on this is that the cost only happens at most once and after which the program will be terminated anyhow (and catching errors is defined to result in UB). In fact, I think that these formatting calls can be marked as "@nogc" because either we can just use malloc and not worry about freeing or simply do the same with D's built-in concatenation.

2. Support/Integration with custom test runners / custom formatting backends

Currently, custom test runners like unit-threaded or fluent-asserts come with their own TDD or BDD style to work around the problem of not being able to catch the values of assert(a == b), with e.g. foo.should.equal(2)

3. Name of the druntime function

TODO

• Add formatting support for more than ints (it's a PoC for now)
• Add support for more than EqualExp
• For now this is hidden behind a flag to simplify testing
• Pass the token type to the formatting helper e.g. _d_assert_failed!"=="(a, b)
• Add more tests
• Check review from Issue 8765, 9255 - Show informative assert errors #7575
• Move implementation _d_assert_fail to druntime

[dlang-bot]

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8517"

[jacob-carlborg]

It's a really useful feature to have but I don't like that it's needs to be implemented like this in the compiler. Implementing this using AST macros would be perfect, but yeah, I know, that's never going to happen :(.

[timotheecour]

@jacob-carlborg , agreed ; in nim, this is relatively easily implemented in library code via macros: ```nim inim INim 0.3.0 Nim Compiler Version 0.18.1 [MacOSX: amd64] at /Users/timothee/.nimble/bin/nim $ import unittest $ check 1+3 < 1+1 Check failed: 1 + 3 < 1 + 1 1 + 3 was 4 1 + 1 was 2 Error ```

[JinShil]

Is it necessary the analyze the expression? What I mean is can't you just plop exp.toChars() into the assert message? The number of expressions that will need to be analyzed seems enormous.

[wilzbach]

Is it necessary the analyze the expression?
What I mean is can't you just plop exp.toChars() into the assert message?

That would lead to:

assert(a == b); // ERROR: a != b 

Not much more helpful than before. You still are in bad luck if the reason for the crash is hard to replicate or simply it happened on a CI test suite or in a production server.

[wilzbach]

Is it necessary the analyze the expression?
What I mean is can't you just plop exp.toChars() into the assert message?

That would lead to:

assert(a == b); // ERROR: a != b 

Not much more helpful than before. You still are in bad luck if the reason for the crash is hard to replicate or simply it happened on a CI test suite or in a production server.

The number of expressions that will need to be analyzed seems enormous.

I will do benchmarking on this once I get back to it.

[andralex]

This is nice. What's the status? FWIW it doesn't require a DIP or a flag - we never specified the contents of assertion messages (and we shouldn't).

[wilzbach]

This is nice. What's the status?

I just took some time to rework this and give you all a Christmas present.
(i.e. I addressed the main points that were blocking this from moving forward)

In short: dlang/druntime#2413 needs to be merged (this as _d_assert_fail to object.d) and then we can move forward here.

This still needs some benchmarking and a bit of further work before it can be activated by default, but I hope it's now in a shape where it can be tested behind the CLI flag and be improved by others as this is a lot better than slowly dying in the PR queue and as long as its behind the flag it won't affect anyone.

[jacob-carlborg]

I addressed the main points that were blocking this from moving forward

Really! Did you implement AST macros? 😃

[WalterBright]

Should integrate with -checkaction switch:

#8718

[wilzbach]

Should integrate with -checkaction switch:

Done.

In short: dlang/druntime#2413 needs to be merged (this as _d_assert_fail to object.d) and then we can move forward here.

No longer true, as this is now is an entirely experimental, opt-in feature, so this can be merged before druntime (and it's indeed green on the CIs) and then we can enable the actual _d_assert_fail logic, tests and changelog at druntime.

Anything else that needs to be done before we can start the first part of this experiment?

[wilzbach]

CC @thewilsonator

[wilzbach]

For the impatient: changelog entry, examples and the final bits can be found at the respective druntime PR: dlang/druntime#2413 (with digger you can even test these druntime changes for playing/experimenting with this locally).

[AndrejMitrovic]

Well done on finally getting this feature through. Nice work @wilzbach!