[RFC] Issue 19087 - final switch cannot be used in -betterC by wilzbach · Pull Request #2274 · dlang/druntime

wilzbach · 2018-08-15T23:44:18Z

Not fully sure about this as the SwitchError does contain a lot more information.
Maybe we should version this and only use assert in betterC and otherwise throw the exception?

dlang-bot · 2018-08-15T23:44:21Z

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	19087	normal	`final switch` cannot be used in -betterC

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 + druntime#2274"

JinShil · 2018-08-15T23:51:34Z

@WalterBright has already affirmed to me (twice actually) in private e-mail exchange that he is OK replacing Errors with asserts even in non-betterC code.

Me: I could use version(D_BetterC) to generate a runtime assert instead of an Error for -betterC. Would that be OK, or do you have something else in mind?
Walter: Seems like just having it assert() would solve the problem.

@wilzbach: Catching Error is defined to result in undefined behavior. We don't need to cater for such code. So you can use assert?
Me: Is that right, can I just make the Error an assert? I think that would solve all problems.
Walter: I thought I already suggested to just use assert(), but perhaps I sent the email awry. Anyhow, I agree with Seb. It should have been implemented that way the first time around.

IMO the decision has been made and direction for the future established.

This applies equally to #2268

JinShil · 2018-08-16T00:03:27Z

src/core/exception.d

 extern (C) void onSwitchError( string file = __FILE__, size_t line = __LINE__ ) @safe pure nothrow
 {
-    throw new SwitchError( file, line, null );
+    assert(0, "No appropriate switch clause found");


One of the things I'd really like to do here, however, is make use of the __FILE__ and __LINE__ parameters in the error message. That was the only reason I hadn't fixed this bug myself. Given that such a trivial thing is actually quite difficult given the limitations discussed in #2268, however, I'd be willing to merge this PR as is, as long as there is a bugzilla issue filed to improve the error message.

Unfortunately, actually building a message with the file and line number gets to be a bit of a pain when you can't use format from Phobos, and something like sprintf isn't pure (though maybe it should be).

jmdavis · 2018-08-16T00:23:37Z

They key issue with replacing Errors with assertions is that assertions are only around without -release. assert(0, ...) does not have that problem, since it inserts a HLT instruction with -release instead of getting compiled out, but we should avoid replacing Errors with assertions that that don't evaluate to false at compile-time. And actually, using assert(0, ...) is arguably better than an Error anyway, since then the program gets killed at the point of failure, and you still have the full program state if there's a core dump. I'm actually inclined to argue that having Errors in the language was a mistake. So, personally, I have no problem with replacing Errors with assert(0, ...).

JinShil · 2018-08-16T00:29:19Z

src/core/exception.d

 {
    // Consider making this a compile time check.
-    throw staticError!SwitchError(file, line, null);
+    assert(0, "No appropriate switch clause found");


Another thing we need is the ability to test runtime assertions in unittests. Any ideas?

It generally works just fine to catch an AssertError in a unit test. That being said, I don't really see anything to test here. The stuff that was in the SwitchError tests all had to do with testing that the members of SwitchError were initialized correctly. I guess that you could do the same with an AssertError, but unless -unittest makes assert(0, ...) still throw an AssertError when the assertion isn't directly in a unittest block when compiling with -release, then you couldn't test it in -release. And honestly, I don't think that testing that the exception is initialized with all of the right values is all that valuable. What you generally need to test is that it's thrown when it's supposed to and not thrown when it's not supposed to, and that would involve testing code that actually triggered __switch_errorT(). All around though, it does get pretty weird to try to test anything that involves assert(0, ...), since pretty much by definition, that code is never supposed to be reached, and it's a serious bug in your program if it is.

From the spec about unit test blocks and assert expressions:

“Unlike AssertExpressions used elsewhere, the assert is not assumed to hold, and upon assert failure the program is still in a defined state.”

“AssertExpression has different semantics if it is in a unittest or in contract.”

The latter suggests to me that we could change the implementation to throw an exception instead of an error when run inside a unit test block.

The latter suggests to me that we could change the implementation to throw an exception instead of an error when run inside a unit test block.

Huh? How would that help us? Also final switch wouldn't be nothrow anymore.

I don't know if it's in the spec but it's been pointed out that Errors should not be caught. Since assert throws an AssertError it's not possible to create a unit test runner that continues after a failed test.

Haha spec != implementation. It works quite well to catch an AssertError and lots of code in Phobos depends on this, e.g.

ag "collectException.*Error" std/exception.d 168: assert(collectExceptionMsg!AssertError(assertNotThrown!StringException( 176: assert(collectExceptionMsg!AssertError(assertNotThrown!StringException( 180: assert(collectExceptionMsg!AssertError(assertNotThrown!StringException( 184: assert(collectExceptionMsg!AssertError(assertNotThrown!StringException( 311: assert(collectExceptionMsg!AssertError(assertThrown!StringException( 706: auto e = collectException!Error(enforceEx!OutOfMemoryError(false)); 714: auto e = collectException!Error(enforceEx!OutOfMemoryError(false, "file", 42)); 770: assert(collectException!RangeError(a[4], b)); 1843: assert(throwRangeError().ifThrown(0).collectException!RangeError() !is null); 1844: assert(throwRangeError().ifThrown(e=>0).collectException!RangeError() !is null);

ag "catch.*Error" std/typecons.d 4302: catch (Error e) {} std/bitmanip.d 708: catch (AssertError ae) 712: catch (AssertError ae) std/datetime/package.d 293: catch (AssertError e) 320: catch (AssertError e) std/range/package.d 720: catch (AssertError unused) std/exception.d 200: catch (AssertError) assert(0); 206: catch (AssertError) assert(0); 212: catch (AssertError) assert(0); 218: catch (AssertError) assert(0); 227: catch (AssertError) thrown = true; 238: catch (AssertError) thrown = true; 249: catch (AssertError) thrown = true; 261: catch (AssertError) thrown = true; 327: catch (AssertError) assert(0); 334: catch (AssertError) assert(0); 341: catch (AssertError) assert(0); 349: catch (AssertError) assert(0); 356: catch (AssertError) 366: catch (AssertError) 376: catch (AssertError) 386: catch (AssertError) std/experimental/allocator/gc_allocator.d 69: catch (OutOfMemoryError) std/parallelism.d 4830: catch (ParallelForeachError e) {}

Haha spec != implementation. It works quite well to catch an AssertError and lots of code in Phobos depends on this, e.g

Yeah, I know. I do that as well. But I would feel better if it was an Exception instead of an Error.

wilzbach · 2018-08-16T08:42:08Z

@WalterBright has already affirmed to me (twice actually) in private e-mail exchange that he is OK replacing Errors with asserts even in non-betterC code.
...
One of the things I'd really like to do here, however, is make use of the FILE and LINE parameters in the error message. That was the only reason I hadn't fixed this bug myself. Given that such a trivial thing is actually quite difficult given the limitations discussed in #2268

I was thinking a bit more about this and I now changed it too:

version (D_Exceptions)
    throw new SwitchError( file, line, null );
else
    assert(0, "No appropriate switch clause found");

So that until we figure out a nice way to do error formatting in -betterC, normal D code will see the more informative error.

JinShil · 2018-08-16T09:18:12Z

I was thinking a bit more about this and I now changed it to.

I'm OK with the change. I'm also OK with leaving it as an assert with a vague error message for both -betterC and non-betterC builds. But, either way, please file a bugzilla issue that the error message needs updating.

wilzbach · 2018-08-16T09:44:49Z

But, either way, please file a bugzilla issue that the error message needs updating.

https://issues.dlang.org/show_bug.cgi?id=19172

n8sh · 2018-08-17T11:29:26Z

Aren't assert messages omitted anyway when compiling in release mode?

JinShil · 2018-08-17T11:38:33Z

Aren't assert messages omitted anyway when compiling in release mode?

No, See item #2 at https://dlang.org/spec/expression.html#assert_expressions

jacob-carlborg · 2018-08-17T11:52:05Z

Aren't assert messages omitted anyway when compiling in release mode?
No, See item #2 at https://dlang.org/spec/expression.html#assert_expressions

There's no message printed in release mode:

$ cat main.d
void main()
{
    assert(false);
}
$ dmd -release main.d
$ ./main
Illegal instruction: 4

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

jmdavis · 2018-08-17T11:52:57Z

Specifically, if the condition is known to be false, then the compiler turns the assertion into a HLT instruction with -release. So, reaching that line will kill your program, and there will be no message.

JinShil · 2018-08-17T11:55:32Z

Ah Right! That's actually #11 at https://dlang.org/spec/expression.html#assert_expressions

jacob-carlborg · 2018-08-17T12:14:47Z

#2 is for when indicating some code path is unreachable, i.e.

int foo(int a)
{
    if (a == 3)
        return 1;

    assert(false);
}

In the above example the compiler will not complain that there's no return after the if since assert(false) is unreachable code.

JinShil · 2018-08-19T07:38:45Z

src/core/exception.d

 extern (C) void onSwitchError( string file = __FILE__, size_t line = __LINE__ ) @safe pure nothrow
 {
-    throw new SwitchError( file, line, null );
+    version (D_Exceptions)


I suggest adding a comment here that this is temporary, and that once we get either a pure malloc, or an alloca for -betterC that these should be changed to an assert for both cases. Either that, or file a bugzilla issue with the same.

This discussion doesn't need to hold up resolving issue #19087.

n8sh · 2018-10-22T02:46:13Z

Bumping. I don't see a reason this shouldn't be merged.

PetarKirov

I don't see a reason this shouldn't be merged.

Me too!

That's a discussion that doesn't need to block progress on this issue.

wilzbach requested review from CyberShadow, MartinNowak, WalterBright and jmdavis as code owners August 15, 2018 23:44

dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Aug 15, 2018

JinShil reviewed Aug 16, 2018

View reviewed changes

wilzbach force-pushed the fix-19087 branch from ccc3ff1 to 9851e29 Compare August 16, 2018 08:37

JinShil mentioned this pull request Aug 16, 2018

[RFC] Add pure fail-fast assertMalloc, assertCalloc, assertRealloc #2276

Closed

n8sh approved these changes Aug 16, 2018

View reviewed changes

JinShil previously requested changes Aug 19, 2018

View reviewed changes

Fix Issue 19087 - final switch cannot be used in -betterC

9cbdab8

n8sh changed the title ~~[RFC] Issue 19087 - cannot be used in -betterC~~ [RFC] Issue 19087 - final switch cannot be used in -betterC Oct 22, 2018

n8sh force-pushed the fix-19087 branch from 9851e29 to 9cbdab8 Compare October 22, 2018 02:59

PetarKirov approved these changes Oct 22, 2018

View reviewed changes

PetarKirov added the auto-merge label Oct 22, 2018

PetarKirov added auto-merge and removed auto-merge labels Oct 22, 2018

PetarKirov merged commit 7f62a68 into dlang:master Oct 22, 2018

wilzbach deleted the fix-19087 branch January 31, 2019 07:03

wilzbach mentioned this pull request Jan 31, 2019

final switch can be used in -betterC dlang/dlang.org#2562

Merged

Uh oh!

Comments

Conversation

wilzbach commented Aug 15, 2018

Uh oh!

dlang-bot commented Aug 15, 2018

Bugzilla references

Testing this PR locally

Uh oh!

JinShil commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JinShil Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdavis commented Aug 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Aug 16, 2018

Uh oh!

JinShil commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Aug 16, 2018

Uh oh!

n8sh commented Aug 17, 2018

Uh oh!

JinShil commented Aug 17, 2018

Uh oh!

jacob-carlborg commented Aug 17, 2018

Uh oh!

jmdavis commented Aug 17, 2018

Uh oh!

JinShil commented Aug 17, 2018

Uh oh!

jacob-carlborg commented Aug 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n8sh commented Oct 22, 2018

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JinShil commented Aug 15, 2018 •

edited

Loading

JinShil Aug 16, 2018 •

edited

Loading

jacob-carlborg Aug 16, 2018 •

edited

Loading

JinShil commented Aug 16, 2018 •

edited

Loading