Hi everyone,
As some of you may know, my company is developing a blockchain where the node is written solely in D (https://github.com/bpfkorea/agora).
One of the reason to pick D was the ease of prototyping and the C++ compatibility (we need to interface with modern C++ code, e.g. C++14 and 17).

Since tests matters, we have developed a testing framework which use(d) `std.concurrency` to simulate a network in memory (1 thread == 1 node), which works *very well*.

However, there was a little gotcha: When an error happens, the thread would crash, and we would get a timeout in the main thread, and sometimes would not be able to print the logs. We use Ocean's Logger, which is derived from Tango's, which allow to essentially set the output, and in test mode we set it to a large static array acting as a circular buffer.

One of our developer was hit particularly hard by this while implementing a new feature, so added a bunch of `writeln`. After discussion with the team, the solution seemed obvious: Let's use `assertHandler` !

For those of you not familiar with it, here is `assertHandler`'s documentation: https://dlang.org/library/core/exception/assert_handler.html
It essentially allows to define a custom behavior on `assert`. It is slightly redundant with other facilities to do so, such as `-checkaction`, which allow to customize the behavior at the compiler level.

That's when things went bad. Since we wanted a core dump, we added an `abort` in our handler, expecting that it will only trigger when we have bugs. And our tests started to fail. And that's where the rant begins.

We got hit by this (reduced) stack trace:
```
[/usr/local/Cellar/ldc/1.23.0/include/dlang/ldc/std/typecons.d:2979] Assertion thrown during test: Called `get' on null Nullable!int.
----------------
??:? nothrow void agora.test.Base.testAssertHandler(immutable(char)[], ulong, immutable(char)[]) [0x107807564]
??:? onAssertErrorMsg [0x10838ea21]
??:? _d_assert_msg [0x10838ee35]
??:? inout pure nothrow ref @property @nogc @safe inout(int) std.typecons.Nullable!(int).Nullable.get() [0x1079146d5]
??:? inout pure nothrow ref @property @nogc @safe inout(int) std.typecons.Nullable!(int).Nullable.get_() [0x107911958]
??:? pure nothrow @nogc @safe int std.typecons.Nullable!(immutable(char)[]).Nullable.__unittest_L2998_C13_1().__dgliteral1() [0x10791193c]
??:? pure void std.exception.assertThrown!(core.exception.AssertError, int).assertThrown(lazy int, immutable(char)[], immutable(char)[], ulong) [0x10791177b]
??:? pure void std.typecons.Nullable!(immutable(char)[]).Nullable.__unittest_L2998_C13_1() [0x1079116b8]
??:? agora.common.Serializer.__unittest [0x107bf97d8]
??:? void agora.test.Base.customModuleUnitTester().runTest(agora.test.Base.customModuleUnitTester().ModTest) [0x107807d46]
??:? core.runtime.UnitTestResult agora.test.Base.customModuleUnitTester() [0x10780712d]
```

The failing test is this: https://github.com/dlang/phobos/blob/8fe6a2762ae1b8b5ea906f15049a4373afbb45b5/std/typecons.d#L3000-L3015

There's so many things wrong here that I don't even know where to start.

First, why on god's green earth are we putting unittests with explicit template instantiation inside a *template* ? `unittest` inside templates are a terrible "feature", and I've never seen a correct usage of it, which would be a unittest that relies on the user-instantiated instance to test that its expectation still holds.
I'm pretty sure the rationale is "for documentation", and we should really re-think this, because those unittests are being run from *every single module that instantiate Nullable*. Actually, they are being run for *every instantiation of Nullable*.

Second, as the title of the post says, *do not catch Error*. Catching `Error` is a sin, a crime, and every catch of `Error` derivative should have you outraged. It is in direct violation of the specs. And obviously, it's also incompatible with scheme that do not throw an `Error`.

The code was introduced 5 years ago (https://github.com/dlang/phobos/pull/3114) and has been sitting there since then.

Another thing that contributed to our issue was that we didn't really knew where this instantiation came from. The module that we see in the stack trace, Serializer, has *no* reference to `Nullable`, and doesn't import `typecons`. It does import some other modules, so I'm conjecturing that the dependencies' unittests are executed first, but I'll take that up with the LDC team after I investigate.

In any case, there's two things I really wish we fixed:
- Either ban unittests in templates, deprecate them, or find a way to make this mistake hard to do;
- Deprecate the ability to catch `Error` in code. If it's not supposed to be caught, it shouldn't be so easy to do it;

Druntime catches exceptions on upper level, logs and aborts there.

On 10/22/20 12:04 AM, Mathias LANG wrote:
> Hi everyone,
> As some of you may know, my company is developing a blockchain where the node is written solely in D (https://github.com/bpfkorea/agora).
> One of the reason to pick D was the ease of prototyping and the C++ compatibility (we need to interface with modern C++ code, e.g. C++14 and 17).
> 
> Since tests matters, we have developed a testing framework which use(d) `std.concurrency` to simulate a network in memory (1 thread == 1 node), which works *very well*.
> 
> However, there was a little gotcha: When an error happens, the thread would crash, and we would get a timeout in the main thread, and sometimes would not be able to print the logs. We use Ocean's Logger, which is derived from Tango's, which allow to essentially set the output, and in test mode we set it to a large static array acting as a circular buffer.
> 
> One of our developer was hit particularly hard by this while implementing a new feature, so added a bunch of `writeln`. After discussion with the team, the solution seemed obvious: Let's use `assertHandler` !
> 
> For those of you not familiar with it, here is `assertHandler`'s documentation: https://dlang.org/library/core/exception/assert_handler.html
> It essentially allows to define a custom behavior on `assert`. It is slightly redundant with other facilities to do so, such as `-checkaction`, which allow to customize the behavior at the compiler level.
> 
> That's when things went bad. Since we wanted a core dump, we added an `abort` in our handler, expecting that it will only trigger when we have bugs. And our tests started to fail. And that's where the rant begins.
> 
> We got hit by this (reduced) stack trace:
> ```
> [/usr/local/Cellar/ldc/1.23.0/include/dlang/ldc/std/typecons.d:2979] Assertion thrown during test: Called `get' on null Nullable!int.
> ----------------
> ??:? nothrow void agora.test.Base.testAssertHandler(immutable(char)[], ulong, immutable(char)[]) [0x107807564]
> ??:? onAssertErrorMsg [0x10838ea21]
> ??:? _d_assert_msg [0x10838ee35]
> ??:? inout pure nothrow ref @property @nogc @safe inout(int) std.typecons.Nullable!(int).Nullable.get() [0x1079146d5]
> ??:? inout pure nothrow ref @property @nogc @safe inout(int) std.typecons.Nullable!(int).Nullable.get_() [0x107911958]
> ??:? pure nothrow @nogc @safe int std.typecons.Nullable!(immutable(char)[]).Nullable.__unittest_L2998_C13_1().__dgliteral1() [0x10791193c]
> ??:? pure void std.exception.assertThrown!(core.exception.AssertError, int).assertThrown(lazy int, immutable(char)[], immutable(char)[], ulong) [0x10791177b]
> ??:? pure void std.typecons.Nullable!(immutable(char)[]).Nullable.__unittest_L2998_C13_1() [0x1079116b8]
> ??:? agora.common.Serializer.__unittest [0x107bf97d8]
> ??:? void agora.test.Base.customModuleUnitTester().runTest(agora.test.Base.customModuleUnitTester().ModTest) [0x107807d46]
> ??:? core.runtime.UnitTestResult agora.test.Base.customModuleUnitTester() [0x10780712d]
> ```
> 
> The failing test is this: https://github.com/dlang/phobos/blob/8fe6a2762ae1b8b5ea906f15049a4373afbb45b5/std/typecons.d#L3000-L3015 
> 
> 
> There's so many things wrong here that I don't even know where to start.
> 
> First, why on god's green earth are we putting unittests with explicit template instantiation inside a *template* ? `unittest` inside templates are a terrible "feature", and I've never seen a correct usage of it, which would be a unittest that relies on the user-instantiated instance to test that its expectation still holds.

I used it in dcollections quite a bit as a way to ensure all integer based types were tested.

The advantage here is, I wanted to run the same tests on every integer type. But I wanted the tests to be close to the functions they were testing.

But I think this can be done a different way, and it is *really* hard to get this right. Plus, they are going to be run when you instantiate them elsewhere, which is a problem.

> I'm pretty sure the rationale is "for documentation", and we should really re-think this, because those unittests are being run from *every single module that instantiate Nullable*. Actually, they are being run for *every instantiation of Nullable*.

Yeah, that is bad. This unittest should be moved outside the template.

> 
> Second, as the title of the post says, *do not catch Error*. Catching `Error` is a sin, a crime, and every catch of `Error` derivative should have you outraged. It is in direct violation of the specs. And obviously, it's also incompatible with scheme that do not throw an `Error`.

I disagree with this. How would you test that the assert error is thrown? It seems that your specific case is being hampered by this, but can't you just look at the parameters to see whether you should handle it or not? Like check the file/line and do the normal thing if it's from phobos?

> Another thing that contributed to our issue was that we didn't really knew where this instantiation came from. The module that we see in the stack trace, Serializer, has *no* reference to `Nullable`, and doesn't import `typecons`. It does import some other modules, so I'm conjecturing that the dependencies' unittests are executed first, but I'll take that up with the LDC team after I investigate.
> 
> In any case, there's two things I really wish we fixed:
> - Either ban unittests in templates, deprecate them, or find a way to make this mistake hard to do;

What we need is a way to make a static unittest that serves to provide documentation, but is run as if it were outside the template.

> - Deprecate the ability to catch `Error` in code. If it's not supposed to be caught, it shouldn't be so easy to do it;

Disagree. This use case you have is very obscure, and it's much less obscure to verify an Error is thrown in a unittest.

-Steve

On Thursday, 22 October 2020 at 14:36:11 UTC, Steven Schveighoffer wrote:
> On 10/22/20 12:04 AM, Mathias LANG wrote:
>> 
>> First, why on god's green earth are we putting unittests with explicit template instantiation inside a *template* ? `unittest` inside templates are a terrible "feature", and I've never seen a correct usage of it, which would be a unittest that relies on the user-instantiated instance to test that its expectation still holds.
>
> I used it in dcollections quite a bit as a way to ensure all integer based types were tested.
>
> The advantage here is, I wanted to run the same tests on every integer type. But I wanted the tests to be close to the functions they were testing.
>
> But I think this can be done a different way, and it is *really* hard to get this right. Plus, they are going to be run when you instantiate them elsewhere, which is a problem.

The point was, if the unittest does not depend on the template parameters, it shouldn't be in the template.
So the following is a correct use of the feature:
```
struct Nullable (T)
{
    unittest
    {
        Nullable inst;
        assert(inst.isNull());
    }
}
```
While this is not:
```
struct Nullable (T)
{
    unittest
    {
        Nullable!int inst;
        assert(inst.isNull());
    }
}
```

Because the first one will run for the user instantiation while the second one is independent on the user instantiation (but will run anyway).

>> 
>> Second, as the title of the post says, *do not catch Error*. Catching `Error` is a sin, a crime, and every catch of `Error` derivative should have you outraged. It is in direct violation of the specs. And obviously, it's also incompatible with scheme that do not throw an `Error`.
>
> I disagree with this. How would you test that the assert error is thrown? It seems that your specific case is being hampered by this, but can't you just look at the parameters to see whether you should handle it or not? Like check the file/line and do the normal thing if it's from phobos?

You do not test for assert error. Assert errors are logic errors, and if they get triggered, all bets are off. Of course, one might find this impractical, because `get` cannot be tested this way. Perhaps that's a sign that `Nullable` should use exceptions, and not `assert`, for this.

Regarding the workaround: We currently do this, however, testing for it in a cross-platform and generic way is non-trivial. We can do `endsWith` but we have to account for different paths. And there's no guarantee that we won't have false-positive (e.g. ourcustomlib.std.typecons).

>> In any case, there's two things I really wish we fixed:
>> - Either ban unittests in templates, deprecate them, or find a way to make this mistake hard to do;
>
> What we need is a way to make a static unittest that serves to provide documentation, but is run as if it were outside the template.

That's something that Andrej Mitrovic brought up during the following discussion. While it would fix this specific instance, the default would still be bad, so I'm not really convinced.

>> - Deprecate the ability to catch `Error` in code. If it's not supposed to be caught, it shouldn't be so easy to do it;
>
> Disagree. This use case you have is very obscure, and it's much less obscure to verify an Error is thrown in a unittest.
>
> -Steve

You disagree with the spec then. The spec says that the stack might not be unwound if an `Error` is thrown. Walter even raised a PR to actually do the optimization the spec promises. In his words:

> This PR is still the right thing to do. Attempting to unwind when an Error is thrown is a bad idea, and leads to undefined behavior. Essentially, code which relies on such unwinding needs to be fixed.

Source: https://github.com/dlang/dmd/pull/6896#issuecomment-362855793

On Thu, Oct 22, 2020 at 04:04:26AM +0000, Mathias LANG via Digitalmars-d wrote: [...]
> First, why on god's green earth are we putting unittests with explicit template instantiation inside a *template* ? `unittest` inside templates are a terrible "feature", and I've never seen a correct usage of it, which would be a unittest that relies on the user-instantiated instance to test that its expectation still holds.
>
> I'm pretty sure the rationale is "for documentation", and we should really re-think this, because those unittests are being run from *every single module that instantiate Nullable*. Actually, they are being run for *every instantiation of Nullable*.
[...]

This is a known issue that has been brought up many times in the past. Basically:

1) A unittest block inside a template is duplicated for *every* instantiation of the template. To alleviate this, you'd need to do something like this:

	template MyTemplate(Args...) {
		auto myFunc(...) { ... }
		static if (Args == ...)
		unittest { ... }
	}

This is not a fool-proof solution, however, because if that specific combination of Args isn't actually instantiated, the unittest is silently ignored (even if it would have triggered a failure due to some bug). To alleviate this, you'd have to insert something like this outside the template body:

	// Make sure unittests get instantiated.
	version(unittest) alias _ = MyTemplate!(...);

Which is, of course, extremely ugly. And definitely not something newcomers would think of.

Another way to solve this is to move the unittest outside the template body. However:

2) Ddoc's unittests require unittest blocks to be attached to the symbol they're documenting. So you pretty much have no choice, unless you use Phobos StdDdoc hack (ugh):

	struct PhobosTemplate {
		auto phobosFunc(...) { ... }

		version(StdDoc) // ugh
		///
		unittest { ... }
	}


Jonathan Davis & myself have proposed in the past an enhancement to solve this problem: have some way of marking a unittest block inside a template as single-instance only. Something like this:

	template MyTemplate(T)
	{
		auto myFunc(...) { ... }

		/// This is instantiated once per template instantiation
		unittest {
			pragma(msg, T.stringof);
		}

		/// This is instantiated only once, ever
		/// (The use of 'static' is hypothetical syntax, it can
		/// be anything else that marks the unittest as
		/// single-instance)
		static unittest {
			//pragma(msg, T.stringof); // Error: cannot reference T here

			// Have to explicitly instantiate the template
			// with the arguments you want to test for
			alias U = MyTemplate!int;
		}
	}

However, so far no action has been taken on this front besides the proposal.


T

-- 
"Maybe" is a strange word.  When mom or dad says it it means "yes", but when my big brothers say it it means "no"! -- PJ jr.

On Thursday, 22 October 2020 at 04:04:26 UTC, Mathias LANG wrote:
<snip>
It just occurred to me that putting `version(none)` on those tests *might* disable them while also ensuring that the tests show up in the documentation (which was the idea behind putting them in there), but I'm not sure about that. I know that something like `static if (false) { declarations... }` will omit them.

On 22.10.20 18:05, Mathias LANG wrote:
>>
> 
> You disagree with the spec then. The spec says that the stack might not be unwound if an `Error` is thrown. Walter even raised a PR to actually do the optimization the spec promises. In his words:
> 
>> This PR is still the right thing to do. Attempting to unwind when an Error is thrown is a bad idea, and leads to undefined behavior. Essentially, code which relies on such unwinding needs to be fixed.
> 
> Source: https://github.com/dlang/dmd/pull/6896#issuecomment-362855793

It's not that simple. The compiler itself catches AssertError when checking preconditions.

On 22.10.20 18:26, Timon Gehr wrote:
> On 22.10.20 18:05, Mathias LANG wrote:
>>>
>>
>> You disagree with the spec then. The spec says that the stack might not be unwound if an `Error` is thrown. Walter even raised a PR to actually do the optimization the spec promises. In his words:
>>
>>> This PR is still the right thing to do. Attempting to unwind when an Error is thrown is a bad idea, and leads to undefined behavior. Essentially, code which relies on such unwinding needs to be fixed.
>>
>> Source: https://github.com/dlang/dmd/pull/6896#issuecomment-362855793
> 
> It's not that simple. The compiler itself catches AssertError when checking preconditions.

Also, code that "relies on such unwinding" can be annotated @safe and the compiler won't complain.

The issue is not that people disagree with the specification, it's that the specification disagrees with itself.

On Thu, Oct 22, 2020 at 04:05:51PM +0000, Mathias LANG via Digitalmars-d wrote:
> On Thursday, 22 October 2020 at 14:36:11 UTC, Steven Schveighoffer wrote:
> > On 10/22/20 12:04 AM, Mathias LANG wrote:
[...]
> > > Second, as the title of the post says, *do not catch Error*.
[...]
> > I disagree with this. How would you test that the assert error is thrown? It seems that your specific case is being hampered by this, but can't you just look at the parameters to see whether you should handle it or not? Like check the file/line and do the normal thing if it's from phobos?
> 
> You do not test for assert error. Assert errors are logic errors, and if they get triggered, all bets are off. Of course, one might find this impractical, because `get` cannot be tested this way. Perhaps that's a sign that `Nullable` should use exceptions, and not `assert`, for this.
[...]

+1.  Catching Errors is an anti-pattern and a code smell.  The usual argument is, I want to test that my asserts are working.  The problem with that is that it quickly falls into the paradox of who watches the watchers.  The next thing you know, I'm asking for a way to test that my code that catches asserts are actually catching asserts.  Next year I'll be asking for code to test my code that tests code that catches asserts. It never ends.

The point is, if your asserts are so complex you have to write unittests for them, then there's something fundamentally wrong with them, and they should be rewritten. Like with exceptions, as Mathias says.

It's a different story altogether if druntime uses catching of Errors as a low-level mechanism to implement certain language semantics.  I know contracts in a class hierarchy are implemented that way for certain reasons, for example.  But that's stuff under the hood, that should not be done in user code, ever.


T

-- 
Obviously, some things aren't very obvious.

On Thu, Oct 22, 2020 at 04:19:30PM +0000, Meta via Digitalmars-d wrote:
> On Thursday, 22 October 2020 at 04:04:26 UTC, Mathias LANG wrote:
> <snip>
> It just occurred to me that putting `version(none)` on those tests
> *might* disable them while also ensuring that the tests show up in the
> documentation (which was the idea behind putting them in there), but
> I'm not sure about that. I know that something like `static if (false)
> { declarations... }` will omit them.

I'm pretty sure ddocs on version(none) symbols are skipped. So this
won't work.


T

-- 
"If you're arguing, you're losing." -- Mike Thomas