Add message() to object.Throwable

[WalterBright]

Having msg as a user accessible field turns out to make customized exceptions more difficult than they should be. Turning it into a property function can break existing code. So added an overridable message function instead, and undocumented msg.

[dnadlinger]

I won't get into all the the bikeshedding and deprecation process discussions that this PR will inevitably trigger, but let me point out that the method should probably take a sink delegate instead of returning the string such as to enable the user to avoid GC allocations.

[WalterBright]

The sink delegate method is incompatible with using ranges.

[dnadlinger]

?!

It allows a strict superset of what the string-based signature does. Please explain.

[mihails-strasuns-sociomantic]

Few comments:

1. The change has been done by Sociomantic request to allow porting existing mutable+reusable exception hierarchy
2. I also believe sink delegate approach is superior but plain const(char)[] message() is good enough for vast majority of our cases so I won't argue with @WalterBright decision
3. Instead undocumenting msg it may be better to replace documentation with one that explains why keep using it is a bad idea - but it is not important

[mathias-lang-sociomantic]

The sink delegate method is incompatible with using ranges.

I'm not sure I fully understand that though. How do you plan to make message range compatible ?

[jmdavis]

Wouldn't it make more sense to turn msg into a non-property function that returns string and add an overload that takes a scoped delegate like the toString overloads that work with output ranges do? Or I suppose that it could still be an @property function, but that would mean that the sink overload would work via assignment rather than a normal function call, for better or worse.

Then if there's concern about that breaking code, another overload which takes string as an argument could be used for a setter, and we can deprecate it so that existing code will work as-is while encouraging folks to not set msg any longer.

The only code that would break that way would be code that passed msg by ref or took its address, neither of which is very likely to be happening in much code (if any).

And if we go that route, then pretty much only the code that declares exceptions will have to change, whereas with the current proposal, we'd be looking to change all code that uses msg (which is obviously more code and thus more breaking changes, even if they're not immediate breaking changes).

[jmdavis]

There's also this related PR that I never merged, because there wasn't enough feedback on it:

#1411

[jmdavis]

Wouldn't it make more sense to turn msg into a non-property function that returns string and add an overload that takes a scoped delegate like the toString overloads that work with output ranges do? Or I suppose that it could still be an @Property function, but that would mean that the sink overload would work via assignment rather than a normal function call, for better or worse.

Or even better, the delegate overload could just have a different name (e.g. writeMsg) so that msg could be a proper property function (with the setter deprecated). And it is pretty weird to have a function that's really supposed to be a property and has a noun for a name take a delegate to write to rather than returning a value or being set with a value, so in that regard, having a separate function for it that has a verb in it makes more sense.

[jmdavis]

So, if I were to propose a specific course of action, it would be to go with #1411 rather than this PR but deprecate the setter overload of msg and add writeMsg which takes a scoped delegate for the cases where it's desirable to avoid allocating a string. I think that such a course of action would solve this problem with less code breakage.

[mihails-strasuns-sociomantic]

No, in absence of const(char)[] message() sink based approach alone wouldn't work for us. Main use case is the code like this:

class CustomEx : Exception
{
    private char[] reusable_buffer;
    override const(char)[] message () { return this.reusable_buffer; }
}

catch (Exception e)
{
    log(e.message());
}

Sink-based version would still have to allocate here unless logging facility is changed to support it (hardly an option). I'd love to see both overloads but plain const(char)[] message() is bare minimum.

All talk about breaking changes is hardly relevant because nothing gets deprecated or changed - it is all purely additive and will only matter for new code which wants to support the facility.

[jmdavis]

Sink-based version would still have to allocate here unless logging facility is changed to support it (hardly an option). I'd love to see both overloads but plain const(char)[] message() is bare minimum.

Hmmm. The problem with const(char)[] is that it's going to lead to unnecessary allocations in many cases, because the return value will have to be duped to get a string. Which logging facilities are the problem? std.experimental.logger? If it's that, then I would very much expect that changing the logging facilities to be an option, though in general, I would expect most stuff to be using toString, not msg or message, in which case, that bumps up against the general problem of what we're going to do with toString. The scoped delegate overload has been our best solution for that thus far, though Walter and Andrei were talking about having some sort of toChars which returned a range. However, I have no idea how we're going to do that without allocating a range on the heap, since it can't be a templated function, and so we can't overload the range by changing its return type, just by using inheritance.

All talk about breaking changes is hardly relevant because nothing gets deprecated or changed - it is all purely additive and will only matter for new code which wants to support the facility.

Having both msg and message is just going to lead to bugs in the long run, since msg obviously won't work correctly if message has been overridden. If we're going to replace msg, we really should deprecated it.

[mihails-strasuns-sociomantic]

it's going to lead to unnecessary allocations in many cases, because the return value will have to be duped to get a string

Only because some libraries are ridiculous in designing string-based API in the first place (terrible thing to do). I can't express how much I hate toString for this very reason - the fact it returns string resulted in days of extra (and completely unnecessary) work for me during porting.

Which logging facilities are the problem?

Tango logger + our extensions. Though std.experimental.logger also doesn't support it per my knowledge. Anyway, changing all that is hardly an option, if this change is rejected I will simply keep using custom patched runtime instead of upstream one - the whole point of this PR is to try to make vanilla D2 usable for our applications without major redesigns.

If we're going to replace msg, we really should deprecated it.

I don't think such breakage (assuming you will eventually remove it at all) is justified compared to minor issue of wrong exception formatting in case of msg vs message mismatch. Most likely trying to do so would result in major fightback and whole change being banned - and I'd really like to be able to use vanilla D2.

[dnadlinger]

Seems like a DIP is in order here, as for any other contentious change to what is effectively the core language.

@mihails-strasuns-sociomantic: Let me point out that in case your CustomEx example is supposed to lazily construct the exception string inside a private buffer (where does that come from, by the way?), it won't work with this PR anyway. The method is marked const in Walter's proposal.

[mathias-lang-sociomantic]

@klickverbot : Lazy construction of the message (or logical const in general) is not the point (the message is constructed at the throw site).

The point is that toString by default prints a stack trace in D2. That stack trace is useful on some occasions, but not everytime. Besides being expensive, you don't want it to cripple your logs with them. As Mihails mentioned, we have a reusable buffer for the message instead of .msg. So when one writes code, (s)he needs to be able to either say "I want the full debugging informations" (toString) or "I just want to know what this exception is about" (file + line + message). And if you catch Exception, there is currently no way to do so.

[dnadlinger]

@mathias-lang-sociomantic: Then why not make an Exception subclass that is inherited by all your exceptions that want a const(char)[] msg instead of a string msg and catch that? Yes, it might be a bit of extra work, and you know that I'm quite a strong proponent of taking the needs of corporate users seriously. But introducing an unforced design error into a core part of D just to allow a certain legacy code base to continue doing its own thing seems like an odd proposition, and one I certainly can't support.

By the way, I think something like this should be a (crude, admittedly) workaround for the issue as you described it:

string toStringMsgOnly(Throwable t) {
  auto oldInfo = t.info;
  scope (exit) t.info = oldInfo;
  t.info = null;
  return t.toString();
}

Of course, you still need to sort out the const/immutable differences for msg. But this is an issue I'm certain must not be solved by just adding an additional, parallel way of doing things.

[dnadlinger]

@mihails-strasuns-sociomantic:

Sink-based version would still have to allocate here unless logging facility is changed to support it (hardly an option)

Not necessarily: e.toMessage(log) (or whatever you want to call it). Of course, this only works as intended if you know that you will do just a single sink call in your implementation.

[mathias-lang-sociomantic]

@mathias-lang-sociomantic: Then why not make an Exception subclass that is inherited by all your exceptions that want a const(char)[] msg instead of a string msg and catch that? Yes, it might be a bit of extra work, and you know that I'm quite a strong proponent of taking the needs of corporate users seriously. But introducing an unforced design error into a core part of D just to allow a certain legacy code base to continue doing its own thing seems like an odd proposition, and one I certainly can't support.

It assumes we have control over all the Exception. As soon as you use a library, it doesn't work, though your toStringMsgOnly does. But then you depend on the implementation of Throwable.toString. I'm sorry but I fail to see how encapsulating the msg member behind a property function is a design mistake.

[dnadlinger]

So… #1141 #1411 it is, then?

[dnadlinger]

@mathias-lang-sociomantic:

It assumes we have control over all the Exception. As soon as you use a library, it doesn't work, […]

You can just use .msg as normal for Exceptions without your special buffer requirements. The library exception types wouldn't be overriding message() either.

The design mistake is having two versions of something that sorta, but not quite, do the same thing, where neither is a replacement for the other nor offers clear advantages (like being able to avoid an allocation for string concatenation, such as when using ranges for constructing the error message).

Yes, msg should have been a property from the get-go (see #1411), but as far as I can see, this is not your main concern, but immutable vs. const is?

[mathias-lang-sociomantic]

@klickverbot the point was :

I'm sorry but I fail to see how encapsulating the msg member behind a property function is a design mistake.

Again, this is basic information hiding. Why do you think using a plain field is better here ?

[dnadlinger]

I don't, see the last paragraph in my reply.

[mathias-lang-sociomantic]

Note: #1141 is probably not the issue you want to link to.

[dnadlinger]

Thanks, fixed the typo.

[mihails-strasuns-sociomantic]

Ok, let's clarify the intention. This isn't about workaround to support bad legacy code - I wouldn't bother anyone about it, adding local hacks is easy (currently patched dmd/druntime is used anyway). Quite the contrary, this is an attempt to fix legacy design mistake in D2 so that we can in future use vanilla upstream package without making our code worse.

Forcing toString and msg to be immutable has viral effect on the code because of transitivity and forces you into unreasonable amount of GC usage. This doesn't work for high performance cases and I am aware of only two "solutions" with existing druntime:

1. Write all code with sink-based overload in mind (actually doesn't work right now because toString was changed to include stacktrace but same applies to proposed message getter). This isn't even supported by Phobos itself - asking D users to rewrite their own code in such way is not reasonable (and rather expensive).
2. Don't do runtime modification of exception messages, stick to literals - AFAIR this was done in vibe.d for most problematic performance cases. Works but at complete sacrifice of helpful logging.

Of course there is also (3) - don't use exceptions :)

At the same time there isn't any practical reason to require immutability for exception messages. It isn't inherently persistent (only used while exception is being handled) and it not inherently shared. Most likely the decision was completely arbitrary (my guess only) in old days when it wasn't truly acknowledged what does it mean to be a string.

Of course, this only works as intended if you know that you will do just a single sink call in your implementation.

It also becomes rather awkward when you try to use it with formatting:

catch (Exception e)
{
    logNoLineBreak("message: ");
    e.message(&logNoLineBreak);
    log("");
    // vs
    log("message: %s", e.message());
}

Again, this isn't about inventing workarounds (I have some as backup plan already). It is about making legitimate use case implementable without workaround. Because I believe our approach is perfectly fine and it is druntime that has issues in this regard.

#1411 is reasonable on its own but unrelated to this PR because here I speak about exceptions that actually get thrown (often several times a second) and processed.

[yebblies]

It also becomes rather awkward when you try to use it with formatting:

It's not clear if you're aware of this from your post, but phobos does support sink-based toString methods in formattedWrite and friends. So it could be:

log("message: %s", e);

if log uses formattedWrite etc internally.

[mihails-strasuns-sociomantic]

It's not clear if you're aware of this from your post

No, I didn't know this, thanks for the hint. It doesn't work with std.experimental.logger though because message gets packed into https://github.com/D-Programming-Language/phobos/blob/master/std/experimental/logger/core.d#L742 - and of course it won't work with Tango logger (I was replying to @klickverbot suggestion how to use sink-based version if logger doesn't support it natively)

[mihails-strasuns-sociomantic]

Ping.

[leandro-lucarella-sociomantic]

Hi, there, I'm sorry to intercede with some information that it might be a bit off-topic in this issue, but I just want to explain the dimension this issue has for us. We are spending a lot of time and effort to move to D2. Our main goal to do this is to be able to contribute to D more, to leave the bubble we are living in. For us from a purely practical point of view, there is very little gain from this move. Even more, given our current memory allocation constraints we probably won't even be able to use phobos or any other library out there. But we want to contribute to make them better to meet our requirements too.

Even more, we want to release some internal libraries, that we think it might be useful for many other users out there, but it makes no sense to do so if we still use D1.

I know is always shitty to ask for support for particular use cases from corporate users, but we believe this will actually make the language better. We don't think we are the only ones with these requirements out there. Is also quite unfortunate that we are still a company, and there are some things we just can't afford (like another rewrite on Tango's logging system to of the rewrite we are already doing to move to D2).

So what I am asking is to try to do an effort to collaborate on finding a solution to this issue. The alternative of keep forking the language (right now we are basically maintaining our own fork of D1) would be a catastrophe for us, after investing so much effort on this, not being able to fully merge with upstream would be heartbreaking. So I'd like to ask you to think very hard about the cost-benefit here. Specially if we are trying to find a good long term solution for high-performance users in general.

A few crude reality facts:

1. We won't use Phobos for a long time, simply because we'll have to prepare phobos to allocate as little as possible (or even not at all) before we can use it. So any solution involving phobos is not an option for us.
2. We can't afford to rewrite Tango logging. At this point, sadly, it would be less costly to maintain a fork of the language than doing that.

So, please don't let this PR die, and please let's try hard to collaborate to find a solution for this that makes all parties happy, and allow Sociomantic to use upstream to be able to contribute back to the community.

Last but not least, I'm not trying to start a big discussion about this (I actually don't want to start a discussion at all!). I'm just telling you what's going on with Sociomantic. I don't want to cause a big fuzz.

Thanks! :)

[andralex]

There is no backwards compatibility issue with this PR.

[jmdavis]

There is no backwards compatibility with this PR.

With this PR, using msg basically becomes broken. If message is not overriden, then using msg is fine, but if it is, then msg will be empty, and the actual message will be in message. So, going forward, all code needs to use message instead of msg and long-term, leaving msg as a public member is just going to result in bugs which won't be found until a program hits an error condition, and the message isn't printed out properly, because the code used msg when it should have used message, much to the frustration of anyone trying to debug that code.

So, no, this PR doesn't strictly speaking break any code, but it does mean that in the long run, any code using msg needs to be changed, and we should seriously consider deprecating msg at some point.

[mihails-strasuns-sociomantic]

I could try look into implementing dfix/dscanner rule for turning msg into message().

[mihails-strasuns-sociomantic]

I could try look into implementing dfix/dscanner rule for turning msg into message().

Sadly, libdparse doesn't seem to support such complicated semantic analysis yet :(

[jmdavis]

Sadly, libdparse doesn't seem to support such complicated semantic analysis yet :(

Well, fortunately, it's an easy fix to change code from msg to message, and most code probably uses toString rather than msg anyway (though I'm sure that plenty of us have code that uses msg). So, I don't think that it's all that big a deal to just deprecate msg when the time comes, but we should wait at least a release so that it's possible to compile code with both master and the last release without getting deprecation messages. And maybe we'll get lucky, and libdparse will be smart enough to make dfix take care of this by then (though probably not).

[dnadlinger]

We need to address the question of pure and @safe before releasing this for the first time: https://issues.dlang.org/show_bug.cgi?id=15507

[MartinNowak]

There are 3 major issues w/ this.

• Adding a new function to our exception hierarchy does break code.
• Adding a virtual function w/o attributes makes it unusable many places (and adding attributes later on would be another breaking change).
• This change goes against an already established idiom to use sinks for avoiding allocations.
  https://github.com/D-Programming-Language/druntime/blob/2ff148b674616a33761bd14fc1e9344d77bdcb19/src/object.d#L1680

The second and particularly the third problem too critical to release this.
Even more so as this will affect many people and breaks some code.

I'd suggest to revert this change and reevaluate the design for the next release.

[MartinNowak]

No, in absence of const(char)[] message() sink based approach alone wouldn't work for us. Main use case is the code like this:

class CustomEx : Exception
{
private char[] reusable_buffer;
override const(char)[] message () { return this.reusable_buffer; }
}

catch (Exception e)
{
log(e.message());
}
Sink-based version would still have to allocate here unless logging facility is changed to support it (hardly an option). I'd love to see both overloads but plain const(char)[] message() is bare minimum.

This is just wrong, whether or not a sink-based method allocates depends on the implementation of message, just like it does for the one returning const(char)[].

class MyException
{
    const(char)[] message1();
    void message2(scope void delegate(const(char)[] sink));
}

void test
{
    log(e.message()); // @nogc if both log and message are @nogc
    e.message(&log); // @nogc if both log and message are @nogc
    e.message(msg => log(msg)); // use any log function
    e.message(&bufferedSink!(msg => log(msg))); // composable in case you want to write the whole message at once
}

BUT, w/ the first signature the Exception implementation has to use a local buffer, and in practice will often just concat strings, to format the message. With a sink it's possible to format in place.

The typical implementations would look like this.

class Exception
{
    int status;

    const(char)[] message1()
    {
        return "Request returned "~status.to!string~".";
        return format("Request returned %s.", status);
    }

    void message2(scope void delegate(const(char)[] sink))
    {
        formattedWrite(sink, "Request returned %s.", status);
    }
}

A sink based method is a strict superset of the method returning a buffer.
There is no reason to choose the inferior design, in particular b/c toString is also drifting towards using sinks.

[MartinNowak]

The sink delegate method is incompatible with using ranges.

No, a sink is an output range is a sink is an output range.
That's why toString(&appender.put) work fabulous.
There is of course a problem to adapt input and output range b/c you need 2 execution contexts, but this is a general impedance mismatch between push and pull, not a particular issue of sinks.

[jmdavis]

Well, there's never any way around adding a symbol being a potential breaking change. Even adding a free function can do that. The only way we could avoid that would be to never add any symbols ever again. So, I certainly don't think that that's sufficient reason not to add this. That being said, I definitely agree that using a sink would be better in the general case and more in line with how the rest of how druntime/Phobos works. But the whole reason that this is here is because of the Sociomantic folks and what they wanted, and they didn't want a sink solution. Part of the problem though is that a sink solution does not work well with how std.experimental.logger is currently designed - or presumably the Tango logger (based on what's been said here) - and that's really want the Sociomantic folks need solved (if I understand correctly). So, presumably, one or both loggers would need to be improved in order for a sink solution to work for the Sociomantic folks.

But if it were up to me, I'd definitely go with the sink solution over what this PR has done.

[MartinNowak]

Part of the problem though is that a sink solution does not work well with how std.experimental.logger is currently designed

But that's not true, see #1445 (comment).

e.message(msg => log(msg)); // use any log function

[mihails-strasuns-sociomantic]

FYI: there is a live chat with @MartinNowak , @andralex and some guys from our side planned, I will delay any further commenting here until it becomes more clear where are we heading :)

[mihails-strasuns-sociomantic]

@MartinNowak can you please write down the relevant summary from the discussion here? Just for the public PR record.

[MartinNowak]

@MartinNowak can you please write down the relevant summary from the discussion here? Just for the public PR record.

With regards to Throwable.message we agreed on making it one of the first users of an upcoming reference counted string. Details on the design of the reference counted string first to be discussed on dlang-study.