More exception classes into Phobos?

[Adam D. Ruppe via Digitalmars-d]

On Friday, 24 March 2017 at 19:38:14 UTC, H. S. Teoh wrote:
> Catching an Exception by message? That sounds like horrible 
> code smell to me.

Yes, it is. That's why I think exception strings are an 
antipattern. You should be making new classes, not new strings.

But, D lets us have both worlds. Consider the following:

---

class MallocedException : Exception {
	@nogc
	this(string msg, string file = __FILE__, size_t line = __LINE__, 
Throwable next = null) {
		super(msg, file, line, next);
	}

	@nogc
	private void freeSelf() {
		import core.stdc.stdlib;
		import core.stdc.stdio;
		printf("freeing\n");
		free(cast(void*) this);
	}

	~this() {
		freeSelf();
	}
}

class RaisedException(string msg) : MallocedException {
	@nogc
	this(string file = __FILE__, size_t line = __LINE__, Throwable 
next = null) {
		super(msg, file, line, next);
	}
}

class RaisedExceptionDetails(string message, T...) : 
RaisedException!message {
	T args;
	@nogc
	this(T args, string file = __FILE__, size_t line = __LINE__, 
Throwable next = null) {
		this.args = args;
		super(file, line, next);
	}

	override void toString(scope void delegate(in char[]) sink) 
const {
		import core.internal.traits : externDFunc;
		alias sizeToTempString = 
externDFunc!("core.internal.string.unsignedToTempString",
						      char[] function(ulong, char[], uint) @safe pure 
nothrow @nogc);

		char[20] tmpBuff = void;

		sink("RaisedException");
		sink("@"); sink(file);
		sink("("); sink(sizeToTempString(line, tmpBuff, 10)); sink(")");

		if (message.length)
		{
		    sink(": "); sink(message);
		}

		foreach(idx, arg; args) {
			sink("\n\t[");
			sink(sizeToTempString(idx, tmpBuff, 10));
			sink("] = ");
			static if(is(typeof(arg) : const(char)[]))
				sink(arg);
			else static if(is(typeof(arg) : long))
				sink(sizeToTempString(arg, tmpBuff, 10));
			else
				{} // FIXME support more
		}

		if (info)
		{
		    try
		    {
			sink("\n----------------");
			foreach (t; info)
			{
			    sink("\n"); sink(t);
			}
		    }
		    catch (Throwable)
		    {
			// ignore more errors
		    }
		}


	}

}

@trusted @nogc
void raise(string msg, string file = __FILE__, size_t line = 
__LINE__, T...)(T args) {
	import core.stdc.stdlib, std.conv;
	enum size = __traits(classInstanceSize, 
RaisedExceptionDetails!(msg, T));
	void* buffer = malloc(size);
	assert(buffer !is null);
	throw emplace!(RaisedExceptionDetails!(msg, T))(buffer[0 .. 
size], args, file, line);
}

void main() {
	raise!"my_exception"(12, "additional info", 32);
}

---


That raise line lets you define your string if you must, as well 
as *any* other data you want to pass along, with a usable, if a 
bit less than ideal, toString representation of it.

BTW, I also went ahead and made this @nogc to show that's not 
really so hard to do. If you catch a MallocException, just 
.destroy(it). Easy (at least if you control both sides of the 
code).

Notice that there are also no Phobos imports and no user 
allocations at the raise site.



The only thing I'd add to it is some kind of 
raise!YourExceptionBase overload and I'd find this pretty useful.


> It probably makes sense for 3rd party libraries to have at 
> least a subclass of Exception, so that you can catch errors 
> originating from that library rather than everything in general.

I tend to do very broad classifications. My script.d, for 
example, can throw three supertypes of exception: compile 
exception, runtime exception, and user-defined exceptions out of 
the script.

Those are the things you'd likely catch, because each one has a 
different yet reasonable possible response.

But, if additional info is easy to attach, I'd go ahead and do 
that too - there's just no reason not to when it is easy, and the 
catcher can then decide to get into more details if interested, 
or just catch one of the super classes if not.

> Actually, I see this as evidence *against* having RangeError in 
> the first place.  If we had stuck to throwing Exception or, in 
> this case, Error, that would have prompted whoever wrote the 
> bounds check code to actually write a useful error message

Come on, be realistic. We both know it would be `throw new 
Exception("RangeError");` with no additional data.

In fact, since building strings is *significantly* more difficult 
than just passing arguments, building a useful string is expected 
to be uncommon... and it is. Grep phobos and find the majority of 
exception strings are static literals, even when more information 
could be available.

> Except that it's not really *that* useful if you don't know 
> what those numbers mean.

The class toString could also label them trivially, or, of 
course, you could document it for those who care (and those who 
don't care will just learn to ignore the noise).

It is far more likely that the class toString would give useful 
results than throw new Exception(format(...)) since:

1) it is written once, at the class definition (or in the mixin 
reflection template), instead of at every throw point

and 2) functions like format("") aren't available in a LOT of 
contexts, whereas toString(sink) from data internal to the 
exception can be done virtually anywhere. format("") adds a 
Phobos dependency, making it a no go for druntime, it adds a GC 
dependency, making it no go in those nogc contexts, and having to 
add the import can be as much of a pain as importing a exception 
helper template anyway.

But even if it isn't labeled, you could look it up, or get used 
to it, or maybe even guess it from context. The point is that the 
info is THERE for those who want to read it. (Frankly, I prefer 
just segfaulting than throwing the useless RangeError as it is 
now, at least a core dump can be loaded up in a debugger.)

> A similar discussion came up some years ago, where Andrei 
> proposed adding a Variant[string] field to Exception where the 
> thrower can stick whatever details he likes into it.

Yeah, that's useless, it has all the same problems as format(), 
plus some.

> It's already kind enough on my part to actually want to use 
> std.format to produce a message that a human reader might find 
> helpful, like "syntax error on line 36: unexpected character 
> 'x'". I could have just done a `throw new Exception("syntax 
> error");` and be done with it.  At least I, the human, would 
> read that message if the Exception got thrown, but put in 
> additional effort so that it's parseable by code?

Reminder: we're using D here. It is LESS effort to add info than 
it is to call format().