fix Issue 22790 - ref-return-scope is always ref-return, scope, unless return-scope appear in that order

[WalterBright]

No description provided.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	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#13691"

[WalterBright]

Azure heisenbug: https://issues.dlang.org/show_bug.cgi?id=22791

[dkorpel]

How would this break pre-dip1000 code? Lifetime errors only appear with dip1000.

[WalterBright]

Much of the scope code predates dip1000 and is enabled with or without the switch for it.

[dkorpel]

That's new to me, can you give a code example of a scope error triggered without dip1000?

[WalterBright]

see the attached test cases

[dkorpel]

That makes no sense for two reasons:

• those tests have REQUIRED_ARGS=-preview=dip1000
• if they didn't, then your PR would break non -preview=dip1000, which is what you said you didn't do because of this extra condition

[dkorpel]

There's some things missing:

• STC.returnScope is not propagated to a TypeFunction's FuncFlags, meaning it doesn't do anything on a struct's this parameter yet
• escapeByValue and escapeByRef still use the old logic on a CallExp instead of buildScopeRef or STC.returnScope
• checkAssignEscape doesn't infer STC.returnScope

Here's my WIP PR addressing this: #13693

[WalterBright]

There's some things missing:

I know, but (as usual) doing things incrementally is better than stuffing everything into one PR. I have explained the rationale for this ad nauseum. This piece of the puzzle stands on its own, and merits its own PR.

As for the other pieces you mention, a bugzilla issue for each would be worthwhile.

[WalterBright]

Now the heisenbug shifts to CyberShadow/DAutoTest

[dkorpel]

I know, but (as usual) doing things incrementally is better than stuffing everything into one PR.

I didn't know you knew, since this your third PR that says it fixes the ref-return-scope ambiguity, even though it only fixes one small part and leaves the rest in an inconsistent state. I'm not asking you to do everything in one PR, but please communicate your plan. I cannot review whether your puzzle piece is in the right place when I can't see anything surrounding it or the picture on the box.

As for the other pieces you mention, a bugzilla issue for each would be worthwhile.

If you're aware of the shortcomings of your PRs, why should I need to file new issues after each one?

[WalterBright]

If you're aware of the shortcomings of your PRs, why should I need to file new issues after each one?

So they don't get forgotten.

[dkorpel]

If you're aware of the shortcomings of your PRs, why should I need to file new issues after each one?

So they don't get forgotten.

It's harder to convey the emphasis in text form, but I meant if you are aware, why should I file them? I'm fine with filing issues and providing test cases, but me re-opening the same issue over and over doesn't feel like effective collaboration.

That's why I opened #13693, not because I think everything needs to be done in one PR, but because I want to communicate my plan so we're on the same wavelength.

[WalterBright]

@dkorpel good points, ok.

So what's holding up this PR?

[dkorpel]

So what's holding up this PR?

• Since this is a partial fix, either don't close the issue, or narrow the issue down to what this PR changes.
• See my comment about avoiding code breakage. There have been several issues in the past of unclear errors popping up because of weird use of global.params.useDIP1000, so I like to avoid such conditions when possible.

[WalterBright]

All the dip1000 fixes are partial fixes. I shall reiterate why I do things incrementally:

1. it makes PRs easy to understand
2. it makes PRs reviewable
3. if the PR breaks something, it's FAR FAR easier to discover the breaking change amid a forest of incremental changes than one large change

As for breakage, fixing DIP1000 will break code, which is why I put it behind the preview switch. It's the only way forward.

[dkorpel]

All the dip1000 fixes are partial fixes. I shall reiterate why I do things incrementally:

And I shall reiterate that I know why you do things incrementally and I am not against it, I'm asking you to not close a bugzilla issue by merging a PR with a partial fix for the described issue. Keep the issue open, or narrow the issue down and open issues for the rest.

2. it makes PRs reviewable

One thing I will say though is that it can be hard to review dead code that you commit because it's supposed to be used in a later PR. But again: This is not my objection.

As for breakage, fixing DIP1000 will break code, which is why I put it behind the preview switch. It's the only way forward.

You say that without anything backing it up. You didn't add -preview=dip1000 checks when you introduced STC.returnScope_ 7 months ago, why would changing its behavior break non-dip1000 code now?

[thewilsonator]

it makes PRs easy to understand
it makes PRs reviewable

It makes it easy to understand and review in the same way that being able to see only a metre out in front of you makes it easy to navigate. It is impossible for reviews to see the forest for the (singular) tree.

if the PR breaks something, it's FAR FAR easier to discover the breaking change amid a forest of incremental changes than one large change

That is completely orthogonal to the problem that we can't tell where (the general) this is going. You could for instance open subsequent PRs that are dependant on the previous one so that the reviewers are not short sighted. Also multi commit PRs are a thing, and as long as they are not squashed they are equivalent to a series of PRs.

[WalterBright]

You say that without anything backing it up.

See the test cases in this PR.

I'm asking you to not close a bugzilla issue by merging a PR with a partial fix for the described issue

I've been meaning to speak to you about this. You've been reopening issues when a case not described in the bugzilla shows the fix was incomplete. While this is semantically correct, in practice, it makes life difficult. The bugzilla becomes a meandering discussion with multiple PRs, and trying to figure out what goes with what is confusing and frustrating. What works better is creating a separate bugzilla that references the previously closed one, and supplies the additional failing test case. Then there's a clear chain of successful PRs lining up with what issues they resolve.

It's the same idea as keeping PRs small and reviewable.

why would changing its behavior break non-dip1000 code now?

Because I'm doing things incrementally, for all the reasons I repeatedly explain.

If you want to change the bugzilla title, that's ok with me.

[WalterBright]

That is completely orthogonal to the problem that we can't tell where (the general) this is going.

The ref-return-scope triple means "ref return-scope" when return immediately precedes scope, otherwise it means "ref-return scope". That's it. The details of implementing it are complicated, but it's just implementation details.

[thewilsonator]

That is completely orthogonal to the problem that we can't tell where (the general) this is going.

The ref-return-scope triple means "ref return-scope" when return immediately precedes scope, otherwise it means "ref-return scope". That's it. The details of implementing it are complicated, but it's just implementation details.

This was in reference to your preference for incremental PRs, the general this, not this, ref-return-scope.

To be blunt, don't expect to get your PRs reviewed if the reviewers can't tell:
* where the PR is going, or
* why it is being added

[WalterBright]

My experience is complex PRs don't get reviewed at all. As for this one, the purpose fits in the title rather nicely.

[WalterBright]

Multi-commit PRs are not a very good substitute. There's no information on whether they pass the test suite or not, making them rather useless in trying to isolate which change caused the problem.

[WalterBright]

@thewilsonator I don't know your experience, but I've had many where the problem was isolated to a PR that had 800 lines of random code changes spread out over a large number of files. They're a freakin' nightmare to figure out. Even if I wrote them, let alone someone else.

Small, incremental, each change passes the test suite, is a method evolved from 40 years of bitter experience. It works, I know it works, I've done it the other way and suffered for it. Have you ever spent an entire week trying to figure out where something is going wrong?

[WalterBright]

There's like 3 possible interpretations depending on whether you look at the specification, one part of dmd, or another part of dmd.

There's only 2, depending on the return type. It works as designed.

After this PR there will be 4 I think.

No, there will be only 1. That's the point.

[WalterBright]

Because that code is compiled with -preview=dip1000,

And it had to CHANGE to get the same semantics.

Please read the spec:

https://dlang.org/spec/function.html#ref-return-scope-parameters

[dkorpel]

There's only 2, depending on the return type. It works as designed.

Yes, there's two cases of ref + return scope and return ref + scope, but I'm talking about 3 ways to resolve the ambiguity.

• spec says: look at whether function returns by ref
• lifetime analysis of function calls says: look at whether function returns by ref and whether the type has pointers
• escape checking says: favor return scope in certain cases when the keywords appear in that order

No, there will be only 1. That's the point.

• this PR adds: look at whether the code is compiled with -preview=dip1000

[WalterBright]

this PR adds

It does not add. It replaces.

[dkorpel]

Because that code is compiled with -preview=dip1000,

And it had to CHANGE to get the same semantics.

Yes, it had to change because it uses -preview=dip1000. I fully understand the change and support it for -preview=dip1000 code, I'm asking for an argument supporting the "don't change non-dip1000 code" part.

Please read the spec:

I did.

[WalterBright]

I'm asking for an argument supporting the "don't change non-dip1000 code" part.

Don't break existing code.

[dkorpel]

It does not add. It replaces.

We go from:

if (!returnByRef && isRefReturnScope(fparam.storageClass))

To:

if (global.params.useDIP1000 != FeatureState.enabled &&
        !returnByRef && isRefReturnScope(stc))

Notice how there's an extra &&, an extra condition to branch on. This adds complexity.

[dkorpel]

Don't break existing code.

#13691 (comment)

The three bullet points, that's what I'm asking for.

[WalterBright]

I'm sorry, I don't get your confusion. This PR changes how ref-return-scope behaves, it broke parts of the test suite that tested the previous behavior, and you don't accept that as evidence the behavior changed?

[WalterBright]

If I post any more tonight, I'm going to say something I'll regret. So I'm going to sign off for today. I recommend you go over what I wrote again. I don't really know how to explain it any differently.

[MoonlightSentinel]

@thewilsonator I don't know your experience, but I've had many where the problem was isolated to a PR that had 800 lines of random code changes spread out over a large number of files. They're a freakin' nightmare to figure out. Even if I wrote them, let alone someone else.

Which is only a problem of large commits, not PR's containing multiple commits. git bisect[1] (or Vladimirs wrapper digger bisect[2]) can automatically track down errors in the history. In fact, that is exactly what happens when searching for a bug introduced by several small and isolated PR's.

[1] Binary search in Git
[2] Digger

---

But here's a proposal that should work for both of you: Create a draft PR to implement a general solution to the ambiguity and extract standalone patches when applicable. That will allow @dkorpel and others to provide feedback /contribute patches while also enforcing that the individual patches are properly structured (build & pass all tests).

[dkorpel]

I'm sorry, I don't get your confusion

I'm sorry as well for sounding like a broken record, but:

This PR changes how ref-return-scope behaves

For code using -preview=dip1000.

it broke parts of the test suite that tested the previous behavior,

Which used -preview=dip1000.

and you don't accept that as evidence the behavior changed?

Of course I do, the behavior changed for code using -preview=dip1000, I understand! I approve of the change!

We're talking about the decision to not change code without -preview=dip1000. You're saying "these changes broke code using -preview=dip1000, so I put the changes behind the -preview=dip1000 switch only". That just doesn't make sense. The decision only makes sense if code not using -preview=dip1000 broke. I don't know how to make this any clearer.

[WalterBright]

It's a new behavior for dip1000. The previous behavior matched the non-dip1000 behavior.

[WalterBright]

It also alters the name mangling.

[dkorpel]

It's a new behavior for dip1000. The previous behavior matched the non-dip1000 behavior.

And the suggestion is to make the new dip1000 behavior match the new non-dip1000 behavior instead of introducing a discrepancy.

Then you say: but that would break non-dip1000 code!
Then I say: which code?
Then you say: look at the test suite diff
Then I say: that has -preview=dip1000, it's not non-dip1000 code

And this somehow repeats every time, I don't know how to get out of this loop.

[dkorpel]

It also alters the name mangling.

Doesn't this mean that after this PR dip1000 code won't link with non-dip1000 code anymore? That doesn't sound desirable.

[WalterBright]

It means older code will have to recompile all their libraries, even though they aren't using -preview.

1. Using -preview means accept breakage.
2. Not using -preview means expect no breakage.

People have made (2) very clear to us.

[dkorpel]

It means older code will have to recompile all their libraries, even though they aren't using -preview.

I don't think we enforce binary compatibility between DMD versions.

In any case, since you brought up name mangling, I can finally stop arguing that the inferReturnScope function might be unnecessary, and simply demonstrate it's actually causing code breakage instead of preventing it:

// module a.d
int* f(return ref scope int* x)
{
    static int* y;
    return y;
}

// module b.d
import a;

void main()
{
    int* x;
    f(x);
}

dmd -m64 -preview=dip1000 -c a.d
dmd -m64 -c b.d
dmd -m64 a.obj b.obj

b.obj : error LNK2019: unresolved external symbol _D1a1fFNkMKPiZQd referenced in function _Dmain

The mangle is now inconsistent between dip1000 code and non-dip1000 code, meaning regular (non-preview) code can fail to link against Phobos on functions where that pattern occurs.

[ibuclaw]

@WalterBright please update the tests before merging, now all pipelines are broken.

[ibuclaw]

==============================
Test 'fail_compilation/test15191.d' failed: 
expected:
----
fail_compilation/test15191.d(35): Error: returning `&identity(x)` escapes a reference to local variable `x`
fail_compilation/test15191.d(41): Error: returning `&identityPtr(x)` escapes a reference to local variable `x`
fail_compilation/test15191.d(47): Error: cannot take address of `ref return` of `identityPtr()` in `@safe` function `addrOfRefTransitive`
fail_compilation/test15191.d(47):        return type `int*` has pointers that may be `scope`
fail_compilation/test15191.d(68): Error: cannot slice static array of `ref return` of `identityArr()` in `@safe` function `sliceOfRefEscape`
fail_compilation/test15191.d(68):        return type `int*[1]` has pointers that may be `scope`
----
actual:
----
fail_compilation/test15191.d(35): Error: returning `&identity(x)` escapes a reference to local variable `x`
fail_compilation/test15191.d(41): Error: returning `&identityPtr(x)` escapes a reference to local variable `x`
fail_compilation/test15191.d(47): Error: cannot take address of `ref return` of `identityPtr()` in `@safe` function `addrOfRefTransitive`
fail_compilation/test15191.d(47):        return type `int*` has pointers that may be `scope`
fail_compilation/test15191.d(61): Error: returning `x` escapes a reference to parameter `x`
fail_compilation/test15191.d(61):        perhaps remove `scope` parameter annotation so `return` applies to `ref`
fail_compilation/test15191.d(68): Error: cannot slice static array of `ref return` of `identityArr()` in `@safe` function `sliceOfRefEscape`
fail_compilation/test15191.d(68):        return type `int*[1]` has pointers that may be `scope`
----
diff:
----
 fail_compilation/test15191.d(41): Error: returning `&identityPtr(x)` escapes a reference to local variable `x`
 fail_compilation/test15191.d(47): Error: cannot take address of `ref return` of `identityPtr()` in `@safe` function `addrOfRefTransitive`
 fail_compilation/test15191.d(47):        return type `int*` has pointers that may be `scope`
+fail_compilation/test15191.d(61): Error: returning `x` escapes a reference to parameter `x`
+fail_compilation/test15191.d(61):        perhaps remove `scope` parameter annotation so `return` applies to `ref`
 fail_compilation/test15191.d(68): Error: cannot slice static array of `ref return` of `identityArr()` in `@safe` function `sliceOfRefEscape`
 fail_compilation/test15191.d(68):        return type `int*[1]` has pointers that may be `scope`
----

[dkorpel]

Why was my review dismissed?

[ljmf00]

Why was my review dismissed?

I think we should revert this.