I'm of a stance that good code should have many throws and few try/catch statements. I was curious how Phobos does there, so I ran a few simple greps around.

Seems like there's about 145 try statements in Phobos. Of these, more than a third (51) belong to std.datetime.

Looks like the following idiom is often present in std.datetime:

    @property FracSec fracSec() const nothrow
    {
        try
        {
            auto hnsecs = removeUnitsFromHNSecs!"days"(adjTime);

            if(hnsecs < 0)
                hnsecs += convert!("hours", "hnsecs")(24);

            hnsecs = removeUnitsFromHNSecs!"seconds"(hnsecs);

            return FracSec.from!"hnsecs"(cast(int)hnsecs);
        }
        catch(Exception e)
            assert(0, "FracSec.from!\"hnsecs\"() threw.");
    }

(It may seem I chose this example to discuss the "let's insert one empty line every other line" idiom. I didn't.) Essentially the code relies on calls that may generally throw, but calls them with parameters that should guarantee there won't be any throwing. In wanting to offer as many "nothrow" guarantees as possible, the code ends up inserting these try/catch statements - seemingly needlessly.

This is quite heavy-handed, so I was wondering what we could do to improve on this. I thought of the following possibility:

    @property FracSec fracSec() const nothrow
    {
        scope(failure) assert(0, "FracSec.from!\"hnsecs\"() threw.");
        auto hnsecs = removeUnitsFromHNSecs!"days"(adjTime);
        if (hnsecs < 0)
            hnsecs += convert!("hours", "hnsecs")(24);
        hnsecs = removeUnitsFromHNSecs!"seconds"(hnsecs);
        return FracSec.from!"hnsecs"(cast(int)hnsecs);
    }

This at least leaves the normal path unaltered and deals with the unexpected in a more isolated manner. I was pleased that the changed code compiled just fine. My happiness was short-lived because before long I figured this compiles as well:

        ...
        scope(failure) {}
        ...

So it's not that the compiler cleverly figured the function will not throw. Instead, the compiler considers everything dandy as soon as it sees a scope(failure), no matter of the statement it controls. I'll report that bug soon.

What would be the best approach here?

0. Do nothing. This is as good as it gets.

1. Fix scope(failure) and then use it.

2. Relax the nothrow guarantees. After all, nothrow is opt-in.

3. Look into API changes that add nothrow, narrower functions to the more general ones that may throw.

4. ...?


Andrei

On Sunday, November 17, 2013 22:32:46 Andrei Alexandrescu wrote:
> (It may seem I chose this example to discuss the "let's insert one empty line every other line" idiom. I didn't.)

That code's like that just because I like to put empty lines before and after if statements and before return statements, as I think that that improves legibility. Short functions like that suffer as a result, because they end up with a larger proportion of the lines being empty than is normal. I'm always a bit torn on that, because I don't like having quite that many empty lines in so few lines, but I also don't like not having space around if statements and return statements. I never feel like I can find a nice, legible balance with short functions like that.

> Essentially the code relies on
> calls that may generally throw, but calls them with parameters that
> should guarantee there won't be any throwing. In wanting to offer as
> many "nothrow" guarantees as possible, the code ends up inserting these
> try/catch statements - seemingly needlessly.

Yeah, I tried to use pure and nothrow fairly heavily in std.datetime and ran into a number of hurdles like this. Fortunately, the situation has improved somewhat (e.g. format can finally be pure at least some of the time), but it does show that it's not always easy to use pure or nothrow even when it arguably should be.

> What would be the best approach here?
> 
> 0. Do nothing. This is as good as it gets.

Well, it works just fine as-is, but it would be kind of nice to be able to solve the problem in a less verbose manner (though you're talking about saving only a few lines of code).

> 1. Fix scope(failure) and then use it.

Fine with me. I might still favor the try-catch in cases where you can clearly wrap it around one function call, because then you avoid problems where you've accidentally effectively marked the whole function as trusted-nothrow when you only want to mark one function call that way. But you could do the same thing with scope(failure) and a new scope. The main problem is when you can't really put the calls that need to be trusted-nothrow inside a new scope, in which case, you're forced to mark the whole function (or at least large portions of it) as trusted-nothrow by wrapping it all in a try-catch or scope(failure).

> 2. Relax the nothrow guarantees. After all, nothrow is opt-in.

I'm not quite sure what you're suggesting here. Make it so that nothrow does checking at runtime instead of compile time? That would be moving in the direction of C++ and throw specifiers (or more precisely, noexcept, I suppose). If that's what you're suggesting, I'd be very much against that. I think that the fact D's nothrow is statically checked is a huge advantage over C++'s noexcept. The fact that you have to sometimes use try-catch blocks (or scope(failure) if that works) to make it work is essentially the same as needing @trusted to make some stuff @safe. I wouldn't want to throw away @trusted in favor of making @safe more lax either (though that's almost all static checking which can't be done at runtime, unlike with noexcept).

Of course, there's no way to verify trusted-nothrow except at runtime like std.datetime is doing with try-catch and assertions, but most code _can_ be checked statically (including the code that calls the functions that use the try-catch-assert idiom to be able to be nothrow), and that's much more pleasant, particularly because it's actually checked by the compiler that way rather than just blowing up on you at runtime.

I suppose that if it were considered annoying enough to have to use try-catch blocks or scope(failure), we could add a nothrow equivalent to @trusted to mark functions with, though it's already been argued that @trusted should be on pieces of a function rather than on the whole function, and it would arguably be better to mark sections of a function as trusted-nothrow rather than the entire thing. try-catch lets us do that already, but it might be nice to be able to do the equivalent of

     @property FracSec fracSec() const nothrow
     {
         trusted-nothrow
         {
             auto hnsecs = removeUnitsFromHNSecs!"days"(adjTime);

             if(hnsecs < 0)
                 hnsecs += convert!("hours", "hnsecs")(24);

             hnsecs = removeUnitsFromHNSecs!"seconds"(hnsecs);

             return FracSec.from!"hnsecs"(cast(int)hnsecs);
         }
     }

and have the compiler insert the catch and assertion for you.

> 3. Look into API changes that add nothrow, narrower functions to the more general ones that may throw.

In some cases (e.g. format) that's probably a good idea, but I don't think that really scales. In some cases, it would be well worth it, whereas in others, it would just be better to use try-catch in the few places where you know the function won't throw, because it would be quite rare to be able to guarantee that it wouldn't throw. It's also not pleasant to have to duplicate functions all over the place.

> 4. ...?

We now have std.exception.assumeWontThrow, which works reasonably well when you need to wrap a single call as opposed to several, but it has the same problem as enforce in that it uses lazy, which is definite performance hit. So, in most cases, I'd be more inclined to just use a try-catch, and if it's more than one expression, you pretty much need to use try-catch or scope(failure) instead anyway, since you wouldn't want to wrap whole function bodies in a call to assumeWontThrow (assuming that you even could).

So, that's a partial solution, but not a perfomant one. However, we do really need to improve the performance of lazy, because enforce gets used all over the place, and it's definitely shown up as being costly in some of the profiling that's been shown in the newsgroup. And if that gets fixed, then using assumeWontThrow wouldn't be as bad.

All in all, I find the need to use try-catch blocks to effectively do trusted- nothrow a bit annoying, but I haven't felt that it was a big enough deal to try and find another solution for it. Having to do trusted-purity is far worse, because that requires using a function pointer and casting it, but that would _should_ be hard to do because of how hard it is to actually guarantee that the function is acting like a pure function even though it isn't. And I'm not sure that I'd entirely trust myself with that, let alone the average D developer. trusted-nothrow on the other hand is something that the average programmer should be able to grasp.

- Jonathan M Davis

On 11/17/13 11:28 PM, Jonathan M Davis wrote:
> On Sunday, November 17, 2013 22:32:46 Andrei Alexandrescu wrote:
>> (It may seem I chose this example to discuss the "let's insert one empty
>> line every other line" idiom. I didn't.)
>
> That code's like that just because I like to put empty lines before and after
> if statements and before return statements, as I think that that improves
> legibility. Short functions like that suffer as a result, because they end up
> with a larger proportion of the lines being empty than is normal. I'm always a
> bit torn on that, because I don't like having quite that many empty lines in
> so few lines, but I also don't like not having space around if statements and
> return statements. I never feel like I can find a nice, legible balance with
> short functions like that.

"Too much of a good thing" comes to mind (as does with the massive and repetitive unittests in std.datetime). If code looks and feels meh, it probably is. Just apply good judgment instead of rote adherence to rules.

>> 0. Do nothing. This is as good as it gets.
>
> Well, it works just fine as-is, but it would be kind of nice to be able to
> solve the problem in a less verbose manner (though you're talking about saving
> only a few lines of code).

I'm also concerned about generated code size and overall efficiency. It looks like the assert(0) insertions are there simply to validate the design (they only fail if Phobos has an internal error), so there should be some means to remove them in release builds. We don't have such a possibility at the moment.

>> 2. Relax the nothrow guarantees. After all, nothrow is opt-in.
>
> I'm not quite sure what you're suggesting here.

What I meant is, not everything that is nothrow needs to be annotated as such.

>       @property FracSec fracSec() const nothrow
>       {
>           trusted-nothrow
>           {
>               auto hnsecs = removeUnitsFromHNSecs!"days"(adjTime);
>
>               if(hnsecs < 0)
>                   hnsecs += convert!("hours", "hnsecs")(24);
>
>               hnsecs = removeUnitsFromHNSecs!"seconds"(hnsecs);
>
>               return FracSec.from!"hnsecs"(cast(int)hnsecs);
>           }
>       }

That can be done at library level.

>> 4. ...?
>
> We now have std.exception.assumeWontThrow, which works reasonably well when
> you need to wrap a single call as opposed to several, but it has the same
> problem as enforce in that it uses lazy, which is definite performance hit. So,
> in most cases, I'd be more inclined to just use a try-catch, and if it's more
> than one expression, you pretty much need to use try-catch or scope(failure)
> instead anyway, since you wouldn't want to wrap whole function bodies in a
> call to assumeWontThrow (assuming that you even could).

Again, one problem here (in addition to the blowup in code size and decay of readability) is we're talking about an internal design validation, not meaningful runtime semantics. There should be a way to remove the baggage.


Andrei

On Monday, November 18, 2013 00:14:24 Andrei Alexandrescu wrote:
> On 11/17/13 11:28 PM, Jonathan M Davis wrote:
> > On Sunday, November 17, 2013 22:32:46 Andrei Alexandrescu wrote:
> "Too much of a good thing" comes to mind (as does with the massive and repetitive unittests in std.datetime). If code looks and feels meh, it probably is. Just apply good judgment instead of rote adherence to rules.

Well, my coding style has adjusted over time, and I'm tending to put fewer blank lines in cases like this, precisely because it's not terribly pretty.

> as does with the massive and repetitive unittests in std.datetime

Those are quite valuable IMHO, as being thorough has caught a lot of problems, and as we've discussed (and disagreed on) before, I prefer unit tests to be dead simple in order to reduce the odds of them being buggy, even if that means that they're more verbose. I have done some work however to move the tests toward using loops instead in order to reduce the number of lines of code, but I have quite a bit still to do there. I'll probably get back to that after I finish with splitting std.datetime, but I haven't finished that yet because of how busy I've been the past few months. So, everything takes forever... Oh well, I should get back to that soon enough.

> >> 0. Do nothing. This is as good as it gets.
> > 
> > Well, it works just fine as-is, but it would be kind of nice to be able to solve the problem in a less verbose manner (though you're talking about saving only a few lines of code).
> 
> I'm also concerned about generated code size and overall efficiency. It looks like the assert(0) insertions are there simply to validate the design (they only fail if Phobos has an internal error), so there should be some means to remove them in release builds. We don't have such a possibility at the moment.

Well, the assertions are there to validate the design, but the try-catches are required to make the function nothrow, and I'd feel very funny having an empty catch block, though having it compiled out in -release wouldn't necessarily be a bad idea (which could probably be done if a function which returned false where used in the assertion instead of a constant).

> >> 2. Relax the nothrow guarantees. After all, nothrow is opt-in.
> > 
> > I'm not quite sure what you're suggesting here.
> 
> What I meant is, not everything that is nothrow needs to be annotated as such.

So, you're suggesting that nothrow be inferred?

> >       @property FracSec fracSec() const nothrow
> >       {
> > 
> >           trusted-nothrow
> >           {
> > 
> >               auto hnsecs = removeUnitsFromHNSecs!"days"(adjTime);
> > 
> >               if(hnsecs < 0)
> > 
> >                   hnsecs += convert!("hours", "hnsecs")(24);
> > 
> >               hnsecs = removeUnitsFromHNSecs!"seconds"(hnsecs);
> > 
> >               return FracSec.from!"hnsecs"(cast(int)hnsecs);
> > 
> >           }
> > 
> >       }
> 
> That can be done at library level.

Well, it is being done at the library level right now via try-catch.

> >> 4. ...?
> > 
> > We now have std.exception.assumeWontThrow, which works reasonably well
> > when
> > you need to wrap a single call as opposed to several, but it has the same
> > problem as enforce in that it uses lazy, which is definite performance
> > hit. So, in most cases, I'd be more inclined to just use a try-catch, and
> > if it's more than one expression, you pretty much need to use try-catch
> > or scope(failure) instead anyway, since you wouldn't want to wrap whole
> > function bodies in a call to assumeWontThrow (assuming that you even
> > could).
> 
> Again, one problem here (in addition to the blowup in code size and decay of readability) is we're talking about an internal design validation, not meaningful runtime semantics. There should be a way to remove the baggage.

I think that that would require a language change of some kind, because it would require that the compiler recognize that the check can go away in -release, which would effectively mean having the try-catch block be optimized away somehow when compiling with -release. Using scope(failure) for that wouldn't change anything, as it would just mean that the compiler was inserting the try-catch statements for you. It would just be slightly less verbose.

Maybe the compiler can be changed to optimize out try-catch blocks if the catch block just contains an assertion? That would at least get rid of the overhead of the idiom, even if it didn't make it less verbose. If we can't get the compiler to recognize the idiom and optimize out the unnecessary code, then if we want it to be optimized out, I think that we'll be forced to add something to the language which the compiler _does_ recognize and therefore can compile out when appropriate - unless someone can come up with some great idea for improving nothrow, but that seems a lot like it would be moving towards not needing @trusted anymore, because the compiler would figure that out for you. I don't see how you could get away from having to have the programmer tell the compiler that they're sure that the function which can throw won't throw inside the nothrow function, because that requires human judgement just like @trusted does, though it should generally be easier for a programmer to be sure that something won't throw than it is to be sure that it's actually @safe.

- Jonathan M Davis

On 2013-11-18 08:28, Jonathan M Davis wrote:

> That code's like that just because I like to put empty lines before and after
> if statements and before return statements, as I think that that improves
> legibility. Short functions like that suffer as a result, because they end up
> with a larger proportion of the lines being empty than is normal. I'm always a
> bit torn on that, because I don't like having quite that many empty lines in
> so few lines, but I also don't like not having space around if statements and
> return statements. I never feel like I can find a nice, legible balance with
> short functions like that.

I like the empty newlines as well. Otherwise it feels like reading a text without paragraphs.

> Fine with me. I might still favor the try-catch in cases where you can clearly
> wrap it around one function call, because then you avoid problems where you've
> accidentally effectively marked the whole function as trusted-nothrow when you
> only want to mark one function call that way. But you could do the same thing
> with scope(failure) and a new scope. The main problem is when you can't really
> put the calls that need to be trusted-nothrow inside a new scope, in which
> case, you're forced to mark the whole function (or at least large portions of
> it) as trusted-nothrow by wrapping it all in a try-catch or scope(failure).

In this case the whole function is wrapped on a try-block. In those cases I think a scope(failure) looks nicer.

>> 2. Relax the nothrow guarantees. After all, nothrow is opt-in.
>
> I'm not quite sure what you're suggesting here. Make it so that nothrow does
> checking at runtime instead of compile time? That would be moving in the
> direction of C++ and throw specifiers (or more precisely, noexcept, I suppose).
> If that's what you're suggesting, I'd be very much against that. I think that
> the fact D's nothrow is statically checked is a huge advantage over C++'s
> noexcept. The fact that you have to sometimes use try-catch blocks (or
> scope(failure) if that works) to make it work is essentially the same as
> needing @trusted to make some stuff @safe. I wouldn't want to throw away
> @trusted in favor of making @safe more lax either (though that's almost all
> static checking which can't be done at runtime, unlike with noexcept).
>
> Of course, there's no way to verify trusted-nothrow except at runtime like
> std.datetime is doing with try-catch and assertions, but most code _can_ be
> checked statically (including the code that calls the functions that use the
> try-catch-assert idiom to be able to be nothrow), and that's much more
> pleasant, particularly because it's actually checked by the compiler that way
> rather than just blowing up on you at runtime.
>
> I suppose that if it were considered annoying enough to have to use try-catch
> blocks or scope(failure), we could add a nothrow equivalent to @trusted to
> mark functions with, though it's already been argued that @trusted should be
> on pieces of a function rather than on the whole function, and it would
> arguably be better to mark sections of a function as trusted-nothrow rather
> than the entire thing. try-catch lets us do that already, but it might be nice
> to be able to do the equivalent of
>
>       @property FracSec fracSec() const nothrow
>       {
>           trusted-nothrow
>           {
>               auto hnsecs = removeUnitsFromHNSecs!"days"(adjTime);
>
>               if(hnsecs < 0)
>                   hnsecs += convert!("hours", "hnsecs")(24);
>
>               hnsecs = removeUnitsFromHNSecs!"seconds"(hnsecs);
>
>               return FracSec.from!"hnsecs"(cast(int)hnsecs);
>           }
>       }
>
> and have the compiler insert the catch and assertion for you.

Just do something like this:

void trusted_nothrow (alias block) ()
{
    scope(failure) assert(0);
    block();
}

trusted_nothrow!({
    // code
});

Not as pretty. Yet another prefect example for AST macros :)

-- 
/Jacob Carlborg

On 2013-11-18 09:14, Andrei Alexandrescu wrote:

> I'm also concerned about generated code size and overall efficiency. It
> looks like the assert(0) insertions are there simply to validate the
> design (they only fail if Phobos has an internal error), so there should
> be some means to remove them in release builds. We don't have such a
> possibility at the moment.

Asserts are supposed to be removed in release builds. There's also the "debug" statement that can be used. Although it won't remove the try-catch.

-- 
/Jacob Carlborg

On Monday, November 18, 2013 10:09:35 Jacob Carlborg wrote:
> On 2013-11-18 09:14, Andrei Alexandrescu wrote:
> > I'm also concerned about generated code size and overall efficiency. It looks like the assert(0) insertions are there simply to validate the design (they only fail if Phobos has an internal error), so there should be some means to remove them in release builds. We don't have such a possibility at the moment.
> 
> Asserts are supposed to be removed in release builds.

The aren't if the condition is statically known to be false, as is the case when you use a try-catch block to catch Exception and then assert within the catch block in order to make the function be able to be nothrow.

> There's also the
> "debug" statement that can be used. Although it won't remove the try-catch.

It also won't run unless you compile with -debug, which I expect very, very few people do.

- Jonathan M Davis

On Monday, November 18, 2013 09:59:44 Jacob Carlborg wrote:
> Just do something like this:
> 
> void trusted_nothrow (alias block) ()
> {
>      scope(failure) assert(0);
>      block();
> }
> 
> trusted_nothrow!({
>      // code
> });
> 
> Not as pretty. Yet another prefect example for AST macros :)

I don't see much gain over simply putting scope(failure) at the top. You just avoid having to write the assertion yourself. I also expect that it wouldn't work with return statements.

- Jonathan M Davis

On 2013-11-18 10:13, Jonathan M Davis wrote:

> The aren't if the condition is statically known to be false, as is the case
> when you use a try-catch block to catch Exception and then assert within the
> catch block in order to make the function be able to be nothrow.

Then it supposed to turn it into a HLT instruction, at least on x86. I don't know how much that affects the code size.

> It also won't run unless you compile with -debug, which I expect very, very
> few people do.

True.

-- 
/Jacob Carlborg

On 2013-11-18 10:18, Jonathan M Davis wrote:

> I don't see much gain over simply putting scope(failure) at the top. You just
> avoid having to write the assertion yourself

You suggested it, but by adding it to the language instead.

> I also expect that it wouldn't work with return statements.

No. It would just return from the delegate.

-- 
/Jacob Carlborg