implement druntime side of DIP1008

[WalterBright]

see dlang/dmd#6681 which is blocking this

[ibuclaw]

Your also forgetting to update dwarfeh?

[WalterBright]

I forgot to check it in. Done.

[marler8997]

Just a thought. Would there be a way to use a different vptr for reference-counted Throwable's? If so, you could use virtual functions instead of adding a _refcount to every Throwable object. Not sure if this design is better or worse (or if it's even feasible to implement) but it's something to consider I think.

[marler8997]

I thought about this more and realized that in order to do this, you would have to create 2 vtables for EVERY CLASS that inherits from throwable, one for RC objects and one for GC objects. A solution like this could be worth looking into for a general solution to RC objects, however, I don't think is appropriate for this feature.

[marler8997]

Since this change would require adding "scope" to Throwable objects in all catch clauses, what do you think about adding a method to Throwable to retreive an object that can escape the scope? Possibly something like:

@property inout(Throwable) unscoped() inout
{
    if (refcount == 0)
    {
        return this; // this object is already GC
    }
    // todo: copy and return it
}

It would be even better if you could guard this function only allowing it to be called on "scoped" objects, but I've never heard of such a feature. Anyway, this type of function would allow catch clauses to easily escape exceptions and prevents the need to copy exceptions that are already GC.

Throwable error = null;
try
{
    // do something "exceptional"
}
catch(Throwable e)
{
    error = e.unscoped;
    //or
    throw new NestingException("my message", ..., e.unscoped);
}

// now you can use the Throwable error however you like

[WalterBright]

I removed the -dip1006 from the makefiles to avoid the chicken-and-egg of which to pull first. Pull this one first!

[WalterBright]

This is failing because dinterpret.d is using direct access to members rather than symbolic :-(

[WalterBright]

Blocked by dlang/dmd#6689

[marler8997]

dlang/dmd#6689 was merged 14 days ago, is there something else blocking this?

[MartinNowak]

The preliminary review of DIP1008 went pretty bad, why are we still going ahead with this before reaching a conclusion?

Those legacy C runtime interfaces have turned out to be a huge pain, because they are holes for attribute checks and cause lots of code breakage when we start to check 'em.
Please move _d_new/deleteThrowable to templated functions in object.d (possibly with an implementation in core.internal if it's too big) in order to not introduce new attribute holes holes for MyException.this and MyException.~this.

[WalterBright]

The preliminary review of DIP1008 went pretty bad, why are we still going ahead with this before reaching a conclusion?

The objections were not technically well founded, none were on the grounds that it wouldn't work, and nobody solidly proposed anything better. This is a low risk change, it's minimally disruptive, and so can be removed/altered if it doesn't work out.

to templated functions

I'm not sure how that would improve things.

[dlang-bot]

Thanks for your pull request, @WalterBright!

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.

[ibuclaw]

Please move _d_new/deleteThrowable to templated functions in object.d

Yeah, I think as a reasonable barrier, we should not be accepting any new runtime functions for compiler generated code that are plain extern(C).

However I am on the sideline with this particular function as it doesn't require RTTI. Otherwise I would strongly agree.

changes are folded in

[Burgos]

Would it be possible to add unittests here, without involving the compiler?

[Burgos]

This needs ddoc syntax, as this is a public method.

[Burgos]

from the exception pool - There is no pool in this implementation, isn't there?

[WalterBright]

Right. But there could be - without changing the interface.

[andralex]

The preliminary review of DIP1008 went pretty bad, why are we still going ahead with this before reaching a conclusion?

I'll summarize the concerns raised:

1. There is no standardized way to clone a class object, making it difficult for folks who escape exceptions to adapt their code. (The solution proposed works only under certain assumptions.)
2. Making exceptions nogc does not solve entirely the problem of making all of phobos nogc. A number of schemata have been discussed, neither in sufficient detail to be assessed for feasibility and impact.
3. Special casing syntax has smell.
4. The DIP is addressing garbage created by exceptions, whereas the right angle is to give the user control over memory allocation strategy, most likely by adding an allocator parameter to standard library functions.
5. This solution becomes redundant (albeit transparently) if we later add reference counting for general classes.
6. Even if exceptions become nogc, the messages they carry may create garbage (unless string literals are used). This limits usefulness of exception messages.
7. The use of reference counting has been questioned, seeing as there is only one reference to the exception object throughout.
8. A simpler alternative has been sketched - require the user to write catch (scope SomeException e). This puts the nogc onus on the other end of the protocol - it's not the new SomeException that creates the garbage, it's the non-scope catching.
9. staticException has also been mentioned as a library-only patch. It has issues with multiple active exceptions of the same type within the same thread.

My responses to these concerns are as follows:

1. Cloning is indeed a problem with porting code forward. There are two mitigating factors: (a) it can be addressed on a case basis on the user side, and (b) there are very few instances when escaping the caught Exception object is useful (I can't recall any).
2. Exceptions are not the only reason phobos creates garbage, but they are one important reason out of a number of unrelated ones. The holistical approach is generous in intent but would need a lot more detail figured out before it becomes a solid argument.
3. Special casing of syntax (throw new) does have liabilities but also the advantage it works automatically with existing code. There is precedent of syntax casing in D and also in many other languages, and we don't believe it should be universally shunned. It comes down to one's design sensibilities on a case basis. In this case @WalterBright and I agree it is a good engineering choice, all things considered.
4. Allowing user-controlled allocation and passing allocators around is a red herring - the issue at hand has to do with tracking the number of references to an object. This is a fundamental language design issue.
5. Acknowledged. We want the solution to be automatic enough to warrant swapping implementations later.
6. This is indeed an issue. It is independent of exceptions themselves and can be addressed in separation.
7. Walter cited implementation effort concerns. As long as this is just an implementation-related concern, it shouldn't be a blocking factor.
8. This alternative would need significant work to reconcile with chained exceptions. Also consider code that creates an exception, escapes it e.g. in a global, then throws it. Now even if the code catches the exception by scope, it can't delete it. An alternative proposal would need to carefully examine all such cases and provide solutions to them.
9. staticException is a good first workaround for certain issues, but the reality is - it just handles one exception of a given object per thread, which leads to surprising effects when multiple exceptions are in flight (e.g via chaining).

There would be a higher level note that is much more important.

The preliminary review opened on May 19, and last comment was made on June 18. There has been no work on anything related to this following that - no alternative DIP, no proposed improvements to the DIP, and no compiler PRs. From a practical perspective it's safe to assume we won't have any competing DIP on our hands anytime soon.

This DIP makes strong progress at a core issue. We can't afford to wait more for the perfect solution. At this point my main concern is avoiding lock-in, i.e. I want the implementation to be encapsulated enough to allow us to improve it later without disrupting user code.

Let's see through the technical issues that will get this PR in so we can experiment with it. It's opt-in so no harm done. Thanks.

[WalterBright]

Those legacy C runtime interfaces have turned out to be a huge pain, because they are holes for attribute checks and cause lots of code breakage when we start to check 'em.
Please move _d_new/deleteThrowable to templated functions in object.d (possibly with an implementation in core.internal if it's too big) in order to not introduce new attribute holes holes for MyException.this and MyException.~this.

_d_newThrowable() has the same interface as _d_newClass(). I don't see where the latter has been causing problems by not being a template. The same for _d_deleteThrowable() and _d_delclass().

As @andralex said, this whole thing is opt-in anyway, so if it doesn't work out we can redo it without breaking things. (And it's not like it's a huge, disruptive change, it's surprisingly small.) But we won't find out the problems unless we try it. It needs to get pulled.

[andralex]

@MartinNowak I'll override your request for changes under the assumptions: (a) the migration of functions to templates can be done subsequently, and (b) the feature is opt-in and allows us to experiment and work the kinks out before making a decision.

Per comment - feel free to undo the pull if there are strong concerns.

[MartinNowak]

Yes let's see how this ends up.
On one-hand we want to move ahead with @nogc and exceptions are an unsolved problem atm.
For the @nogc @safe io library I had to resort to statically allocated exceptions for now.
https://github.com/MartinNowak/io/blob/14819fa88016d3f7dcdbe7172c3447e09f5f46a9/src/std/io/exception.d
On the other hand, we haven't yet decided on a candidate for safe manual memory management.
I'm currently researching and trying different approaches. So it remains a bit unclear how the ref-counted EH approach in this PR will fit into that work.

A simpler alternative has been sketched - require the user to write catch (scope SomeException e). This puts the nogc onus on the other end of the protocol - it's not the new SomeException that creates the garbage, it's the non-scope catching.

This alternative would need significant work to reconcile with chained exceptions. Also consider code that creates an exception, escapes it e.g. in a global, then throws it. Now even if the code catches the exception by scope, it can't delete it. An alternative proposal would need to carefully examine all such cases and provide solutions to them.

The idea was to transition catch parameters to scope to prevent implicit escaping, and provide an explicit clone method in case escaping is needed (e.g. collectException).
Also DIP1008 has a similar scope restriction without providing a way to clone exceptions:

In catch blocks, e is regarded as scope so that it cannot escape the catch block.

It's opt-in

Does it depend on a dmd switch?

_d_newThrowable() has the same interface as _d_newClass(). I don't see where the latter has been causing problems by not being a template. The same for _d_deleteThrowable() and _d_delclass().

_d_delclass/_d_deleteThrowable finalized the deleted object but don't use the static type to infer the attributes of the finalizer. Since Object.~this doesn't exist, rt_finalize has to assume the worst.
Throwable.~this is @trusted nothrow, but unfortunately not @nogc.

staticException is a good first workaround for certain issues, but the reality is - it just handles one exception of a given object per thread, which leads to surprising effects when multiple exceptions are in flight (e.g via chaining).

Well, it's questionable whether chained exceptions are that useful. Providing a mechanism to disable chaining, or to access the current exception in flight might be alternatives here.

---

Would be nice if we had a way to throw struct values that are implicitly convertible to Throwable.

void foo()
{
    throw refCounted!Exception();
}

void bar()
{
    try {}
    catch (scope Exception e)
    {
        // RefCounted!Exception destroyed here unless rethrown
    }
}