fix Issue 17795 - [scope] Scope errors not detected in ~= operation

[WalterBright]

No description provided.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Description
✓	17795	[scope] Scope errors not detected in ~= operation

[mathias-lang-sociomantic]

The name of the parameter is wrong, should be e instead of ae.

[WalterBright]

e implies any Expression, while ae implies assignment expression, which it is.

[mathias-lang-sociomantic]

I'm fine with either name, I was pointing out that the function parameter in the code is named 'e' while in here it's ae, so DDOC will not match them.

[WalterBright]

oh, ok. Fixed.

[mathias-lang-sociomantic]

Why isn't the check done for T[] ~= T[] ?
If T is a reference type, then the assignment should be checked as well. I checked out your branch and tested it:

void main () @safe
{
    Object o = test();
    assert(o !is null);
}

Object test() @safe
{
    scope Object obj = new Object;
    scope Object[] arr;
    arr ~= obj;
    Object[] array;
    array ~= arr;
    return array[0];
}

It compiles and pass.

[WalterBright]

I believe that requires distinctly different code to implement, and so in order to avoid confusing things, should be a separate bugzilla issue and PR.

[WalterBright]

https://issues.dlang.org/show_bug.cgi?id=17842

[mathias-lang-sociomantic]

Somehow I forgot to reply here...
The reason I came up with this test-case was because the new code is explicitly checking for it, and explicitly excluding it. But if you prefer to separate the issue, fine by me.

[WalterBright]

There are 3 completely distinct cases for appending:

1. appending an array to an array
2. appending an element to an array
3. appending a multibyte utf character to an array

This fixes 2. Your case is 1. After this is pulled, I intend to do a bit of refactoring to make that clearer, then attend to case 1.

[mathias-lang-sociomantic]

Perfect.
However, as you can see from the failing Jenkins test, this check should be wrapped to only be performed with -dip1000 or it'll break code.

[WalterBright]

ok

[WalterBright]

@mathias-lang-sociomantic @MartinNowak these days-weeks delays in pulling things make for absolutely glacial progress in fixing scope issues, and makes it hard to do the incremental progress method for making PRs small and easily reviewed.

[mathias-lang-sociomantic]

@WalterBright : The P.R. has been red for the whole time it's been up. And in the case reported in my comment, it was because of this P.R. I assume you did not look at the Jenkins failure otherwise you'd had seen the failure.

For the last 3 days Travis has been red because another P.R. was merged with broken tests. That doesn't help much either, but could have been avoided by looking at the test of the original P.R.

Lastly, bear in mind my review is based mostly on test case. Whenever I report an issue with a P.R., it usually ends up being reported as another bugzilla issue, like it was here. Which leaves me with no way on knowing when things are actually fixed. If you wish to separate the issue, fixing it in the same P.R. but in different commits might achieve what you want to do (as one can review on a per-commit basis).

[mathias-lang-sociomantic]

The Travis CI failure is unrelated, merging

[WalterBright]

@mathias-lang-sociomantic I greatly appreciate your contributions in finding cases that are not correctly handled. Your work has significantly helped in making scope work. Thank you!