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.

[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.