Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Issue 21861 - ctfe fails when a nested enum or function has a UDA #12472

Merged
merged 1 commit into from
Apr 27, 2021

Conversation

BorisCarvajal
Copy link
Member

If declaration is an UDA just eval the symbol it contains.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @BorisCarvajal!

Bugzilla references

Auto-close Bugzilla Severity Description
21861 normal ctfe fails when a nested enum or function has a UDA

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

Comment on lines 2230 to 2231
assert(uda.decl && uda.decl.dim == 1);
s = (*uda.decl)[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only one declaration?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, at function scope the parser only allows one declaration after UDAs.

Copy link
Contributor

@MoonlightSentinel MoonlightSentinel Apr 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.
But the issue should rather be fixed by changing the AttribDeclaration branch below (which rejects enums ATM and hence causes the bug). The current implementation doesn't care about the UDA and only considers the AttribDeclaration part.

Copy link
Contributor

@MoonlightSentinel MoonlightSentinel Apr 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible solution (unless im missing something):

diff --git a/src/dmd/dinterpret.d b/src/dmd/dinterpret.d
index 8676f3b59..3d383b33d 100644
--- a/src/dmd/dinterpret.d
+++ b/src/dmd/dinterpret.d
@@ -2313,12 +2313,8 @@ public:
             AttribDeclaration ad = e.declaration.isAttribDeclaration();
             if (ad && ad.decl && ad.decl.dim == 1)
             {
-                Dsymbol sparent = (*ad.decl)[0];
-                if (sparent.isAggregateDeclaration() || sparent.isTemplateDeclaration() || sparent.isAliasDeclaration())
-                {
-                    result = null;
-                    return; // static (template) struct declaration. Nothing to do.
-                }
+                result = null;
+                return; // static declaration. Nothing to do.
             }

             // These can be made to work, too lazy now

Copy link
Member Author

@BorisCarvajal BorisCarvajal Apr 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That branch just returns null, but if there is a variable declaration inside the UDA it needs to be evaluated.
(I modified the test to check for that)
I kept that code untouched, although I think it should be revisited some day.

Copy link
Contributor

@MoonlightSentinel MoonlightSentinel Apr 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, good catch. Probably still better to simply check for any AttribDeclaration instead of UDAs:

diff --git a/src/dmd/dinterpret.d b/src/dmd/dinterpret.d
index 8676f3b59..85c1858a2 100644
--- a/src/dmd/dinterpret.d
+++ b/src/dmd/dinterpret.d
@@ -2225,6 +2225,11 @@ public:
             printf("%s DeclarationExp::interpret() %s\n", e.loc.toChars(), e.toChars());
         }
         Dsymbol s = e.declaration;
+        if (auto ad = s.isAttribDeclaration())
+        {
+            assert(ad.decl && ad.decl.dim == 1);
+            s = (*ad.decl)[0];
+        }
         if (VarDeclaration v = s.isVarDeclaration())
         {
             if (TupleDeclaration td = v.toAlias().isTupleDeclaration())

Unless we know for sure that other AD's can never reach this code.

EDIT: Albeit that might require a loop instead of a plain if (if nesting can occur)

@BorisCarvajal BorisCarvajal force-pushed the fix21861 branch 2 times, most recently from 13f571c to 9479f80 Compare April 26, 2021 00:38
@thewilsonator thewilsonator merged commit 81bc991 into dlang:master Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants