fix Issue 19035 - Escape in scope inference, improve scope inference #8408

WalterBright · 2018-06-27T00:40:49Z

I.e. when two or more parameters are mutually dependent on each other for scope inference, this detects more cases.

dlang-bot · 2018-06-27T00:40:50Z

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	19035	normal	Escape in scope inference, improve scope inference

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8408"

WalterBright · 2018-06-27T03:26:33Z

don't pull needs work

jacob-carlborg · 2018-06-27T07:19:15Z

don't pull needs work

Give us a ping when it's ready for review.

WalterBright · 2018-06-27T08:54:42Z

I'm surprised this slipped through all the tests! I guess it is good to go. Ping @jacob-carlborg

jacob-carlborg · 2018-06-27T09:27:57Z

test/fail_compilation/retscope6.d

+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


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.

I do multiple blocks with a #line precisely because they are easier to update. Adding a line doesn't require updating every message line.

./run.d fail_compilation AUTO_UPDATE=1 for the win...

Yeah, this only works with a single output section.

jacob-carlborg · 2018-06-27T09:50:56Z

src/dmd/declaration.d

    Expression edtor;               // if !=null, does the destruction of the variable
    IntRange* range;                // if !=null, the variable is known to be within the range

+    VarDeclarations* maybes;        // STC.maybescope variables that are assigned to this STC.maybescope variable


Can this be a D array?

The comment should be a Ddoc comment

Can we call this maybeScopes, possibleScopeVariables or some variations of those?

No, as it's variable sized, and we generally avoid D arrays in the front end for that reason.

As the rest or not, such a change should await that

I'm not a big fan of long names

I'm not a big fan of long names

I'm not a fan of names that are not understandable. maybes, "maybe" what?

No, as it's variable sized, and we generally avoid D arrays in the front end for that reason.

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 addMaybe not 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

jacob-carlborg · 2018-06-27T09:53:16Z

src/dmd/escape.d

+ * complete, we can finalize this by turning off
+ * maybescope for array elements that cannot be scope.
+ *
+ *  va      v      =>  va     v


Are va and v referring to the foreach variables in the source below? If that’s the case please quote them with backticks to make that more clear.

jacob-carlborg · 2018-06-27T10:27:45Z

src/dmd/escape.d

+    //printf("inferScope = %d, %d\n", inferScope, (va.storage_class & STCmaybescope) != 0);
+
+    // Determine if va is a parameter that is an indirect reference
+    bool vaIsRef = va && va.storage_class & STC.parameter &&


Can be made const.

Addressed all points

JinShil · 2018-06-28T01:12:32Z

Am I the only one drowning in DIP1000 sea? Can we please document all of this first, and then move to implementation? Or at the very least, provide some formal documentation with these PRs?

This PR could also use an elaborate problem statement. There's no bugzilla issue, so I don't understand what the problem is that this solves. How are we supposed to be able to predict the behavior of the compiler with all this inference magic? We'll be debugging our understanding of the language before we ever get to debugging our code. (cue Scott Meyers "The last thing D needs")

WalterBright · 2018-06-28T02:25:35Z

I thought the test case was pretty clear. Errors that are spurious, and errors that should be diagnosed.

JinShil · 2018-06-28T02:29:33Z

I thought the test case was pretty clear.

Sorry, but it's not for me. Perhaps you could augment it with some helpful comments.

JinShil · 2018-06-28T02:47:21Z

test/fail_compilation/retscope6.d

+    static void emplace4(Args...)(scope ref S s, scope out S t, scope Args args) @safe
+    {
+        s.payload = args[0];
+        t.payload = args[0];


Due to the lack of documentation surrounding the semantics of scope I'm assuming that in this context, scope means that args[0]'s lifetime must not exceed the lifetime of this emplace4's scope. However, since we are assigning to a ref and out we are escaping out of emplace4's scope. Is this correct?

When I analyze the implementation of foo, however, there's no escape because everything is local to foo. Perhaps this could use a more compelling test that would actually result in a leak or memory corruption without this PR.

I'll add some more documentation.

@WalterBright Is it really too much to ask to have you open a dlang.org PR to document the semantics of scope and return function parameters with illustrative examples so we can refer to it when we review these PRs? Or are you still trying to figure all this out, and this is your way of working through it?

It's documented in DIP25 and DIP1000. I'd start with a gentler introduction, http://dconf.org/2017/talks/bright.html

JinShil · 2018-06-28T07:14:12Z

test/fail_compilation/retscope6.d

+
+/**********************/
+
+void betty()(int* p, int* q)


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.

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!

JinShil · 2018-06-28T09:20:42Z

test/fail_compilation/retscope6.d

+        s.payload = args[0];
+    }
+
+    static void emplace4(Args...)(scope ref S s, scope out S t, scope Args args) @safe


https://dlang.org/spec/attribute.html#scope

scope cannot be applied to globals, statics, data members, ref or out parameters.

This is violating the spec. The spec needs an update.

https://dlang.org/spec/attribute.html#scope

Assignment to a scope, other than initialization, is not allowed.

Also not accurate.

As I've mentioned before, dip1000 has not been folded into the spec.

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?

Why hasn't it been done?

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.

One large reason is because I can't get my spec fixes pulled.

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.

There is another issue. I used to work at Boeing in design...

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 scope is being inferred, and I don't like what I'm seeing in the implementation so far. I think we need a way to set storage classes meta-programmatically, rather than reaching for inference.

WalterBright · 2018-06-29T07:19:47Z

I need to understand what these PRs are doing and why.

I won't argue that it is simple. But what do you suggest we do with a 2 person rule when nobody else understands the compiler?

jacob-carlborg · 2018-06-29T08:29:29Z

I won't argue that it is simple. But what do you suggest we do with a 2 person rule when nobody else understands the compiler?

We need to incrementally try to get more people to understand the compiler.

JinShil · 2018-06-29T10:20:40Z

But what do you suggest we do with a 2 person rule when nobody else understands the compiler?

IMO, if you are determined, highly confident in your solution, and you don't receive any feedback or don't receive feedback that you consider significant to change your position, then after a reasonable amount of time, you're in a unique position to just merge away.

But, I'd prefer that you did more to bring others along with you. With the lack of documentation about these inference rules (formal or informal) I don't think you're doing enough to justify this PR to the team. Though it could just be me.

This is an important feature, and I don't recommend anyone hold up this PR based on my objections alone. I won't protest if my objections are overruled, and may even merge this myself if I can be convinced that it's the right solution.

wilzbach

I'm going to merge this now on the understanding that DIP1000 is still under work/ in flux and it needs to move forward (and this is indeed a nice improvement).
However, I agree with @JinShil that if we want -dip1000 to be a success there needs to be a better plan and roadmap.

WalterBright force-pushed the maybescope branch 3 times, most recently from 8472010 to a8d9fec Compare June 27, 2018 01:39

WalterBright added the Review:Needs Work label Jun 27, 2018

WalterBright force-pushed the maybescope branch from a8d9fec to 0076f1d Compare June 27, 2018 08:29

dlang-bot removed the Review:Needs Work label Jun 27, 2018

jacob-carlborg reviewed Jun 27, 2018

View reviewed changes

jacob-carlborg suggested changes Jun 27, 2018

View reviewed changes

jacob-carlborg previously requested changes Jun 27, 2018

View reviewed changes

WalterBright force-pushed the maybescope branch from 0076f1d to 1d8da05 Compare June 27, 2018 23:42

WalterBright added the Review:Vision Vision Plan https://wiki.dlang.org/Vision/2018H1 label Jun 27, 2018

JinShil reviewed Jun 28, 2018

View reviewed changes

fix Issue 19035 - Escape in scope inference, improve scope inference

9475d86

WalterBright force-pushed the maybescope branch from 1d8da05 to 9475d86 Compare June 28, 2018 04:54

dlang-bot added the Severity:Bug Fix label Jun 28, 2018

WalterBright changed the title ~~improve inference of 'scope' parameters~~ fix Issue 19035 - Escape in scope inference, improve scope inference Jun 28, 2018

JinShil reviewed Jun 28, 2018

View reviewed changes

wilzbach approved these changes Jul 1, 2018

View reviewed changes

wilzbach merged commit d917058 into dlang:master Jul 1, 2018

This was referenced Jul 3, 2018

Wrong filetype with a changelog entry dlang/phobos#6624

Merged

Merge remote-tracking branch 'upstream/stable' into merge_stable dlang/phobos#6625

Merged

[WIP] Re-enable -dip1000 on auto-tester + fix CircleCi dlang/phobos#6626

Merged

WalterBright deleted the maybescope branch July 10, 2018 21:48

ibuclaw mentioned this pull request Aug 4, 2018

Add missing fields C++ headers #8536

Merged

JinShil mentioned this pull request Aug 18, 2018

fix Issue 19097 - Extend Return Scope Semantics #8504

Merged

thewilsonator mentioned this pull request Aug 23, 2018

Document recent -dip1000 changes dlang/dlang.org#2453

Merged

Uh oh!

fix Issue 19035 - Escape in scope inference, improve scope inference #8408

fix Issue 19035 - Escape in scope inference, improve scope inference #8408

Uh oh!

Conversation

WalterBright commented Jun 27, 2018

Uh oh!

dlang-bot commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

WalterBright commented Jun 27, 2018

Uh oh!

jacob-carlborg commented Jun 27, 2018

Uh oh!

WalterBright commented Jun 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Jun 28, 2018

Uh oh!

WalterBright commented Jun 28, 2018

Uh oh!

JinShil commented Jun 28, 2018

Uh oh!

JinShil Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlang-bot commented Jun 27, 2018 •

edited

Loading

jacob-carlborg Jun 28, 2018 •

edited

Loading

JinShil Jun 28, 2018 •

edited

Loading

JinShil Jun 28, 2018 •

edited

Loading

WalterBright Jun 29, 2018 •

edited

Loading

JinShil commented Jun 29, 2018 •

edited

Loading