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 20682 - [DIP1000] wrong error: scope variable may not be co… #10935

Merged
merged 2 commits into from Mar 19, 2020

Conversation

aG0aep6G
Copy link
Contributor

…pied into allocated memory

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @aG0aep6G! 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
20682 normal [DIP1000] wrong error: scope variable may not be copied into allocated memory

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

@aG0aep6G aG0aep6G changed the title fix issue 20682 - [DIP1000] wrong error: scope variable may not be co… [WIP] fix issue 20682 - [DIP1000] wrong error: scope variable may not be co… Mar 17, 2020
@aG0aep6G
Copy link
Contributor Author

I think a better fix could be possible. In its current state, this PR only fixes a special case: If the escaping value has no pointers, then it can't possibly refer to the scope parameter. But there are also cases where the escaping value is a pointer, and it still can't refer to the scope parameter.

For example, this could also be allowed, as far as I understand:

ref int* f(return scope int** a) @safe
{
    return *a;
}

int* test(scope int** a) @safe
{
    // return *a; // already allowed
    return f(a); // could be made to work
}

But it seems that the escape checker currently doesn't compare types in that manner at all. Adding it would be a larger task with many pitfalls.

@aG0aep6G aG0aep6G force-pushed the 20682 branch 3 times, most recently from 6346988 to c1325b9 Compare March 18, 2020 18:37
@aG0aep6G aG0aep6G changed the title [WIP] fix issue 20682 - [DIP1000] wrong error: scope variable may not be co… fix issue 20682 - [DIP1000] wrong error: scope variable may not be co… Mar 18, 2020
@aG0aep6G
Copy link
Contributor Author

ref int* f(return scope int** a) @safe
{
    return *a;
}

int* test(scope int** a) @safe
{
    // return *a; // already allowed
    return f(a); // could be made to work
}

Making that work is beyond me at the moment. But I've found a better spot for the hasPointers check, and also fixed the same issue for cast.

This is good to go from my side.

@dlang-bot dlang-bot merged commit 03de563 into dlang:master Mar 19, 2020
@aG0aep6G aG0aep6G deleted the 20682 branch March 19, 2020 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants