fix Issue 19035 - Escape in scope inference, improve scope inference #8408
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,3 +21,120 @@ int* test() @safe | |
| arr[0] ~= &i; | ||
| return arr[0][0]; | ||
| } | ||
|
|
||
| /* TEST_OUTPUT: | ||
| --- | ||
| fail_compilation/retscope6.d(7034): Error: reference to local variable `i` assigned to non-scope parameter `_param_1` calling retscope6.S.emplace!(int*).emplace | ||
| fail_compilation/retscope6.d(7035): Error: reference to local variable `i` assigned to non-scope parameter `_param_0` calling retscope6.S.emplace2!(int*).emplace2 | ||
| fail_compilation/retscope6.d(7024): Error: scope variable `_param_2` assigned to `s` with longer lifetime | ||
| fail_compilation/retscope6.d(7025): Error: scope variable `_param_2` assigned to `t` with longer lifetime | ||
| fail_compilation/retscope6.d(7037): Error: template instance `retscope6.S.emplace4!(int*)` error instantiating | ||
|
Contributor
There was a problem hiding this comment. I recommend a single section of test output, it will be much easier if the error messages need to be updated. It caused me quite some pain in another PR.
Member
Author
There was a problem hiding this comment. I do multiple blocks with a
Contributor
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment. Yeah, this only works with a single output section. |
||
| --- | ||
| */ | ||
|
|
||
| #line 7000 | ||
|
|
||
| alias T = int*; | ||
|
|
||
| struct S | ||
| { | ||
| T payload; | ||
|
|
||
| static void emplace(Args...)(ref S s, Args args) @safe | ||
| { | ||
| s.payload = args[0]; | ||
| } | ||
|
|
||
| void emplace2(Args...)(Args args) @safe | ||
| { | ||
| payload = args[0]; | ||
| } | ||
|
|
||
| static void emplace3(Args...)(S s, Args args) @safe | ||
| { | ||
| s.payload = args[0]; | ||
| } | ||
|
|
||
| static void emplace4(Args...)(scope ref S s, scope out S t, scope Args args) @safe | ||
|
Contributor
There was a problem hiding this comment. https://dlang.org/spec/attribute.html#scope
This is violating the spec. The spec needs an update.
Contributor
There was a problem hiding this comment. https://dlang.org/spec/attribute.html#scope
Also not accurate.
Member
Author
There was a problem hiding this comment. As I've mentioned before, dip1000 has not been folded into the spec.
Contributor
There was a problem hiding this comment. Yes, and that's part of the problem. Why hasn't it been done? And shouldn't it be done either prior to PRs like this or together with PRs like this?
Member
Author
There was a problem hiding this comment.
One large reason is because I can't get my spec fixes pulled. Aside from that, DIP1000 despite being years old has little use, so I don't know how well it works. I can't get people to use DIP1000 features because phobos doesn't compile with dip1000. So I try to get phobos to compile with dip1000. I run into problems. I make corrections (like this PR). And I get blocked. End result, no progress anywhere. There is another issue. I used to work at Boeing in design. The designs ended up as drawings, using pen and ink on special plastic paper. Unsurprisingly, revising these drawings (including redrawing them) is quite expensive. But the designs changed constantly (bug fixes, improvements, changing requirements, the same issues that affect software). What was done was issuing DCN (Drawing Change Notices), which contained the fixes to the drawing. The official "drawing" was the original drawing plus a stack of DCNs, applied in chronological order (much like git, git is hardly a new concept). Eventually, the drawing may get redone with all the DCNs folded in. The analogy for us is the spec, plus approved DIPs, plus Bugzilla Enhancement Requests and bug fixes. It's not ideal, but it's pragmatic. Halting all progress because it all hasn't been folded into the spec yet is not pragmatic, especially when my PRs to update the spec just sit there.
Contributor
There was a problem hiding this comment.
I suggest putting a time limit on those PRs that haven't received feedback. We already have a `72h no objection -> merge" label in the DMD repository, and I don't see why that can't be applied to your dlang.org PRs. Noone has a right to complain if they chose not to participate.
I understand that, but I'm not going to be your rubber stamp. I need to understand what these PRs are doing and why. Others on this team are well within their charter to move this PR forward if they understand it and have confidence in the direction you're heading, but I'm not on board yet and have concerns. I've already sent you an e-mail or two about the way |
||
| { | ||
| s.payload = args[0]; | ||
| t.payload = args[0]; | ||
|
Contributor
There was a problem hiding this comment. Due to the lack of documentation surrounding the semantics of When I analyze the implementation of
Member
Author
There was a problem hiding this comment. I'll add some more documentation.
Contributor
There was a problem hiding this comment. @WalterBright Is it really too much to ask to have you open a dlang.org PR to document the semantics of
Member
Author
There was a problem hiding this comment. It's documented in DIP25 and DIP1000. I'd start with a gentler introduction, http://dconf.org/2017/talks/bright.html |
||
| } | ||
|
|
||
| } | ||
|
|
||
| void foo() @safe | ||
| { | ||
| S s; | ||
| int i; | ||
| s.emplace(s, &i); | ||
| s.emplace2(&i); | ||
| s.emplace3(s, &i); | ||
| s.emplace4(s, s, &i); | ||
| } | ||
|
|
||
|
|
||
| /* TEST_OUTPUT: | ||
| --- | ||
| fail_compilation/retscope6.d(8016): Error: reference to local variable `i` assigned to non-scope parameter `s` calling retscope6.frank!().frank | ||
| fail_compilation/retscope6.d(8031): Error: reference to local variable `i` assigned to non-scope parameter `p` calling retscope6.betty!().betty | ||
| fail_compilation/retscope6.d(8031): Error: reference to local variable `j` assigned to non-scope parameter `q` calling retscope6.betty!().betty | ||
| fail_compilation/retscope6.d(8048): Error: reference to local variable `j` assigned to non-scope parameter `q` calling retscope6.archie!().archie | ||
| --- | ||
| */ | ||
|
|
||
| // https://issues.dlang.org/show_bug.cgi?id=19035 | ||
|
|
||
| #line 8000 | ||
| @safe | ||
| { | ||
|
|
||
| void escape(int*); | ||
|
|
||
| /**********************/ | ||
|
|
||
| void frank()(ref scope int* p, int* s) | ||
| { | ||
| p = s; // should error here | ||
| } | ||
|
|
||
| void testfrankly() | ||
| { | ||
| int* p; | ||
| int i; | ||
| frank(p, &i); | ||
| } | ||
|
|
||
| /**********************/ | ||
|
|
||
| void betty()(int* p, int* q) | ||
|
Contributor
There was a problem hiding this comment. It's interesting that you chose to use a template here? Why is that? Is it to ensure the body of the function is needed in order to do the inference. For example, I noticed that Example 1 does not compile, but Example 2 does. Example 1 @safe:
void betty(int* p, int* q) // Notice this is not a template
{
p = q;
}
void testbetty()
{
int i;
int j;
betty(&i, &j); // Error: reference to local variable i assigned to non-scope parameter p calling betty
// Error: reference to local variable j assigned to non-scope parameter q calling betty
}Example 2 @safe:
void betty()(int* p, int* q) // Notice this is a template
{
p = q;
}
void testbetty()
{
int i;
int j;
betty(&i, &j);
}There's a lot you're not saying here, and I think you're taking a lot for granted expecting us to review the semantics of this PR.
Member
Author
There was a problem hiding this comment. Templates have the scope-ness of their parameters (and other attributes) inferred. Regular functions do not. Yes, I did take such knowledge for granted. Now you know! |
||
| { | ||
| p = q; | ||
| escape(p); | ||
| } | ||
|
|
||
| void testbetty() | ||
| { | ||
| int i; | ||
| int j; | ||
| betty(&i, &j); // should error on i and j | ||
| } | ||
|
|
||
| /**********************/ | ||
|
|
||
| void archie()(int* p, int* q, int* r) | ||
| { | ||
| p = q; | ||
| r = p; | ||
| escape(q); | ||
| } | ||
|
|
||
| void testarchie() | ||
| { | ||
| int i; | ||
| int j; | ||
| int k; | ||
| archie(&i, &j, &k); // should error on j | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybeScopes,possibleScopeVariablesor some variations of those?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of names that are not understandable.
maybes, "maybe" what?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several cases where D arrays are used [1] [2] and you're usually arguing for D string instead of C strings, which are D arrays. Also, using a D array would make
addMaybenot necessary because D arrays don't need to be newed.[1] https://github.com/dlang/dmd/blob/master/src/dmd/dclass.d#L56
[2] https://github.com/dlang/dmd/blob/master/src/dmd/dclass.d#L189