Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue 21209 - scope attribute inference fails on foreach #12620

Merged
merged 1 commit into from
Jun 2, 2021

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Jun 1, 2021

Scope inference fails as soon as you do:

auto foo(int[] arr) {
    int[] tmp = arr; // tmp must be marked explicitly `scope`
}

A foreach on an array gets rewritten to:

int[] __r83 = arr[];
ulong __key84 = 0LU;
for (; __key84 < __r83.length; __key84 += 1LU)
{
	int c = __r83[__key84];
	// body
}

This fix explicitly marks the temporary __r83 scope. It can't escape since it's hidden.

This does not fix foreach on an AliasSeq (which gets lowered to a special "unrolled" with the same temporary problem). If this fix gets accepted, I'll open a follow-up issue for that case, which is a bit harder since the temporary is not hidden, so you cannot just mark it scope.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
21209 normal scope attribute inference with does not work well with foreach

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#12620"

@dkorpel
Copy link
Contributor Author

dkorpel commented Jun 1, 2021

I still have to account for foreach(ref a; arr) {return a;} I see

@Geod24
Copy link
Member

Geod24 commented Jun 1, 2021

Well, foreach (scope foo; a) doesn't even parse, so there's that too.

@MoonlightSentinel MoonlightSentinel self-requested a review June 2, 2021 10:57
@RazvanN7
Copy link
Contributor

RazvanN7 commented Jun 2, 2021

Does this also fix foreach on a struct that defines an opApply?

@dkorpel
Copy link
Contributor Author

dkorpel commented Jun 2, 2021

Does this also fix foreach on a struct that defines an opApply?

It only changes foreach on an array. The lowering to opApply is already scope compatible it seems.

@safe:
void foo(T)(T s) {
    static int x; x++; // force impure for issue 20150
    foreach(node; s) {cast(void) node;}
}

struct S {
    int* ptr;
    int opApply(int delegate(scope S) @safe dg) scope {return 0;}
}

void main() {
    scope S t;
    foo(t);
}

@Geod24
Copy link
Member

Geod24 commented Jun 2, 2021

The lowering to opApply is already scope compatible it seems.

Pretty sure it just ignores it. Try to escape something from the foreach.

@dkorpel
Copy link
Contributor Author

dkorpel commented Jun 2, 2021

Pretty sure it just ignores it. Try to escape something from the foreach.

Then it says "cannot pass argument __foreachbody3 of type int delegate(S node) nothrow @nogc @safe to parameter int delegate(scope S) @safe dg"

@thewilsonator thewilsonator merged commit 633a5b3 into dlang:master Jun 2, 2021
@dkorpel dkorpel deleted the scope-foreach branch June 3, 2021 08:00
@dkorpel
Copy link
Contributor Author

dkorpel commented Jun 3, 2021

Follow-up issue is here: https://issues.dlang.org/show_bug.cgi?id=21990

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants