Fix Issue #22567 - alias this and nested AAs cause compiler SIGILL

[divyansharma001]

Problem

When an alias this redirects a nested AA write through a struct field,
after alias this resolution the LHS foo["bar"]["baz"] becomes
foo["bar"].data["baz"]. The inner foo["bar"] is an IndexExp with
modifiable=true that was never lowered to _d_aaGetRValueX — the
modifiable flag suppresses that lowering in visit(IndexExp). The IR
generator then hits the unlowered AA index expression and fires
assert(false) → SIGILL.

Fix

Add revertModifiableAAIndexReads(), called after the multi-dim loop in
rewriteAAIndexAssign(). It recurses through DotVarExp wrappers alias
this produces and lowers any modifiable=true AA IndexExp to a proper
rvalue read via lowerAAIndexRead() before side-effect extraction.

The true multi-dimensional case (direct nested AAs, no alias this) is
unaffected: after the loop eaa is the base variable, on which the
function is a no-op.

Regression

Introduced in 2.112, works in 2.111.

Fixes #22567

[dlang-bot]

Thanks for your pull request and interest in making D better, @divyansharma001! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

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 "stable + dmd#22622"

[Copilot]

Pull request overview

This PR fixes issue #22567, a regression introduced in DMD 2.112 where the compiler crashes with SIGILL when using alias this with nested associative array (AA) assignments. The issue occurs because after alias this resolution, inner AA index expressions retain their modifiable=true flag and are never lowered to proper rvalue reads (_d_aaGetRValueX), causing the IR generator to hit an assertion failure.

Changes:

• Added revertModifiableAAIndexReads() function to recursively lower modifiable AA IndexExp nodes through DotVarExp wrappers produced by alias this
• Integrated the fix into rewriteAAIndexAssign() to handle the alias this case before side-effect extraction
• Added comprehensive test case to prevent regression

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
compiler/src/dmd/expressionsem.d	Implements the fix by adding revertModifiableAAIndexReads() function and calling it in rewriteAAIndexAssign() to properly lower modifiable AA index expressions wrapped by alias this
compiler/test/runnable/testaa3.d	Adds test case test22567() that reproduces the original bug and validates the fix works correctly

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[rainers]

Good analysis and fix. I was trying to find the place where the alias this replacement actually happens, but failed to spot it yet. Would it work to just reset IndexExp.modifiable there?

[divyansharma001]

The alias this replacement for this case happens in opOverloadArray in opover.d, around where:

ae.e1 = resolveAliasThis(sc, ae1save, true);
At that point ae1save is the original IndexExp(foo, "bar") with modifiable=true, and ae.e1 becomes DotVarExp(IndexExp(foo, "bar"), data). So resetting ae1save.isIndexExp().modifiable = false there would let visit(IndexExp) naturally lower it to a rvalue read.

However, opOverloadArray is a general overload resolution path, it's reached for both reads and writes. Resetting modifiable there unconditionally could be wrong if the IndexExp genuinely needs to be mutable in some other context that goes through the same path.

The current fix resets it only inside rewriteAAIndexAssign, where we are definitively in an assignment context and have already established that the inner IndexExp is the base of a DotVarExp chain leading to the actual write target. This makes it more conservative.

That said, if you think the alias this site is the right place I'm happy to move it there, do you have a specific location in mind? I wasn't able to find a narrower alias this site that's only reached for lvalue/assignment contexts.

[rainers]

Thanks for the link to opOverloadArray. Just resetting modifiable doesn't work, but this seems to work, too:

--- a/compiler/src/dmd/opover.d
+++ b/compiler/src/dmd/opover.d
@@ -404,13 +404,19 @@ Expression opOverloadArray(ArrayExp ae, Scope* sc)
         // Didn't find it. Forward to aliasthis
         if (ad.aliasthis && !isRecursiveAliasThis(att, ae.e1.type))
         {
-            //printf("att arr e1 = %s\n", this.e1.type.toChars());
+            //printf("att arr e1 = %s\n", ae.e1.type.toChars());
             /* Rewrite op(a[arguments]) as:
              *      op(a.aliasthis[arguments])
              */
             ae.e1 = resolveAliasThis(sc, ae1save, true);
             if (ae.e1)
+            {
+                if (auto dve = ae.e1.isDotVarExp())
+                    if (auto ie1 = dve.e1.isIndexExp())
+                        if (ie1.modifiable && ie1.e1.type.isTypeAArray())
+                            dve.e1 = lowerAAIndexRead(ie1, sc);
                 continue;
+            }
         }
         break;
     }

Not sure which version is better, both add special cases into other logic.

[rainers]

but this seems to work, too:

Not quite good enough, it converts

    foo["bar"] = S();

into a read, too. Let's go with your version then.

[ibuclaw]

Can you add a comment to the effect of:

// Ditto, but traverses DotVarExp from `alias this` rewrites.

Is there no way that this can be merged with revertIndexAssignToRvalues above?

Should this be given a similar name revertXXXToRvalues ?

[thewilsonator]

Can you add a comment to the effect of:

Done. I will leave any potential merge of these functions for a future PR.