(Re)Introduce Throwable.message

[nemanja-boric-sociomantic]

This patch introduces Throwable.message method which returns
Throwable.msg's contents. In order to prevent name clashes with the
code already defining message method inside their own subclasses of
Throwable, Throwable.message is marked as @__future, providing
"forward deprecation" message to users.

[dlang-bot]

Thanks for your pull request, @nemanja-boric-sociomantic! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

[nemanja-boric-sociomantic]

cc @mathias-lang-sociomantic @mihails-strasuns-sociomantic

[MartinNowak]

Don't think druntime is the right place to test the implementation of @__future. There should be proper tests in dmd (hopefully already).

[leandro-lucarella-sociomantic]

Thanks for taking this off of my hands :D

[nemanja-boric-sociomantic]

@MartinNowak They already are, I'm not testing dmd's output, but the Throwable's behavior. I can remove the test case, though.

[leandro-lucarella-sociomantic]

Is there anything else missing?

[nemanja-boric-sociomantic]

No, I think this should be it. @MartinNowak @WalterBright ?

[nemanja-boric-sociomantic]

Oh, except the question if the test is suitable here.

[PetarKirov]

After rereading the whole discussion in #1445, the main concerns that remain (the breaking change issue should be addressed with use @__future), as far as I know, are:

• Function attributes:
  • pure is not an option since it precludes the use of statically allocated buffers.
  • nothrow should be easily doable - I don't see any reason why anyone would need to throw an exception, and exceptions from callees (e.g. arising from formatting functions) are easily catchable.
  • @nogc - the main motivation is avoiding unnecessary allocations, so why not advertise this?
  • @safe - can't decide on this one, there are pros & cons either way.
  • annotating the return type with scope - (assuming the signature modulo attributes remains the same, if not see the point below) this imposes a limitation on the users, but allows reusing buffers more safely since users won't be able to assign the result to a variable with longer lifetime (modulo bugs in the current implementation of -dip1000). This should work well with your wide use of scope classes.
• Changing the signature to accept a sink delegate. It seems this was the biggest point of contention in the previous discussion, so I hope we can quickly reach an agreement (whatever it would be) here.
  • Cons:
    • It won't work without additional work from your side with the rest of libraries, e.g. https://github.com/sociomantic-tsunami/ocean/blob/v2.x.x/src/ocean/core/Exception.d#L189
    • May look complicated to beginners
  • Pros for void writeMessage(scope void delegate(const(char)[] msg) sink) (*):
    • A strict super set of the alternatives: log(ex.message()) can be expressed as ex.writeMessage(msg => log(msg))
    • More efficient
    • More idiomatic
    • Easier to avoid allocations in @nogc core. Concatenating two strings in writeMessage would be just two calls to sink().
    • Smaller probability for being a breaking change (pure speculation on my part)

(*) I would prefer if the signature would be void writeMessage(scope void delegate(scope const(char)[] msg) sink) (the difference being that the delegate takes a scope slice), which would help enforce safe usage, but it's not workable at the moment since very few code in the wild is scope-proof now. Even std.stdio.writeln(str) is not!

[mihails-strasuns-sociomantic]

Whatever maintainers agree on.

I would myself prefer to avoid @nogc and don't see how it can be scope without unnecessary restricting buffer lifetime (also we are definitely not going to use scope in our codebase in any near future).

Keep in mind that D override rules allow to override with more restrictive attributes, but not the other way around.

[leandro-lucarella-sociomantic]

But I want to emphasize that we need a decision. We've said before that at this point we don't really care much about what the solution is as long as it fixes our problem. We are already proxying all our code reading exception messages through a function to be able to adapt effortless to what the upstream solution is.

We've been going around this problem for years now (literally), we even thought about mechanisms (and wrote a DIP) to avoid any code breakage when implementing this (incidentally the DIP wasn't really implemented, a different solution was implemented but it still solves our problems so at this point we don't even want to fight to get the DIP properly implemented).

Please make up your mind soon so we can get over this and finally start moving forward... :)

[leandro-lucarella-sociomantic]

Based on the added label, I think it's fair to ping @andralex here...

[WalterBright]

Need a test case to turn the red to green.

[leandro-lucarella-sociomantic]

I see all tests in green, or do you mean the green of a review approval?

Also, there are some test cases already included, is there anything in particular you see missing?

[WalterBright]

Line 1745 above is red. It isn't executed because all the test cases override Throwable.message(). Need a test case that does not override it, and just calls it.

[PetarKirov]

@leandro-lucarella-sociomantic There is a CodeCov browser extension, which overlays the unittest coverage on top of the diff for pull requests. I think what Walter is saying is that this line doesn't show up as covered by a unittest.

[WalterBright]

@ZombineDev is right. Here it is: https://github.com/codecov/browser-extension Please install it!

[Burgos]

Thanks, had no idea there's an extension, will do it today!

[nemanja-boric-sociomantic]

I've started looking into this and it feels something is off with the code coverage - because this is already covered by: https://github.com/dlang/druntime/pull/1895/files#diff-8289be999021da628af8f313e9992573R4 (I even confirmed this with printouts in the line - it sure gets triggered)

Could it be that source of the problem is that code coverage doesn't collect ones from the test suite? IIRC, we have the similar problem in sociomantic's ocean, about the code coverage overriding each other, I wonder if it same here.

[nemanja-boric-sociomantic]

I've added the unittest, let's see if that helps

[nemanja-boric-sociomantic]

Ok, green now!

[PetarKirov]

[..] it feels something is off with the code coverage - because this is already covered [..]

Not sure if there's a bugzilla issue about this (if not there should be), the reason for this initially not appearing covered is that coverage in external test files is essentially ignored. I think it is a build-system level issue, though I haven't looked in detail.

[leandro-lucarella-sociomantic]

❤️