Fix issue 20881, 22790 - make ref-return-scope consistent

[dkorpel]

I'm working on the same issue as Walter: #13691

It's not finished, Phobos doesn't compile because sometimes methods such as std.typecons.Tuple.opBinary!"~" are inferred return ref when they should be return scope, which I'm still debugging. I'm already opening this to show @WalterBright the parts of the code I think need to be changed to properly fix issue 22790.

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkorpel! 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
✓	20881	normal	[DIP1000] scope inference turns return-ref into return-scope
✓	22790	enhancement	ref-return-scope is always ref-return, scope, unless return-scope appear in that order

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

[dkorpel]

I've found the bug: escape.d modifies the TypeFunction before attribute inference, which might get replaced at the end of semantic3 of a FuncDecl. This involves copying over the storage classes, and returnScope was forgotten in certain copy functions. This should be ready to go now, fixing 2 issues and unblocking #12689

The diff will be smaller when #13789 is merged first.

[WalterBright]

I already have a (much simpler) PR that fixes this.

Dennis, I know where I'm going with this. Trying to do it another way is just confusing, we are working at cross-purposes.

[dkorpel]

I already have a (much simpler) PR that fixes this.

No, that only fixes a tiny part of it. I'm not making superfluous code changes for the kicks, visit(CallExp e) has to change to work properly with the new rules.

Dennis, I know where I'm going with this. Trying to do it another way is just confusing, we are working at cross-purposes.

I'm not doing it another way, you introduced STC.returnScope_ and buildScopeRef, I just make dmd actually use it.

[WalterBright]

Instead of return scope, rewrite as scope return.

[dkorpel]

Okay. I didn't add scope back because it gets stripped anyway since S700 has no (pointer) members.

[dkorpel]

Done

[WalterBright]

Ok, I studied this for a bit. The issue is there are 3 ways of representing the dip1000 bits:

1. storage class
2. funcflags
3. ScopeRef

This adds an isreturnscope to funcflags to mimic the behavior of returnScope as a storage class.

I need to think about this a bit more.

[WalterBright]

How about this sequence of PRs:

1. simply introduce isreturnscope, setting it correctly in the FuncFlags
2. use isreturnscope to set returnScope in the storage class for the this parameter
3. use ScopeRef in the escape analysis functions for the this parameter

Then it should be in a good position to fix bugs with it.

[WalterBright]

Also, please rebase this PR, as the changes in #13802 are not reflected in the git diffs.

[dkorpel]

How about this sequence of PRs:

Sure

1. simply introduce isreturnscope, setting it correctly in the FuncFlags

That would be #13789

[WalterBright]

See #13811 for next step.

[WalterBright]

And now #13817

[dkorpel]

How about this sequence of PRs:
Then it should be in a good position to fix bugs with it.

Those 3 PRs are merged now. I looked into splitting this PR up further, but found it hard to isolate a fix for issue 20881 without also incorporating changes needed for 22790.