Fix Issue 9701 - allow UDAs to be attached to enum values by JinShil · Pull Request #8319 · dlang/dmd

JinShil · 2018-06-01T08:16:22Z

This is a reboot of #6161

I've also added the ability to mark enum members as deprecated and @disabled.

It was decided a while ago that a proper implementation would be accepted without a DIP. Here's the forum announcement: https://forum.dlang.org/post/gcgtotrhuirhcvvbxrfm@forum.dlang.org

This only implements the feature for enum members. For function arguments, see #7576

dlang-bot · 2018-06-01T08:16:24Z

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	9701	normal	UDAs cannot be attached to enum values.

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 "master + dmd#8319"

jacob-carlborg · 2018-06-01T08:21:14Z

test/compilable/test9701.d

+{
+    value0,
+    @("uda1") value1,
+    @("uda2") @("uda3") value2,


Should be a test with the @uda syntax as well.

JinShil · 2018-06-01T08:22:36Z

src/dmd/expression.d

        super(loc, TOK.variable, __traits(classInstanceSize, VarExp), var, hasOverloads);
        //printf("VarExp(this = %p, '%s', loc = %s)\n", this, var.toChars(), loc.toChars());
        //if (strcmp(var.ident.toChars(), "func") == 0) assert(0);
-        this.type = var.type;


This was needed to prevent semantic from printing a deprecation multiple times. I used the common pattern to check for type to see if semantic has already been run. This also required some refactoring of the expression semantic visitor for VarExp.

Is this really the correct fix?

I'm not 100% sure, but if you search expressionsem.d for if (e.type) you will see it is a common idiom. The expression classes don't seem to have a flag indicating whether or not semantic has been run, so it looks like the type property is (ab)used for that.

It doesn't work at the moment anyway because there are other places in the code that don't call expressionSemantic on the VarExp class, so I need to submit a separate PR to remedy that. I worked on it all day today, and it's proving to be quite difficult.

If I don't do this, then a deprecation message is printed every type VarExp.expressionSemantic is run, and that's suboptimal. I need some way to know whether or it has already been run, and the dominating idiom for that seems to be if (e.type) in the semantic visitor functions.

JinShil · 2018-06-01T08:25:37Z

src/dmd/expressionsem.d

                VarDeclaration v = ve.var.isVarDeclaration();
                if (v && ve.checkPurity(sc, v))
                    return setError();
+                exp.e1 = ve.expressionSemantic(sc);


Because we removed setting type in VarExp's constructor. See https://github.com/dlang/dmd/pull/8319/files#r192327829

JinShil · 2018-06-02T10:40:04Z

This is currently held up on a problem with the VarExp semantic visitor. It seems to be needlessly called multiple times. So if I put a deprecation message there, it gets printed multiple times. I'm trying to resolve that in a separate PR, but I'm having a hell of a time getting it right.

jacob-carlborg · 2018-06-06T07:39:02Z

test/compilable/test9701.d

+    @uda5 @uda6 value4,
+    @("uda7") @uda8 value5,
+    @uda9 @("uda10") value6,
+    deprecated value7


Please add a test for deprecated with a message as well, i.e. deprecated("foo").

JinShil · 2018-06-07T11:17:07Z

I'm closing this. My frustration with D has hit a new high, and I need to walk away for a while.

IgorStepanov · 2018-06-25T00:52:54Z

@JinShil Hi. Why do you closed this PR? Are there any troubles in implementation?

IgorStepanov · 2018-06-25T00:59:30Z

You may call e.checkDeprecated(sc, e.var); only if e.type is null. (before the first semantic of VarExp)
Or you may return from visit if e.type is not null (if you are sure that there should be only one semantic for VarExp).

JinShil · 2018-06-25T01:01:09Z

Are there any troubles in implementation?

Hi @IgorStepanov, Yes, there is one fundamental issue that I can't find a solution for. The semantic pass for VarExp is called multiple times. Therefore, when I add the code to display an error message for the disable/deprecated attributes, the error message prints at least 3 times for each VarExp. I tried to solve that by checking if the type property is null to determine if semantic had already been run (that pattern exists elsewhere for other derived Expression types), but while it solved the multiple error message problem, it caused a cascade of other failures. I tried to work through it little-by-little solving each failure, but the problem was so systematic, it became hopeless.

I asked for help on the forum but never received a response.

Edit: If you think you can fix this, please adopt this PR and let's bring it to completion.

IgorStepanov · 2018-06-25T01:28:30Z

@JinShil we may add the field with flag only for attribute errors. Ensure that this field properly copies in copy syntaxCopy functions. This is an ugly solution, but it doesnt block a very useful feature.
And why do you check deprecation in VarExp semantic, not in EnumMember semantic?
I'll try to fix it in a week, I hope.

JinShil · 2018-06-25T01:35:35Z

And why do you check deprecation in VarExp semantic, not in EnumMember semantic?

Because we want the error message at the usage site, not the declaration site.

I'll try to fix it in a week, I hope.

👍

JinShil requested a review from RazvanN7 as a code owner June 1, 2018 08:16

dlang-bot added the Severity:Bug Fix label Jun 1, 2018

JinShil added the Review:WIP Work In Progress - not ready for review or pulling label Jun 1, 2018

JinShil mentioned this pull request Jun 1, 2018

fix issue 9701 - allow UDAs to be attached to enum values #6161

Closed

jacob-carlborg reviewed Jun 1, 2018

View reviewed changes

JinShil commented Jun 1, 2018

View reviewed changes

JinShil added the Review:Needs Work label Jun 2, 2018

dlang-bot removed the Review:Needs Work label Jun 6, 2018

jacob-carlborg reviewed Jun 6, 2018

View reviewed changes

Fix Issue 9701 - allow UDAs to be attached to enum values

5db047d

JinShil closed this Jun 7, 2018

IgorStepanov mentioned this pull request Jun 25, 2018

Fix Issue 9701 - allow UDAs to be attached to enum values #8404

Merged

JinShil deleted the fix_9701 branch August 27, 2018 01:00

Uh oh!

Conversation

JinShil commented Jun 1, 2018

Uh oh!

dlang-bot commented Jun 1, 2018

Bugzilla references

Testing this PR locally

Uh oh!

jacob-carlborg Jun 1, 2018

Choose a reason for hiding this comment

Uh oh!

JinShil Jun 6, 2018

Choose a reason for hiding this comment

Uh oh!

JinShil Jun 1, 2018

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg Jun 2, 2018

Choose a reason for hiding this comment

Uh oh!

JinShil Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil Jun 1, 2018

Choose a reason for hiding this comment

Uh oh!

JinShil commented Jun 2, 2018

Uh oh!

jacob-carlborg Jun 6, 2018

Choose a reason for hiding this comment

Uh oh!

JinShil commented Jun 7, 2018

Uh oh!

IgorStepanov commented Jun 25, 2018

Uh oh!

IgorStepanov commented Jun 25, 2018

Uh oh!

JinShil commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IgorStepanov commented Jun 25, 2018

Uh oh!

JinShil commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JinShil Jun 2, 2018 •

edited

Loading

JinShil commented Jun 25, 2018 •

edited

Loading

JinShil commented Jun 25, 2018 •

edited

Loading