[scope] improve 'scope' and 'return' inference for delegates

[WalterBright]

No description provided.

[dlang-bot]

Fix	Bugzilla	Description
✓	17123	[REG 2.073] Issues with return @safe inference

[mathias-lang-sociomantic]

Does that mean those are implicitly convertible now ?

[WalterBright]

yes - I had gotten them wrong before.

[Geod24]

No you didn't. The following snippet (rightfully) doesn't compile with master, but does with your P.R.:

struct Bar { int i; }

alias FunDG = Bar* delegate () @safe;

void main () @safe
{
    Bar* b = getBar();
}

Bar* getBar () @safe
{
    Bar b;
    scope FunDG x = () return { return &b; };
    return forward(x);
}

Bar* forward (scope FunDG escaper) @safe
{
    return escaper();
}

[WalterBright]

You're right. Will fix.

[WalterBright]

If FunDG itself is declared as delegate () scope then the error is diagnosed. So the trouble is adding scope on later. Will investigate.

[mihails-strasuns]

Wrong base branch, must be stable.

[WalterBright]

Wrong base branch, must be stable.

I did a separate one for stable:
#6497

[mihails-strasuns]

I did a separate one for stable

Close this one in this case. The fix will get into master automatically when @MartinNowak merges stable branch after 2.073.1

It was also not necessary to create an new PR - GitHub now allows to change different base branch when editing the PR.

[WalterBright]

This needs to be reopened. This fix is much more complete, and is needed.

[dnadlinger]

Seems like this is a superset of #6497? Definitely separate the new changes into their own commit then, and it will be up to @MartinNowak how to sequence the merge.

[WalterBright]

Well, maybe I should just redo it as a separate PR, to avoid confusion. I'll think about it. It would need #6497 merged into master, though.

[WalterBright]

@MartinNowak doesn't need to sequence the merge. He can just merge it - I'll rebase this one when he does.

[MartinNowak]

Please rebase, stable change already in master since #6498.

[WalterBright]

Rebase done.

[WalterBright]

Currently blocked by dlang/druntime#1753

[MartinNowak]

Looking at dlang/druntime#1753, this will be fairly unusable in practice. People have added scope to delegate parameters basically everywhere to avoid GC closure allocations, now requiring to pass a delegate that actually is scope will break lots of code. Let's please do this as a deprecation.

[WalterBright]

@MartinNowak the changes to druntime are necessary because druntime is now compiled with -dip1000. This does not affect non-dip1000 builds.

As the numerous druntime and phobos PRs I've submitted attest, compiling with -dip1000 will need changes here and there. There is no other way I can think of - and that's why it's behind a switch. The transition period will be a long one.

It's turning out to be a bit tricky to get code to compile both with and without -dip1000, but it's worth the effort.

[mathias-lang-sociomantic]

People have added scope to delegate parameters basically everywhere to avoid GC closure allocations, now requiring to pass a delegate that actually is scope will break lots of code.

I'm not sure I follow your comment. The issue in the linked P.R. seems to be about a scope return, as opposed to a scope delegate (e.g. the missing scope on the RHS is the issue, not the one on the LHS).

[WalterBright]

The scope attribute, whether it is on the left or right side, refers to the this parameter, i.e. the .funcptr as that is the value that should not escape.

[andralex]

Auto-merge toggled on

[MartinNowak]

As the numerous druntime and phobos PRs I've submitted attest, compiling with -dip1000 will need changes here and there. There is no other way I can think of - and that's why it's behind a switch. The transition period will be a long one.

It's turning out to be a bit tricky to get code to compile both with and without -dip1000, but it's worth the effort.

Let me be a bit clearer on my comment (#6495 (comment)).
Of course everything is behind the -dip1000 switch for now. But the change in this PR will affect much more people than most of the others, b/c scope for delegate parameters were one of the few use-cases of scope prior to DIP1000.
Requiring people to rewrite all their code and dependencies before being able to even test -dip1000 will hamper adoption. So for this scope check it would be particularly helpful if it were a deprecation instead of an error (both behind -dip1000 of course).

[WalterBright]

Requiring people to rewrite all their code and dependencies before being able to even test -dip1000 will hamper adoption.

I don't see another way. It's taking me a while to work through Phobos on this, and sometimes the rat's nest of circular dependencies in there make things difficult. But the ends are worth it. (Heck, we're still working on making Phobos @safe!) It will be quite a while until we can make -dip1000 the default.

[MartinNowak]

I don't see another way.

Just make it a deprecation instead of an error, so that things can be adopted incrementally.

[MartinNowak]

Remaining unexpected test failure/diff.

fail_compilation/retscope.d(1867): Error: retscope.foo22 called with argument types (char[]) matches both:
fail_compilation/retscope.d(1860):     retscope.foo22!().foo22(ref char[] s)
and:
fail_compilation/retscope.d(1894):     retscope.foo22!().foo22(ref char[] s)

[WalterBright]

Blocked by dlang/druntime#1762