Added location to user defined attributes by AsterMiha · Pull Request #12524 · dlang/dmd

AsterMiha · 2021-05-16T18:07:26Z

Created a new node (UadItem) to preserve the location of all user defined attributes

dlang-bot · 2021-05-16T18:07:27Z

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

ibuclaw · 2021-05-16T18:18:52Z

src/dmd/parse.d

-            udas.push(exp);
+            udas.push(new AST.UadItem(udastart, exp, parens));


And why can't exp have its loc set to udastart?

Because udas can be written in any of these ways: @ e, @(e), @(e1, e2)
In the first 2 cases changing the loc of the exp would work but in the last it would be confusing to set all of them to the loc of '@', especially when the expressions are on different lines.

I also think that introducing a new AST type to represent this is overkill, and will not scale well.

ghost

That's a lot of changes to have the position so I wont give my mind on this aspect.
The changes requested are more about

The coverage that is not really good.
The style a bit inconsistent with the use of isUadItem() and most importantly the name of the new node is not good. the offical name for those attribs are UDA, so UDAItem is better.
ASTBase declaration

src/dmd/expression.d

src/dmd/hdrgen.d

src/dmd/mtype.d

src/dmd/semantic2.d

ghost · 2021-05-16T20:56:01Z

src/dmd/semantic2.d

                            TupleExp te = cast(TupleExp)e;
                            eval(sc, te.exps, lastTag);
                        }
+                        if (e.op == TOK.at)


if (UadItem te = e.isUadItem()){...}

However now I see that the way you write the cast is consistent with the other cast in the block.

jacob-carlborg · 2021-05-20T20:07:19Z

I will not do an extended review of this, but the tests are a bit lacking. They need to include:

UDAs on multiple lines, i.e:
```
@(3)
@(4)
int a;
```
Both of the other two UDA syntaxes (if those are indented to be supported by this PR), i.e.:
```
@(3) {
    int a;
}
```
```
@(3):
    int a;
```
A combination of all of the above, i.e.:
```
@(4):
@(3) {
    @(6)
    @(5) int a;
}
```

AsterMiha · 2021-05-23T18:51:48Z

@tungstenheart Renamed the node and added the ASTBase declaration. I forgot to remove some of the code from previous attempts and that caused the coverage issues, but I fixed that now. Also added a couple more tests.

ghost · 2021-05-24T20:45:19Z

thanks, (someone can cancel my request for changes)

RazvanN7 · 2021-05-25T07:53:57Z

src/dmd/dtemplate.d

 {
    // None of these can be value parameters
-    if (e.op == TOK.tuple || e.op == TOK.scope_ ||
+    if (e.op == TOK.tuple || e.op == TOK.at || e.op == TOK.scope_ ||


I am confused by this addition. What do UDAs have to do with value parameters? Do you have a code snippet that is directly affected by this?

Semantic2 attempts to interpret all expressions in UserAttributeDeclaration [1]. Now that the actual expressions are inside a UDAItem node we need to reach its contents first. An alternative would be to add UDAItem to the Interpretor visitor to unpack the node there.

[1]

dmd/src/dmd/semantic2.d

Lines 573 to 574 in 2453350

if (definitelyValueParameter(e))

e = e.ctfeInterpret();

RazvanN7

Looks good to me. I'll wait for @jacob-carlborg 's review

jacob-carlborg · 2021-05-26T15:01:11Z

I'll wait for jacob-carlborg 's review

I've already done the review I'll be doing.

ghost · 2021-05-27T10:00:37Z

SInce this was not discussed (here at least), what is the value added , what are the use cases ?

RazvanN7 · 2021-05-27T10:36:34Z

Currently, UDAs do not contain the location, as a result third-party tools using dmd-as-a-lib that want to properly expose the location of UDAs cannot do that. This PR fixes that. More info: #11788 (comment)

ghost

see comments. Mostly the assert that will avoid to merge possibly dead code.

src/dmd/expressionsem.d

Geod24 · 2021-05-30T09:14:59Z

src/dmd/astbase.d

        }
    }

+    extern (C++) class UDAItem : Expression


No new class should be introduced without a proper documentation. Someone should be able to look at this code and know why this class exists and when it should be used / where one can expect to find it in the tree.

Geod24 · 2021-05-30T11:31:28Z

src/dmd/parse.d

-            udas.push(exp);
+            udas.push(new AST.UadItem(udastart, exp, parens));


I also think that introducing a new AST type to represent this is overkill, and will not scale well.

AsterMiha requested review from Geod24, RazvanN7, UplinkCoder, WalterBright, ibuclaw and kinke as code owners May 16, 2021 18:07

ibuclaw reviewed May 16, 2021

View reviewed changes

ghost suggested changes May 16, 2021

View reviewed changes

AsterMiha force-pushed the uadItemExp branch 8 times, most recently from ef903b2 to 872e34c Compare May 23, 2021 17:50

AsterMiha force-pushed the uadItemExp branch from 872e34c to 3ddfed5 Compare May 24, 2021 19:35

RazvanN7 reviewed May 25, 2021

View reviewed changes

RazvanN7 approved these changes May 26, 2021

View reviewed changes

RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label May 26, 2021

ghost suggested changes May 27, 2021

View reviewed changes

src/dmd/expressionsem.d Outdated Show resolved Hide resolved

Added location to user defined attributes

0f3a2d8

AsterMiha force-pushed the uadItemExp branch from 3ddfed5 to 0f3a2d8 Compare May 27, 2021 17:57

ghost approved these changes May 28, 2021

View reviewed changes

Geod24 reviewed May 30, 2021

View reviewed changes

ibuclaw added Review:Walter Bright and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels May 30, 2021

dlang-bot added Review:Needs Rebase Review:Needs Work labels Jun 11, 2021

dlang-bot added the Review:stalled label Aug 28, 2021

RazvanN7 mentioned this pull request Dec 16, 2021

Add location to user defined attributes #9359

Closed

RazvanN7 closed this Feb 1, 2022

		udas.push(exp);
		udas.push(new AST.UadItem(udastart, exp, parens));

Uh oh!

Conversation

AsterMiha commented May 16, 2021

Uh oh!

dlang-bot commented May 16, 2021

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AsterMiha May 16, 2021 • 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!

ghost left a comment • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AsterMiha commented May 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 left a comment

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented May 26, 2021

Uh oh!

ghost commented May 27, 2021

Uh oh!

RazvanN7 commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AsterMiha May 16, 2021 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

jacob-carlborg commented May 20, 2021 •

edited

Loading

AsterMiha commented May 23, 2021 •

edited

Loading

RazvanN7 commented May 27, 2021 •

edited

Loading