fix Issue 17494 - Do not execute scope(...) if an Error exception has…

[WalterBright]

… been thrown

fix 17494

This is an enhancement, see https://issues.dlang.org/show_bug.cgi?id=17494

It's also a regression fix.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	17494	enhancement	Do not execute scope(...) if an Error exception has been thrown

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#6896"

[CyberShadow]

fix 17493

This should be in the commit message as well, so it's picked up by @dlang-bot.

[WalterBright]

This should be in the commit message as well, so it's picked up by @dlang-bot.

I don't know what dlang-bot looks for, everything I try fails. I tried "fix 17493" and "fix issue 17493", to no avail.

[CyberShadow]

I don't know what dlang-bot looks for, everything I try fails. I tried "fix 17493" and "fix issue 17493", to no avail.

Are you editing the commit message (when creating the git commit on your computer), or the pull request description (in your web browser)? We're talking about the former.

[wilzbach]

Are you editing the commit message (when creating the git commit on your computer), or the pull request description (in your web browser)? We're talking about the former.

I think I mention the link to the explanation almost daily ;-) https://github.com/dlang-bots/dlang-bot

And in dubito, try it online

Of course you can also execute the D function yourself:

import std.algorithm, std.conv, std.range, std.string;
import std.format : format;

auto matchIssueRefs(string message)
{
    import std.regex;

    static auto matchToRefs(M)(M m)
    {
        enum splitRE = regex(`[^\d]+`); // ctRegex throws a weird error in unittest compilation
        auto closed = !m.captures[1].empty;
        return m.captures[5].stripRight.splitter(splitRE)
            .filter!(id => !id.empty) // see #6
            .map!(id => IssueRef(id.to!int, closed));
    }

    // see https://github.com/github/github-services/blob/2e886f407696261bd5adfc99b16d36d5e7b50241/lib/services/bugzilla.rb#L155
    enum issueRE = ctRegex!(`((close|fix|address)e?(s|d)? )?(ticket|bug|tracker item|issue)s?:? *([\d ,\+&#and]+)`, "i");
    return message.matchAll(issueRE).map!matchToRefs.joiner;
}

struct IssueRef { int id; bool fixed; }

void main()
{
   import std.stdio;
   matchIssueRefs("fix Issue 17494 17493 - Do not execute scope(...) if an Error ").writeln;
}

[andralex]

Cool! A promise made by the spec finally fulfilled. This will allow smaller and faster functions for most cases.

We'll need a changelog entry as well.

[WalterBright]

We're talking about the former.

It used to work by looking at the github comments.

[wilzbach]

Hmm #6816 also broke D-YAML, but this bugfix PR doesn't seem to fix the regression observed in D-YAML.

(working on a reduction)

[MartinNowak]

It used to work by looking at the github comments.

It never did that, we want our information in git to be available for tooling, not locked into a proprietary platform.

[MartinNowak]

We're about to release 2.075.0 soon, so let's not try to counter a risky change with yet another one (changing all scope (failure) catch handlers from Throwable to Exception). Given the amount of fallout, I'd rather revert #6816 for now and revisit it for the next major version (2.076 if we don't yet change the version scheme).

[WalterBright]

I agree it's a risky change.

[MartinNowak]

Apparently hangs some test in dmd's test suite.

[andralex]

@WalterBright I'll approve pending the unittester, I suppose this needs a bit of work.

[braddr]

removing the auto-merge flag because the tests don't pass and in fact are stalling resulting in abnormally long builds that must hit timeouts to end.

[JinShil]

@WalterBright Is this PR still needed? The test case in https://issues.dlang.org/show_bug.cgi?id=17493 compiles fine, and https://issues.dlang.org/show_bug.cgi?id=17494 is marked RESOLVED.

[wilzbach]

Probably still a good idea to add the test case to the test suite then.

[JinShil]

Probably still a good idea to add the test case to the test suite then.

It looks like the test case was added in #6913

[andralex]

@WalterBright please review and close

[JinShil]

I don't see any reason to keep this open.

[andralex]

cc @WalterBright sup

[WalterBright]

This PR is still the right thing to do. Attempting to unwind when an Error is thrown is a bad idea, and leads to undefined behavior. Essentially, code which relies on such unwinding needs to be fixed.

[JinShil]

This appears to break code in druntime

timelimit -t 10 generated/linux/release/32/unittest/test_runner rt.minfo
core.exception.InvalidMemoryOperationError@src/core/exception.d(702): Invalid memory operation
----------------
uncaught exception
core.exception.InvalidMemoryOperationError@src/core/exception.d(702): Invalid memory operation
----------------

[jpf91]

ping @ibuclaw I guess if DMD now starts to really employ the 'no cleaning up after throwing an error' rule and druntime and other libraries are fixed, we could finally turn such throw statements into a simple abort in GDC and map nothrow to TREE_NOTHROW for better optimizations?

(IIRC trying to unwind if we use TREE_NOTHROW is a bad idea, I think it crashes the unwinder on ARM. So a clean approach would probably have to give up on unwinding and use an abort-solution).

[ibuclaw]

@jpf91 - I think that a program crashing abnormally because something threw inside a nothrow function, although may not be seen as desirable, should be considered acceptable behaviour in release mode.

[jpf91]

Also OK, I don't have a strong opinion on that. Although an abort could make debugging easier than a crash somewhere in the unwind logic. OTOH architectures which can unwind from nothrow functions benefit from a stack trace...

[JohanEngelen]

If the agreement is that no unwinding should happen if an Error is thrown, then the standard exception handling mechanism should not be used: by throwing an exception, (some) unwinding will always happen...

[ghost]

What is the status of the issue ? It's marked closed. Can this PR be as well ? Also note that the test case has never failed to compile.

rdmd playground.d

All versions: Success and no output

[MoonlightSentinel]

What is the status of the issue ? It's marked closed. Can this PR be as well ?

Commented on the issue, scope(exit) is still executed for Errors.

Also note that the test case has never failed to compile.

Not for official releases - the changes from -preview=dtorfields were initiallly enabled by default but hidden as a preview because of the code breakage IIRC.

---

A small test case;

// nothrow: // Uncomment this to make the test pass

void main()
{
    bool caught;

    try
        foo();
    catch (Error)
        caught = true;

    assert(caught);
    assert(counter == 0);
}

__gshared int counter = 0;

void foo()
{
    scope (exit) counter++;
    throwError();
}

void throwError()
{
    throw new Error("");
}

[12345swordy]

@WalterBright tests have failed.

[WalterBright]

The tests all fail because the test suite hangs and times out. There's no clue which test hung.