fix issue 20011, 17828 - crash or accept write operation on members of manifest constant structs

[ghost]

This fixes a regression introduced by this fix as well as a related bug.

[dlang-bot]

Thanks for your pull request and interest in making D better, @Basile-z! 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
✓	17828	normal	[ICE] Internal error: ddmd/backend/cgcs.c 352 - CTFE appending to an array on a struct from a template
✓	20011	regression	[REG] modification of member of a manifest constant that's also a struct is allowed

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + dmd#10115"

[ghost]

Considering that next minor release is about to be published, is this still good for stable ?

[thewilsonator]

Yes, Please target stable.

[ghost]

🤞 no conflicts 🤞

[thewilsonator]

No conflicts but you've got appveyor there, how old is the parent commit for that branch?

[ghost]

It must be an Appveyor issue because my local stable branch is up to date (9ddedc4)

[ghost]

I will force push otherwise automerge won't happen.

[kinke]

Auto-merge 13 mins after opening the PR? - I think this only patches over existing cracks. Handling this in modifiableLvalue() or so (I'm not familiar with this code here) seems more appropriate IMO.

[ghost]

The Lvalue logic is either botched or misused. I had to do this that way because the combination of the two bugs were driving me crazy but honestly this is not to bad: The error check is called in every overridden visit for an assign expression so although the Lvalue logic is not used this patch is clean. It's a leaf, easy to remove, it doesn't impose himself in the middle of something more complex.

[kinke]

The Lvalue logic is either botched or misused.

Yes, something is off since #9505, and that should be analyzed and fixed properly IMO, incl. fixing this:

void main()
{
    const x = 0;
    const p = &x;
    (cast() x) = 5; // wrongly const-folded to 0 = 5
    assert(*p == 5); // fails, still 0; that's what #9505 was supposed to fix
}

I guess that as soon as the root problem is understood, we don't need the special cases handled here (we didn't before #9505, did we? - edit: ah, one issue was from 2017, my bad).

[ghost]

Yes this fix is a bit special. There was this 2017 issue I was trying to understand and when minimizing by hand I found the smaller test case that was accepted and then only I realized there was a REG too. Now with this patch all the cases are handled.

My opinion on the root of the issue is that dotted expressions on enum were never really checked, only simple values (using STC.manifest) are.

Anyway I don't think this gonna make it for 2.087. PRs on stable branch are blocked by the project tester.
So no hurry. I chalenge other contributors to find a better fix. But fix the two issues too.

[wilzbach]

Considering that next minor release is about to be published, is this still good for stable ?

Please be a bit careful when pushing to stable in those two days between the last release candidate and the actual release.

[thewilsonator]

PRs on stable branch are blocked by the project tester.

built kite fails for unrelated reasons and is not required.

[WalterBright]

see comment

[wilzbach]

built kite fails for unrelated reasons and is not required.

Come on. It is not unknown - it lacks a PR from druntime where the mangling was changed for Mecca. That one needs to be cherry-picked to stable.

[ghost]

@WalterBright, can you see if the changes requested are applied correctly ?

[ghost]

As seen in libasync through the project tester, fixed const ref function parameters which can accept members of structInit as the intention is not to modify.

[anass-O]

@kinke did you ever revert this? Its causing more bugs and it seems to have introduced (partially) rvalues Bindable to const ref.

Also this is the cause of this regression.

Issues.dlang.org/show_bug.cgi?id=20608

[kinke]

It would be reverted in #10124, but that's blocked because of the Ocean lib, which needs a new release (waiting for months now).