Merge stable into master

[mihails-strasuns]

1. reverts fix Issue 16589 - Taking address of stack variables in @safe code is … #6172 because after moving it from flag hidden error to deprecation I have noticed some false positives in the test suite. FYI @WalterBright - this may need more work when re-implemented in master
2. provides merge commit for stable -> master which removes all traces of -transition=safe flag

After this PR is merges, following bugzilla issues need to be reopened:

• https://issues.dlang.org/show_bug.cgi?id=16589
• https://issues.dlang.org/show_bug.cgi?id=16193

@MartinNowak would be nice to merge this soon-ish to minimize propagation of -transition=safe related conflicts.

[dlang-bot]

Fix	Bugzilla	Description
✓	13927	optimizer hangs in optelem with SIMD initialization
✓	16102	[REG2.070] struct dtor replace value on stack
✓	16193	opApply() doesn't heap allocate closure
✓	16574	[REG 2.072.0-b1] Unexplained errors about functions that overridde anything
✓	16678	[REG] Fix for issue 16193 creates major breakage
✓	16699	[REG 2.070] stack corruption with scope(exit)

[MartinNowak]

I don't quite understand the thing about the reversion? Is it just a commit in your merge-stable?
Merging back stable should do nothing but merging and resolving merge conflicts.
If you want to revert sth. on master, I'd expect it to be a normal PR, if there are problems w/ a feature on stable, it should be reverted there. Reverting a whole feature during the merge b/c it now causes failures on master seems wrong.
I'd usually disable the test (and open a bug ticket) or fix the feature, sort of an extended/semantic merge conflict resolution.

[mihails-strasuns]

I don't quite understand the thing about the reversion?

It was a pre-requisite for the merge. Because -transition=safe gets removed, I had to turn the fix there intro deprecation - which has uncovered it has false positives.

Is it just a commit in your merge-stable?

Yep, before actual merge commit.

If you want to revert sth. on master, I'd expect it to be a normal PR, if there are problems w/ a feature on stable, it should be reverted there.

I thought reasoning will be more obvious if related changes are bound together, can split into two PRs sure.

[mihails-strasuns]

Have split revert bit into #6290

[WalterBright]

Doesn't reverting this leave things in quite a mess? I no longer have idea what the state of the source code is in, between this, master, and the scope feature branch.

I had put these things behind -transition=safe in order to not break things while we work out all the details. Changing it to deprecated naturally broke things - is the solution really to just remove it all? Maybe the solution is to leave it as -transition=safe?

Renaming global.params.safe to global.params.vsafe has also fouled things up, as all those branches have now diverged.

I have noticed some false positives in the test suite.

Please be specific.

[mihails-strasuns]

Doesn't reverting this leave things in quite a mess? I no longer have idea what the state of the source code is in, between this, master, and the scope feature branch.

Things are in mess right now, this is part of attempt to get it all back in sync. Goal is to remove -transition=safe flag completely from all those places. Current state is:

• stable branch has no presence of -transition=safe at all, affected fixes have been turned into deprecations or reverted
• master has exactly one more usage of -transition=safe, Revert "Merge pull request #6172 from WalterBright/fix16589" #6290 removes it so that we can merge stable and remove -transition=safe from master too
• scope is not affected for now - we will need to agree on migration path between everyone concerned first

I had put these things behind -transition=safe in order to not break things while we work out all the details

That was both misuse of -transition and sub-optimal approach to working things out, thus all the changes. There is feature branch now which serves the same purpose.

Changing it to deprecated naturally broke things - is the solution really to just remove it all?

There is nothing natural about deprecations breaking anything - such cases must never happen, otherwise deprecated is completely compromised. The fact that it broke stuff means the fix was not polished enough to be merged into master - it should have either gone to scope branch or be done as non-breaking deprecation instead (like I did with some other scope fixes in stable).

Renaming global.params.safe to global.params.vsafe has also fouled things up, as all those branches have now diverged.

Nope, vsafe existed only for a very short period of time in stable and was removed as soon as only change it was used for got reverted. Right now there is neither safe or vsafe in stable.

Please be specific.

I see you have already commented on it in linked PR.

[WalterBright]

Nope, vsafe existed only for a very short period of time in stable and was removed as soon as only change it was used for got reverted. Right now there is neither safe or vsafe in stable.

vsafe is there in master:
https://github.com/dlang/dmd/blob/master/src/globals.d#L115

But is safe in the feature branch:
https://github.com/dlang/dmd/blob/scope/src/globals.d#L115

[mihails-strasuns]

vsafe is there in master:
https://github.com/dlang/dmd/blob/master/src/globals.d#L115

Sorry, I stand corrected. Must have missed one of previous merges of stable that did it (it isn't widely communicated when it happens). It doesn't change any of the other points though,

[quickfur]

We really ought to have people target bugfix PRs to both master and stable (i.e., for the same bugfix submit 2 PRs, one for stable, one for master). This should be a requirement for merging. Otherwise once the branches diverge it's a lot of churn trying to resync the code and not break anything else.

[schveiguy]

@quickfur That's a recipe for disaster. If you fix the same thing in both branches, everything will be merge conflicts.

[WalterBright]

This PR is problematic because it lumps merging fixes in with merging reversions in an all-or-nothing manner.

[WalterBright]

After this PR is merges, following bugzilla issues need to be reopened:
https://issues.dlang.org/show_bug.cgi?id=16193

The reversion for that is #6251, and I only agreed to it for stable, not for master.

After this PR is merges, following bugzilla issues need to be reopened:
https://issues.dlang.org/show_bug.cgi?id=16589

I'm not seeing a rationale for this other than "reason for reverting
is that trying to remove flag protection has shown it results in some false
triggers in dmd test suite" which gives essentially no information. There's no clarification in https://issues.dlang.org/show_bug.cgi?id=16589 nor #6172 that it is reverting. Note that it did pass all tests when merged, so I have no idea what "some false triggers" are.

[schveiguy]

The reversion for that is #6251, and I only agreed to it for stable, not for master.

This is problematic. Stable should only get fixes that should be pushed upstream. I don't like the idea that stable cannot be merged at-will into master. This will poison all future merges because you'd have to cherry pick this one thing out.

Perhaps we need a version branch to which such temporary reversions could be merged?

ping @MartinNowak, we need some guidance on this.

For the meantime, we likely have to merge the reversion to master, then revert the reversion!

[MartinNowak]

You guys are starring at the wrong PR. The reversion has already been split off to a separate PR #6290, as requested #6287 (comment), and I can hardly provide even more information than there #6290 (comment) and in the bug report Issue 16949 – [Reg 2.073] confusing @safe error message for fields with unsafe destructors.

[schveiguy]

@MartinNowak I'm concerned about a possibility of merging a PR into stable that should not be merged into Master (e.g. #6251). How does that affect future merges? How would one know not to remove it from a merge? Or am I not understanding something?

[MartinNowak]

@MartinNowak I'm concerned about a possibility of merging a PR into stable that should not be merged into Master (e.g. #6251). How does that affect future merges? How would one know not to remove it from a merge? Or am I not understanding something?

That's not planned at all! It would be nightmare to keep an overview.
There is a lot of confusion around this particular PR.

[MartinNowak]

Let's just merge back stable and do the rest of the adjustments on master, no need to conflate so many discussions into a single merge commit.
#6335