return scope: first support #5972

WalterBright · 2016-07-25T06:36:24Z

Works much like return ref.

Fix https://issues.dlang.org/show_bug.cgi?id=5270
Fix https://issues.dlang.org/show_bug.cgi?id=8993
Fix https://issues.dlang.org/show_bug.cgi?id=14238
Fix https://issues.dlang.org/show_bug.cgi?id=15544
Fix https://issues.dlang.org/show_bug.cgi?id=15996

mathias-lang-sociomantic · 2016-07-25T18:02:05Z

The commit message / P.R. message is highly insufficient for most people to understand what the design is.
I don't see a changelog entry, nor a dlang.org P.R. linked. How is anyone supposed to review this P.R. if the only thing available is the diff ?

WalterBright · 2016-07-26T05:18:43Z

https://wiki.dlang.org/DIP90

JackStouffer · 2016-07-26T05:44:21Z

It seems that DIP 90 was never discussed with the community, as I couldn't turn up anything on the forum
The DIP says draft, was it accepted?
If it wasn't offically accepted, then it should probably go through the new DIP process

iain-buclaw-sociomantic · 2016-07-26T12:25:48Z

So how does (return scope int* p) differ from (int* p)?

WalterBright · 2016-07-26T22:36:10Z

@iain-buclaw-sociomantic it enables the compiler to see which arguments are being returned by a function, so it can see if those arguments are escaping. It's analogous to how return ref works.

wilzbach · 2016-08-03T05:49:29Z

It seems that DIP 90 was never discussed with the community, as I couldn't turn up anything on the forum
The DIP says draft, was it accepted?
If it wasn't offically accepted, then it should probably go through the new DIP process

The new DIP process is intended to be different for Walter. To quote:

Language changes initiated by language authors are also supposed to go through the DIP queue. By their very nature formal approval is not needed. Hence they are processed slightly different and an increased focus is put into bringing community attention and feedback.
At the time of writing this document only Walter Bright and Andrei Alexandrescu are meant as a language authors here.

@WalterBright there is a conversion tool from MediaWiki to Markdown, I just ran it for DIP90 and cleaned its output a bit. How about submitting DIP90 to the new DIP list? I think no matter whether it's draft or already decided, this should be the intended process. Is this correct @Dicebot?

mihails-strasuns · 2016-08-05T10:13:43Z

I have already converted DIP90 to new format myself and sent to @WalterBright via e-mail for submitting to the queue. Will start forum discussion as soon as it finally happens ;)

mihails-strasuns · 2016-08-05T10:14:56Z

(but it was ages ago thus there must be some misunderstanding here)

WalterBright · 2016-08-06T05:53:50Z

@Dicebot it's there now.

jacob-carlborg · 2016-08-17T06:24:00Z

What about the -scope flag that the DIP mentions. Shouldn't that be the first step, or included in this PR?

mihails-strasuns · 2016-08-17T10:47:33Z

OK, for the record - we had tele-meeting discussing further plan to move with DIP1000 (https://github.com/dlang/DIPs/blob/master/DIPs/DIP1000.md) which will be announced on NG by the end of this week in more details.

But essentials regarding this PR is that @WalterBright will continue work on this PR as he sees fit until it is more feature complete. Reviewing the source patches is still welcome but please don't worry about actual feature set and compiler flags while the [WIP] is there in the title.

jacob-carlborg · 2016-08-17T11:15:41Z

Reviewing the source patches is still welcome but please don't worry about actual feature set and compiler flags while the [WIP] is there in the title.

Fair enough, as long as it doesn't get merged without compiler flags.

dlang-bot · 2016-08-21T08:13:13Z

@WalterBright, thanks for your PR! By analyzing the annotation information on this pull request, we identified @yebblies, @9rnsr and @MartinNowak to be potential reviewers. @yebblies: The PR was automatically assigned to you, please reassign it if you were identified mistakenly.

(The DLang Bot is under development. If you experience any issues, please open an issue at its repo.)

codecov-io · 2016-08-22T05:23:54Z

Current coverage is 87.37% (diff: 89.28%)

Merging #5972 into scope will decrease coverage by 0.05%

@@              scope      #5972   diff @@
==========================================
  Files           107        104      -3   
  Lines         61712      57703   -4009   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
- Hits          53954      50416   -3538   
+ Misses         7758       7287    -471   
  Partials          0          0

Powered by Codecov. Last update 9e6f8a7...48b7815

dlang-bot · 2016-08-23T02:10:46Z

Fix	Bugzilla	Description
✓	8838	Slicing static arrays should be considered unsafe (@System)
✓	14238	DIP25: escape checks can be circumvented with delegate
✓	15544	Escaping fields to a heap delegate must be disallowed in @safe code

@safe

…ed in @safe code

…System

…System)

WalterBright · 2016-11-01T09:15:42Z

@MartinNowak I rebased this on master, and now it has 6000 different lines. Uh-oh, maybe this has something to with it going on a feature branch?

MartinNowak · 2016-11-01T09:19:00Z

@MartinNowak I rebased this on master, and now it has 6000 different lines. Uh-oh, maybe this has something to with it going on a feature branch?

No worries, already working on it ;). I merged master into the scope base branch so that the diff should be the actual PR again.
But just like with the stable branch we shouldn't drag in master unless really necessary.

WalterBright · 2016-11-01T09:20:42Z

Thanks, @MartinNowak , you're a life saver!

MartinNowak · 2016-11-01T09:23:23Z

Seems like github isn't updating the diff view, but my local diff looks fine.
I was about to pull this anyhow, so we have a discussion base for the call today.
Also PRs on feature branches shouldn't become so big, their point is incremental work on the feature.

WalterBright · 2016-11-01T09:33:27Z

Yeah, I wasn't thinking when I rebased it. I tend to routinely rebase things so they don't diverge too far.

My diff view isn't changing, either. Maybe if I rebased it again? :-)

MartinNowak · 2016-11-01T10:28:37Z

Yeah, I wasn't thinking when I rebased it. I tend to routinely rebase things so they don't diverge too far.

Just like stable it has a different base branch, please rebase this branch against upstream/scope next time.
Actually there are few reasons to rebase it, unless we really need something from master.

My diff view isn't changing, either. Maybe if I rebased it again? :-)

Yes, GH only updates on branch changes to limit the repercussions of a commit on master.

MartinNowak

Looks reasonable with that few questions and suggestions.
As said before starring at the low-level code is no replacement for properly reviewing the design. It's not impossible to infer a high-level implications for a low-level escape checker.

MartinNowak · 2016-11-01T09:25:48Z

src/mtype.d

@@ -6247,9 +6247,6 @@ extern (C++) final class TypeFunction : TypeNext
                        fparam.storageClass |= STCscope;        // 'return' implies 'scope'
                        if (tf.isref)
                        {
-                            error(loc, "parameter %s is 'return' but function returns 'ref'",
-                                fparam.ident ? fparam.ident.toChars() : "");
-                            errors = true;


Test case for that would have been nice.

MartinNowak · 2016-11-01T09:31:53Z

src/escape.d

@@ -53,20 +53,19 @@ bool checkAssignEscape(Scope* sc, Expression e, bool gag)
    if (e1.op == TOKslice)
        return false;

-    VarDeclarations byref, byvalue;
-    Expressions byexp;
+    EscapeByResults er;


That's a very undescriptive name, as usual any translation a reader has to juggle in mind while going through the code makes understanding harder, a bit similar to using rare loanwords in articles.
How about Escapes instead? I wouldn't argue if that name didn't hinder my understanding of the code.

MartinNowak · 2016-11-01T09:36:14Z

src/escape.d

@@ -41,11 +41,13 @@ import ddmd.arraytypes;
 */
 bool checkAssignEscape(Scope* sc, Expression e, bool gag)
 {
-    if (e.op != TOKassign && e.op != TOKblit)
+    //printf("checkAssignEscape(e: %s)\n", e.toChars());
+    if (e.op != TOKassign && e.op != TOKblit && e.op != TOKconstruct)


Looks like it's not strictly part of the commit Issue to fix 15544.

MartinNowak · 2016-11-01T09:38:08Z

src/escape.d

        return false;

    VarDeclaration va;
+    while (e1.op == TOKdotvar)
+        e1 = (cast(DotVarExp)e1).e1;


Sure about this? Couldn't one of the DotVarExps return a value with an independent lifetime?

MartinNowak · 2016-11-01T09:42:56Z

src/escape.d

+    for (auto p = fd.parent; p; p = p.parent)
+    {
+        auto fdp = p.isFuncDeclaration();
+        if (fdp)


Please get accustomed to early return/continue to avoid such deep nesting ;).

ìf (fdp is nulll) continue;

MartinNowak · 2016-11-01T10:01:18Z

src/func.d

+                    assert(!(p.storageClass & STCmaybescope));
+                }
+            }
+        }


Shouldn't this unset STCreturn if that doesn't apply?

MartinNowak · 2016-11-01T10:07:55Z

src/declaration.d

@@ -1016,6 +1016,8 @@ extern (C++) class VarDeclaration : Declaration
 {
    Initializer _init;
    uint offset;
+    uint sequenceNumber;            // order the variables are declared


order the variables are declared???
Thought you were the native speaker :).
Did you mean order the declaration of variables?

MartinNowak · 2016-11-01T10:12:52Z

src/declaration.d

@@ -1016,6 +1016,8 @@ extern (C++) class VarDeclaration : Declaration
 {
    Initializer _init;
    uint offset;
+    uint sequenceNumber;            // order the variables are declared
+    __gshared uint nextSequenceNumber;   // the counter for sequenceNumber


Also seems to work for nested scopes. Could we have some weird semantic order of nested functions or so that conflicts with this sequence number scheme?

MartinNowak · 2016-11-01T10:14:42Z

src/escape.d

+        {
+            auto t = e.e1.type.toBasetype();
+            if (t.ty == Tstruct)
+                e.e1.accept(this);


Just a single layer, why not a.b.c?

MartinNowak · 2016-11-01T10:21:50Z

src/escape.d

+            /* v is not 'scope', and is assigned to a parameter that may escape.
+             * Therefore, v can never be 'scope'.
+             */
+            v.doNotInferScope = true;


Why can't this use the prevalent scheme of clearing STCmaybescope?
Maybe I'm looking at the commits in the wrong order, the PR is a bit messed up.

MartinNowak · 2016-11-01T10:32:45Z

In fact, please never rebase PRs, unless really necessary, you just killed half of my review comments ;), please expand the show outdated stuff.

MartinNowak · 2016-11-01T10:34:08Z

What about those @safe failures btw? https://travis-ci.org/dlang/dmd/jobs/172243053#L1070
We could build a preview if you can manage to fix those.

WalterBright · 2016-11-01T11:14:10Z

What about those @safe failures btw?

I don't know. My first thought is it has something to do with Dicebot's changes from the transition switch to deprecations.

WalterBright · 2016-11-01T11:14:31Z

rebase this branch against upstream/scope next time.

git fetch upstream scope

right?

mihails-strasuns-sociomantic · 2016-11-01T11:16:00Z

I don't know. My first thought is it has something to do with Dicebot's changes from the transition switch to deprecations.

I was just about to ask what is the current state of branches regarding merging stable into master and so on. I will need to fixup both master and this branch when it is done.

CyberShadow · 2017-07-02T11:14:32Z

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=17445

Specifically, commit 62be2fb.

WalterBright force-pushed the return-scope branch from 96b69e2 to 5210031 Compare July 25, 2016 06:58

WalterBright force-pushed the return-scope branch from 5210031 to 723f4cb Compare August 8, 2016 23:27

WalterBright changed the title ~~return scope: first support~~ [WIP] return scope: first support Aug 16, 2016

dlang-bot assigned yebblies Aug 21, 2016

WalterBright force-pushed the return-scope branch 2 times, most recently from 9be2540 to 6bf1c9d Compare August 22, 2016 04:56

WalterBright force-pushed the return-scope branch 2 times, most recently from abe2342 to bac69c5 Compare August 22, 2016 23:21

WalterBright force-pushed the return-scope branch 4 times, most recently from 0ba2d30 to d45f171 Compare August 24, 2016 07:25

WalterBright mentioned this pull request Aug 24, 2016

fix tempCString() to follow strict scope rules dlang/phobos#4746

Merged

WalterBright force-pushed the return-scope branch 2 times, most recently from e4428f3 to 4ecb21e Compare August 24, 2016 19:02

WalterBright added 10 commits November 1, 2016 02:13

fix Issue 14238 - DIP25: escape checks can be circumvented with delegate

7c3e47a

create EscapeByResults struct

56b904c

fix Issue 15544 - Escaping fields to a heap delegate must be disallow…

e61378a

…ed in @safe code

fix Issue 8838 - Slicing static arrays should be considered unsafe (@…

66187b5

…System)

add parameterStorageClass()

128f3fa

add inference of 'scope' for parameters for inferrable functions

d612c94

refactor: add Parameter.isCovariant()

57a0b21

add 'scope' support to 'this'

1cce2ee

add lifetime checks

75647f5

fix checking for struct fields

6d4de39

WalterBright force-pushed the return-scope branch from ca77791 to b2cb96c Compare November 1, 2016 09:13

know when to not infer 'scope'

225c713

WalterBright force-pushed the return-scope branch from b2cb96c to 225c713 Compare November 1, 2016 10:06

MartinNowak approved these changes Nov 1, 2016

View reviewed changes

MartinNowak merged commit 20add0d into dlang:scope Nov 1, 2016

WalterBright deleted the return-scope branch November 1, 2016 11:09

CyberShadow mentioned this pull request Jul 2, 2017

merge scope/DIP1000 preview #6388

Merged

dkorpel mentioned this pull request Jan 13, 2022

Fix issue 22298 - Nested function's scope parameters can be assigned to variables in enclosing function #13530

Merged

return scope: first support #5972

return scope: first support #5972

Conversation

WalterBright commented Jul 25, 2016 • edited

mathias-lang-sociomantic commented Jul 25, 2016

WalterBright commented Jul 26, 2016

JackStouffer commented Jul 26, 2016

iain-buclaw-sociomantic commented Jul 26, 2016

WalterBright commented Jul 26, 2016

wilzbach commented Aug 3, 2016

mihails-strasuns commented Aug 5, 2016

mihails-strasuns commented Aug 5, 2016

WalterBright commented Aug 6, 2016

jacob-carlborg commented Aug 17, 2016

mihails-strasuns commented Aug 17, 2016

jacob-carlborg commented Aug 17, 2016 • edited

dlang-bot commented Aug 21, 2016

codecov-io commented Aug 22, 2016 • edited

Current coverage is 87.37% (diff: 89.28%)

dlang-bot commented Aug 23, 2016 • edited

WalterBright commented Nov 1, 2016

MartinNowak commented Nov 1, 2016

WalterBright commented Nov 1, 2016

MartinNowak commented Nov 1, 2016

WalterBright commented Nov 1, 2016

MartinNowak commented Nov 1, 2016

MartinNowak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinNowak commented Nov 1, 2016 • edited

MartinNowak commented Nov 1, 2016 • edited

WalterBright commented Nov 1, 2016

WalterBright commented Nov 1, 2016

mihails-strasuns-sociomantic commented Nov 1, 2016

CyberShadow commented Jul 2, 2017

WalterBright commented Jul 25, 2016 •

edited

jacob-carlborg commented Aug 17, 2016 •

edited

codecov-io commented Aug 22, 2016 •

edited

dlang-bot commented Aug 23, 2016 •

edited

MartinNowak commented Nov 1, 2016 •

edited

MartinNowak commented Nov 1, 2016 •

edited