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

IgorStepanov · 2018-06-25T23:02:33Z

Reboot of #8319
@JinShil @jacob-carlborg Let finish it.

dlang-bot · 2018-06-25T23:02:37Z

Thanks for your pull request and interest in making D better, @IgorStepanov! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
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
✓	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#8404"

wilzbach

Wow! I hope that the third attempt works 👍

wilzbach · 2018-06-25T23:08:09Z

src/dmd/denum.d

    Type origType;

    EnumDeclaration ed;
+    bool isdeprecated;


FYI: We now have an official D style which we try to follow for new variables.
So this would be isDeprecated

Base class Dsymbol already has a virtual method isDeprecated(). And some classes e.g. AggregateDeclaration has a field isdeprecated, which is returned by overridden method isDeprecated().
Maybe stay it as is?

Hmm, I guess _deprecated or _isDeprecated or isDeprecatedEnum would be better than having a symbol which only differences in the spelling?
BTW isdeprecated is only set once and never looked at, which looks a bit suspicious.

BTW isdeprecated is only set once and never looked at, which looks a bit suspicious.

Yeah, may be a leftover of some debugging I was doing. @IgorStepanov, please investigate if it's actually needed.

Ok, I've just removed it:)

JinShil · 2018-06-26T00:22:43Z

src/dmd/expression.d

 */
 extern (C++) final class VarExp : SymbolExp
 {
+    bool hasCheckedAttrs;


It's a little naughty, but I don't know of any other solution, and it'd be a shame to hold up this feature on such a minor problem, so OK, let's do it. I do, however, suggest adding a comment explaining why this exists.

JinShil · 2018-06-26T00:25:25Z

We probably need to update the language spec also. I'll try to get to it later, but I won't mind if someone beats me to it.

IgorStepanov · 2018-06-26T01:16:03Z

BTW, What purpose of astbase module and classes. Looks like astbase.Dsymbol.deprecatedDeclaration hasn't used anywhere. Maybe we should skip dd parameter of constructor?

JinShil · 2018-06-26T01:44:39Z

BTW, What purpose of astbase module and classes. Looks like astbase.Dsymbol.deprecatedDeclaration hasn't used anywhere. Maybe we should skip dd parameter of constructor?

Keep in mind that I had been playing with that code for a very long time, trying many different things to work through the problem. If you find that the DeprecatedDeclaration is not needed, then just remove it.

wilzbach · 2018-06-26T01:48:45Z

BTW, What purpose of astbase module and classes.

AstBase is used nowhere in DMD. It was an attempt by @RazvanN7 to separate the AST nodes and create a pure "data" layer.
However, the switch to AstBase has never been made though I don't recall the exact reasons (I think it had sth. to do with the proposal needing too many templates).
See also:

IgorStepanov · 2018-06-26T02:17:51Z

Keep in mind that I had been playing with that code for a very long time, trying many different things to work through the problem. If you find that the DeprecatedDeclaration is not needed, then just remove it.

astbase.EnumMember.__ctor should have the same signature with denum.EnumMember.__ctor because they both are used in Parser(AST).
I may just ignore dd parameter in astbase.EnumMember.__ctor, but in can be needed in astbase.EnumMembe in future.

JinShil · 2018-06-26T02:23:48Z

but in can be needed in astbase.EnumMembe in future.

I suggest adhering to the YAGNI principle. In other words, if it's not used, remove it from both constructors.

IgorStepanov · 2018-06-26T02:31:49Z

@JinShil

remove it from both constructors.

It needed in denum.EnumMember.__ctor and isn't needed in astbase.EnumMember.__ctor.
Thus it should be in constructor but may be ignored in astbase.EnumMember.

IgorStepanov · 2018-06-26T02:35:15Z

may be ignored in astbase.EnumMember

Done

JinShil · 2018-06-26T04:15:55Z

src/dmd/expression.d

 {
+    // Semantic can be called many times for a one expression.
+    // This field need to ensure that deprecation message will be printed only one time.
+    bool hasCheckedAttrs;


Changes for proper English grammar:

// Semantic can be called multiple times for a single expression. // This field is needed to ensure the deprecation message will be printed only once.

JinShil · 2018-06-26T04:17:18Z

It looks good to me. I would like to hear from @RazvanN7 about the discrepancy with ASTBase.

JinShil · 2018-06-26T12:25:33Z

Spec PR: dlang/dlang.org#2396

jacob-carlborg · 2018-06-26T13:18:10Z

src/dmd/expression.d

 */
 extern (C++) final class VarExp : SymbolExp
 {
+    // Semantic can be called many times for a one expression.


Please make this a Ddoc comment. All public or protected symbols should have a documentation, i.e. a Ddoc comment.

jacob-carlborg · 2018-06-26T13:23:17Z

changelog/enum_attributes.dd

@@ -0,0 +1,24 @@
+D now supports attributes on enum members
+


Perhaps explicitly list the supported attributes since it's only three (counting UDAs as one) and to avoid any confusion.

jacob-carlborg

Looks good, just a few minor comments.

IgorStepanov · 2018-06-26T22:37:52Z

@JinShil @jacob-carlborg All comments resolved. Any other objections?

jacob-carlborg · 2018-06-27T07:05:40Z

It looks good to me. I would like to hear from @RazvanN7 about the discrepancy with ASTBase.

As far as I understand, ASTBase was introduced to allow using the parser without the rest of the frontend. Using the rest of the frontend as a library caused some problems. Those are fixed and the full frontend is available as a library. I don't think there's a need for ASTBase.

jacob-carlborg

I'll approve this and if there's a need to update ASTBase we can do that in a followup PR.

IgorStepanov · 2018-06-27T11:05:35Z

Thanks!

IgorStepanov requested review from RazvanN7, dnadlinger and ibuclaw as code owners June 25, 2018 23:02

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

wilzbach reviewed Jun 25, 2018

View reviewed changes

IgorStepanov force-pushed the uda-enum branch from 5ec233c to f2bfb6b Compare June 25, 2018 23:29

JinShil reviewed Jun 26, 2018

View reviewed changes

IgorStepanov force-pushed the uda-enum branch 2 times, most recently from ee289ed to da07d98 Compare June 26, 2018 01:06

IgorStepanov force-pushed the uda-enum branch from da07d98 to f48bc12 Compare June 26, 2018 02:34

JinShil suggested changes Jun 26, 2018

View reviewed changes

JinShil mentioned this pull request Jun 26, 2018

Add enum member attributes to the spec dlang/dlang.org#2396

Merged

jacob-carlborg reviewed Jun 26, 2018

View reviewed changes

jacob-carlborg approved these changes Jun 26, 2018

View reviewed changes

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

36d349c

IgorStepanov force-pushed the uda-enum branch from f48bc12 to 36d349c Compare June 26, 2018 22:09

jacob-carlborg approved these changes Jun 27, 2018

View reviewed changes

jacob-carlborg added the Merge:auto-merge label Jun 27, 2018

dlang-bot merged commit 972ea45 into dlang:master Jun 27, 2018

JinShil mentioned this pull request Aug 31, 2018

Fix Hack: Remove hasCheckedAttrs from VarExp #8644

Merged

ibuclaw mentioned this pull request Jun 24, 2021

[refactor] dmd.traits: Remove dead isEnumMemX helper #12746

Merged

Uh oh!

Conversation

IgorStepanov commented Jun 25, 2018

Uh oh!

dlang-bot commented Jun 25, 2018

Bugzilla references

Testing this PR locally

Uh oh!

wilzbach left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Jun 25, 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 26, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Jun 26, 2018

Uh oh!

IgorStepanov commented Jun 26, 2018

Uh oh!

JinShil commented Jun 26, 2018

Uh oh!

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

Uh oh!

Uh oh!

IgorStepanov commented Jun 26, 2018

Uh oh!

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

Uh oh!

Uh oh!

IgorStepanov commented Jun 26, 2018

Uh oh!

IgorStepanov commented Jun 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Jun 26, 2018

Uh oh!

JinShil commented Jun 26, 2018

Uh oh!

jacob-carlborg Jun 26, 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!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg Jun 26, 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!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg left a comment

Choose a reason for hiding this comment

Uh oh!

IgorStepanov commented Jun 26, 2018

Uh oh!

jacob-carlborg commented Jun 27, 2018

Uh oh!

jacob-carlborg left a comment

Choose a reason for hiding this comment

Uh oh!

IgorStepanov commented Jun 27, 2018

Uh oh!

wilzbach left a comment •

edited

Loading

wilzbach Jun 25, 2018 •

edited

Loading

JinShil Jun 26, 2018 •

edited

Loading

wilzbach commented Jun 26, 2018 •

edited

Loading

JinShil commented Jun 26, 2018 •

edited

Loading

jacob-carlborg Jun 26, 2018 •

edited

Loading

jacob-carlborg Jun 26, 2018 •

edited

Loading