Fix: Prevent ICE on final switch forward referencing its enum #21001

abulgit · 2025-03-15T18:58:59Z

Closes: #20867
Fix Details-

First I Added a check to detect when a final switch statement is used within an enum's own definition
Another secondary check to detect incomplete enum member values during the missing members check

It now properly detects and reports the circular reference with the error message: "cannot use final switch on enum %s while it is being defined".

Testing-
I added a testcase to verify the fix: fail_compilation/fix20867.d

This test includes:
Test Case 1: The exact scenario from the GitHub issue
Test Case 2: A variation with multiple enum members
Test Case 3: A regular use of final switch (should compile normally)
Test Case 4: A nested circular reference case

All test cases Work Fine.

dlang-bot · 2025-03-15T18:59:03Z

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

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

dkorpel · 2025-03-15T19:32:13Z

Props for tackling this bug, forward references can be tricky. 👍

Your code for inferring whether an enum is incomplete looks functional, but there's a simpler way to check the semantic state of symbols that the rest of the compiler uses: the semanticRun variable in Dsymbol stores the last completed pass, so this should work better:

if (ed.semanticRun < PASS.semanticdone)
    error(...);

For the tests, please minimize the amount of added test code, since the test suite is already so large.

The presence of multiple enum members doesn't change any code paths
Regular final switch is already tested somewhere else
The nested case makes sense for testing your code that walks up the scopes, but it will be redundant with the simpler check.

So just keep test 1.

abulgit · 2025-03-15T20:01:59Z

Thank you for the review and suggestions! I've updated the PR

I simplified the enum incompleteness check using semanticRun < PASS.semanticdone as suggested.
And reduced the test file to only include the original case from the issue. and that makes sense since the test suite is already so big, and keeps getting bigger.

dkorpel · 2025-03-15T20:09:26Z

compiler/src/dmd/statementsem.d

+                for (Scope* scx = sc; scx; scx = scx.enclosing)
+                {
+                    if (scx.scopesym && scx.scopesym.isEnumDeclaration() && scx.scopesym == ed)
+                    {
+                        error(ss.loc, "cannot use `final switch` on enum `%s` while it is being defined", ed.toChars());
+                        sc.pop();
+                        return setError();
+                    }
+                }


This check still needs to be removed

dkorpel · 2025-03-15T20:09:54Z

compiler/src/dmd/statementsem.d

@@ -1959,8 +1959,33 @@ Statement statementSemanticVisit(Statement s, Scope* sc)
                ed = ds.isEnumDeclaration(); // typedef'ed enum
            if (!ed && te && ((ds = te.toDsymbol(sc)) !is null))
                ed = ds.isEnumDeclaration();
+
+            // Circular references
+            if (ed)


Suggested change

if (ed)

if (ed && ed.semanticRun < PASS.semanticdone)

dkorpel · 2025-03-15T20:10:26Z

compiler/src/dmd/statementsem.d

+                if (ed.semanticRun < PASS.semanticdone)
+                {
+                    error(ss.loc, "cannot use `final switch` on enum `%s` while it is being defined", ed.toChars());
+                    sc.pop();


Why did you add this line?

I kept both checks thinking they might catch different cases.

Nevermind this comment, I was asking about sc.pop() specifically, but I see now that a scope is pushed before and every return path needs to pop it.

dkorpel · 2025-03-15T20:15:34Z

compiler/src/dmd/statementsem.d

            if (ed && ss.cases.length < ed.members.length)
            {
+                // Remove the old check for incomplete enum members


This looks like an AI instruction accidentally left in here

no! I remove the previous enum members checks that's why I added that line.

But the comment doesn't make sense without the context of this Pull Request, and only adds confusion when someone reads it later.

if (!ed && te && ((ds = te.toDsymbol(sc)) !is null)) ed = ds.isEnumDeclaration(); if (ed && ed.semanticRun < PASS.semanticdone) { error(ss.loc, "cannot use `final switch` on enum `%s` while it is being defined", ed.toChars()); sc.pop(); return setError(); } if (ed && ss.cases.length < ed.members.length) {

did u mean this, remove all the other checks only ed && ed.semanticRun < PASS.semanticdone

But the comment doesn't make sense without the context of this Pull Request, and only adds confusion when someone reads it later.

I wrote it for my own, mistakenlyforgot to remove it.

Okay, I understand your point that the in-progress enum declarations should be done with the semanticRun state. So my question is -
You mentioned the semanticRun check should be sufficient. but how the enum's semantic state works.
Also, could you suggest specific debugging or maybe some instrumentation I could add to better understand this problem
Because I wanna understand this whole thing.

You could print the value of ed.semanticRun at the time of the crash, and then locate where it was last set with a debugger / debug print statements.

I tried adding debug points and print statements to trace how semanticRun interacts with enum member initializers, but things are getting so overwhelming for me.

If you or someone else solve this, I’d love to see the final implementation to better understand how this compiler tracks enum in these edge cases. A concrete example would help me learn how to handle similar issues in the future.
Thanks for your guidance!

I found that at the time of the crash, em.semanticRun == PASS.semanticdone which got set here:

dmd/compiler/src/dmd/enumsem.d

Line 195 in a7e2b2e

ed.semanticRun = PASS.semanticdone;

I tried moving that to after members have run semantic, but that breaks things, so I think we need to use semantic2 here. For structs, semantic2 does analysis of members, and for variables, semantic2 does analysis of initializers. I think we can extend this distinction to enums: semantic(1) can mean 'enum base type is determined', while semantic2 can mean 'all member values have been computed'. So after moving the enum member analysis to semantic2, the final switch check can verify we're at PASS.semantic2done and that should work.

If you want to give that a try, you can do so, but I can also take care of it. In that case, just close this PR and I'll open my own when I have time.

Yeah, I will give it a try.

abulgit · 2025-03-18T20:31:04Z

I don't understand why this failed
It looks like solving an issue gives a new one.

src/core/time.d(360): Error: cannot use `final switch` on enum `ClockType` while it is being defined
            with(ClockType) final switch (clockType)
                            ^
src/core/time.d(2055): Error: template instance `core.time.MonoTimeImpl!(ClockType.normal)` error instantiating
alias MonoTime = MonoTimeImpl!(ClockType.normal);
                 ^
src/core/time.d(360): Error: cannot use `final switch` on enum `ClockType` while it is being defined
            with(ClockType) final switch (clockType)
                            ^
src/core/time.d(2055): Error: template instance `core.time.MonoTimeImpl!(ClockType.normal)` error instantiating
alias MonoTime = MonoTimeImpl!(ClockType.normal);
                 ^
make: *** [Makefile:420: ../generated/linux/release/64/libdruntime.a] Error 1
make: *** Waiting for unfinished jobs....
make: *** [Makefile:414: ../generated/linux/release/64/libdruntime.so.a] Error 1
make: Leaving directory '/__w/dmd/dmd/druntime'
Error: Process completed with exit code 2.

dkorpel · 2025-03-18T20:46:36Z

Yes, that's why solving semantic analysis bugs is so hard! In this case, I think semantic2 doesn't get run. It's best to take small steps, so I'd start by simply setting the semantic2done flag at the end of the current enumSemantic function and see if that breaks anything.

I'd only really extract / move the code as a follow-up refactoring PR.

abulgit · 2025-03-19T20:21:03Z

Yes, that's why solving semantic analysis bugs is so hard! In this case, I think semantic2 doesn't get run. It's best to take small steps, so I'd start by simply setting the semantic2done flag at the end of the current enumSemantic function and see if that breaks anything.

I'd only really extract / move the code as a follow-up refactoring PR.

Yes, I Debugged the code and Tried so many different approaches, It took so many hrs and finally, this worked.
can u please review this?

Fix: Prevent ICE on final switch forward referencing its enum

768d06c

more simpler approach

93323f0

dkorpel reviewed Mar 15, 2025

View reviewed changes

abulgit requested a review from dkorpel March 16, 2025 18:10

moved the enum member analysis

e92dcf0

abulgit added 2 commits March 20, 2025 01:01

Move enum number analysis to sementic2

43dd5b4

WhiteSpace Remove

e451599

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Prevent ICE on final switch forward referencing its enum #21001

Fix: Prevent ICE on final switch forward referencing its enum #21001

abulgit commented Mar 15, 2025

dlang-bot commented Mar 15, 2025

dkorpel commented Mar 15, 2025

abulgit commented Mar 15, 2025

dkorpel Mar 15, 2025

dkorpel Mar 15, 2025

dkorpel Mar 15, 2025

abulgit Mar 15, 2025

dkorpel Mar 15, 2025

dkorpel Mar 15, 2025

abulgit Mar 15, 2025

dkorpel Mar 15, 2025

abulgit Mar 15, 2025

abulgit Mar 15, 2025

abulgit Mar 16, 2025

dkorpel Mar 16, 2025

abulgit Mar 16, 2025

dkorpel Mar 18, 2025

abulgit Mar 18, 2025

abulgit commented Mar 18, 2025 •

edited

Loading

dkorpel commented Mar 18, 2025

abulgit commented Mar 19, 2025

Fix: Prevent ICE on final switch forward referencing its enum #21001

Are you sure you want to change the base?

Fix: Prevent ICE on final switch forward referencing its enum #21001

Conversation

abulgit commented Mar 15, 2025

dlang-bot commented Mar 15, 2025

Bugzilla references

Testing this PR locally

dkorpel commented Mar 15, 2025

abulgit commented Mar 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abulgit commented Mar 18, 2025 • edited Loading

dkorpel commented Mar 18, 2025

abulgit commented Mar 19, 2025

abulgit commented Mar 18, 2025 •

edited

Loading