Make parser recognize AliasAssign by RazvanN7 · Pull Request #441 · dlang-community/libdparse

RazvanN7 · 2021-05-06T09:50:28Z

A few notes:

I have added a new class AliasAssign that is viewed as a declaration so that it could appear in the same context as declarations. This might be a bit hacky, but it seemed like the easiest way to implement this.
The AliasAssign class is just a wrapper for AliasInitializer. I considered using an AliasInitializer as that would make the diff smaller, however, that comes with 2 drawbacks: comments cannot be attached to AliasInitializers and it uses AliasInitializer for a purposes that it wasn't designed for. If the maintainers come to the conclusion that it is better to use AliasInitializer, I am open to amend the PR.
AliasAssignments should be present only in template declaration contexts, therefore I have added another default parameter to parseDeclaration, so that is signaled to the declaration.

This is currently blocking some PRs in phobos, so if you merge, please also release a new tag so that we can unblock those PRs.

Cheers,
RazvanN

RazvanN7 · 2021-05-06T09:58:43Z

Test suite is failing with "All tests have passed".

src/dparse/ast.d

MoonlightSentinel · 2021-05-06T10:09:19Z

src/dparse/parser.d

+    /**
+     * Parses an AliasAssign.
+     *
+     * $(GRAMMAR $(RULEDEF aliasDeclaration):


Suggested change

* $(GRAMMAR $(RULEDEF aliasDeclaration):

* $(GRAMMAR $(RULEDEF aliasAssign):

src/dparse/parser.d

WebFreak001

thanks for the PR! Got some points that should be changed

WebFreak001 · 2021-05-06T13:19:38Z

src/dparse/parser.d

+        mixin(traceEnterAndExit!(__FUNCTION__));
+        auto startIndex = index;
+        auto node = allocator.make!AliasAssign;
+        node.initializer = parseAliasInitializer();


AliasInitializer allows adding template parameters, are these really supported by alias assign? (as this is a public function that may be called freely - and for good code style)

Nope. I can implement it properly, but I thought I would just reuse as much code as possible to make the diff smaller.

src/dparse/ast.d

WebFreak001 · 2021-05-06T13:29:22Z

src/dparse/parser.d

     *     ;)
     */
-    Declaration parseDeclaration(bool strict = false, bool mustBeDeclaration = false)
+    Declaration parseDeclaration(bool strict = false, bool mustBeDeclaration = false, bool inTemplateDeclaration = false)


are alias assignments only strictly allowed in templates?

Additionally you are missing this parameter in a lot of sub-calls here.

Example that would break as far as I can see:

// parseConditionalDeclaration static if (true) { A = B; }

If I see a static if (true) in there at release I'm gonna burn down Adam's shed

As far as I could tell from the implementation [1], AliasAssign is allowed only inside template A(..){...} declarations.

I understand your point, however, it is not possible to deduce whether the static if is at function level or at template level during parsing. In dmd, this is checked during semantic analysis, so I assume that the solution for libdparse is to create an AliasAssignDeclaration every time we encounter an assignment that occurs in non-function contexts. ParseDeclaration seems to be called at function scope level also. Any ideas on how I could differentiate between assignments at function level and assignments non-function levels?

[1] https://github.com/dlang/dmd/pull/11846/files?file-filters%5B%5D=.d&file-filters%5B%5D=.h#diff-90e6074482976dda59fff5b12d8aa29f1af3012b5db226a827e8e304b953ec10R6774

I think you want to nest the inTemplateDeclaration parameter into all calls that should keep it, ie. here the parseConditionalDeclaration function and all the other parse*Declaration functions you want to support.

I personally think it would be more pretty doing this with some kind of parse state struct that is passed as argument in all related functions though if this is expected to expand in the future to more edge cases.

I'm not sure this will work. Consider this case:

template X(T) { alias X = int; static if (cond) X = float; // this one is currently accepted by dmd void fun() { X = T; // while this one is not because the assignment // does not have the same parent as the declaration of X } }

This is not going to get solved by passing the inTemplate boolean. Or maybe it could work, but that would mean inserting a primitive form of semantic analysis. Right now, libdparse issues an error when
assignments occur in non-function contexts. If we want to keep things in parser territory, we need to relax that condition and to create AliasAssignDeclarations if an assignment occurs in non-function context. Of course that will allow for code that is not an alias assignment to pass parsing, but that is fine.

the parseFunctionDeclaration body would reset back the boolean value that you are in a template, (implicitly by simply not passing it down as parameter or for a struct explicitly) you will want to do that with anything adding scope.

hmm, I'll try that out. Thanks for your assistance!

test/pass_files/alias_assign.d

WebFreak001 · 2021-05-06T13:33:12Z

src/dparse/parser.d

+     * Parses an AliasAssign.
+     *
+     * $(GRAMMAR $(RULEDEF aliasAssign):
+     *       $(RULE aliasInitializer)


this documentation says it would be syntactically valid to use templateParameters in an assignment. As it's never the case that it would actually be valid code I think it would be better to define a proper grammar here.

test/pass_files/alias_assign.d

RazvanN7 · 2021-05-10T15:39:04Z

@WebFreak001 @maxhaton I gave it another pass. Kindly asking for a review

WebFreak001

looks good, just 2 minor things

I think the state passing through parameters is functional and works well with the stack without much overhead, but it might get hard to maintain in case more stuff like this gets added. What does @Hackerpilot think about this? Maybe some explicit push/pop functionality could be considered too? (I think the current way would be the most elegant solution once we have named parameters and not need to write down all the defaults)

WebFreak001 · 2021-05-10T16:03:06Z

src/dparse/parser.d

+        mixin(tokenCheck!(`node.identifier`, "identifier"));
+        mixin(tokenCheck!"=");
+        mixin (parseNodeQ!(`node.type`, `Type`));
+        node.tokens = tokens[startIndex .. index];


this line is not needed / covered by attachCommentFromSemicolon

Suggested change

node.tokens = tokens[startIndex .. index];

WebFreak001 · 2021-05-10T16:07:29Z

src/dparse/parser.d

+        comment = null;
+        mixin(tokenCheck!(`node.identifier`, "identifier"));
+        mixin(tokenCheck!"=");
+        mixin (parseNodeQ!(`node.type`, `Type`));


and just a style thing because otherwise the PR LGTM :p

Suggested change

mixin (parseNodeQ!(`node.type`, `Type`));

mixin(parseNodeQ!(`node.type`, `Type`));

maxhaton · 2021-05-10T16:25:19Z

@RazvanN7 Don't have time to consider this thoroughly but looks good to me for now. Thanks.

RazvanN7 · 2021-05-11T06:49:27Z

@WebFreak001 I agree that keeping all the state in one place is easier to maintain, however, at this point that might be an overkill. Anyway, I would rather merge this ASAP to unblock the phobos PRs and refactor later if needed.

WebFreak001 · 2021-05-11T09:58:47Z

travis fail unrelated

WebFreak001 · 2021-05-11T10:00:25Z

can't merge because travis is a required check

RazvanN7 · 2021-05-11T10:52:02Z

There seems to be a dependency error on stdx.allocator:

../subprojects/stdx-allocator/source/stdx/allocator/internal.d(722): Error: static assert:  `!__traits(compiles, emplace(& s2, 1))` is false

How do we get passed that? Did I mention that this PR is needed to unblock phobos :-" ?

RazvanN7 · 2021-05-11T11:39:03Z

Ok, so the problem seems to be that libdparse is using an ancient version of stdx.allocator (2.77.5), whereas newer versions don't even contain the assert that is tripping. How do we fix this? Should travis-ci be upgraded somehow or does it suffice to simply change the tag in submodules/stdx-allocator.wrap [1]? I'm gonna try this.

Edit: Hmm, do I have to specifically mention all recursive dependencies? It looks like the problem is that meson does not know how to pull mir-core (that the newer version of stdx.allocator requires).

[1] https://github.com/dlang-community/libdparse/blob/master/subprojects/stdx-allocator.wrap

RazvanN7 · 2021-05-20T11:10:02Z

@WebFreak001 I have disabled the meson build as it seemed redundant. Once the dependency issues for meson are fixed, the tests can be un-commented. Is that acceptable?

codecov · 2021-05-20T11:10:39Z

Codecov Report

Merging #441 (469bffd) into master (6f0893f) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
+ Coverage   81.86%   82.12%   +0.25%     
==========================================
  Files          11       11              
  Lines        8454     8463       +9     
==========================================
+ Hits         6921     6950      +29     
+ Misses       1533     1513      -20

Impacted Files	Coverage Δ
src/dparse/ast.d	`70.78% <100.00%> (+0.05%)`	⬆️
src/dparse/parser.d	`91.24% <100.00%> (+0.04%)`	⬆️
src/dparse/formatter.d	`42.83% <0.00%> (+1.25%)`	⬆️
src/std/experimental/lexer.d	`86.95% <0.00%> (+2.17%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f0893f...469bffd. Read the comment docs.

RazvanN7 · 2021-05-20T11:50:46Z

Is this good to go?

RazvanN7 force-pushed the AliasAssign branch from 866d20c to 04246ba Compare May 6, 2021 09:53

RazvanN7 mentioned this pull request May 6, 2021

Simplify and improve Erase and EraseAll dlang/phobos#8033

Merged

RazvanN7 mentioned this pull request May 6, 2021

Dscanner does not take into account AliasAssignment dlang-community/D-Scanner#831

Open

MoonlightSentinel reviewed May 6, 2021

View reviewed changes

RazvanN7 force-pushed the AliasAssign branch from 04246ba to e7791dc Compare May 6, 2021 10:18

WebFreak001 requested changes May 6, 2021

View reviewed changes

WebFreak001 reviewed May 6, 2021

View reviewed changes

maxhaton reviewed May 6, 2021

View reviewed changes

test/pass_files/alias_assign.d Show resolved Hide resolved

RazvanN7 force-pushed the AliasAssign branch from e7791dc to 52687e4 Compare May 10, 2021 15:38

WebFreak001 requested changes May 10, 2021

View reviewed changes

RazvanN7 force-pushed the AliasAssign branch from 52687e4 to e599afd Compare May 11, 2021 06:45

WebFreak001 approved these changes May 11, 2021

View reviewed changes

RazvanN7 force-pushed the AliasAssign branch 3 times, most recently from 39cf5ed to 8878047 Compare May 11, 2021 11:51

Make parser recognize AliasAssign

09a234a

RazvanN7 force-pushed the AliasAssign branch from 8878047 to 09a234a Compare May 20, 2021 11:08

Add note why meson tests are disabled now

469bffd

ghost approved these changes May 20, 2021

View reviewed changes

WebFreak001 merged commit fbdf71b into dlang-community:master May 20, 2021

	* $(GRAMMAR $(RULEDEF aliasDeclaration):
	* $(GRAMMAR $(RULEDEF aliasAssign):

	mixin (parseNodeQ!(`node.type`, `Type`));
	mixin(parseNodeQ!(`node.type`, `Type`));

Conversation

RazvanN7 commented May 6, 2021

Uh oh!

RazvanN7 commented May 6, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

WebFreak001 left a comment

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!

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!

RazvanN7 May 7, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RazvanN7 commented May 10, 2021

Uh oh!

WebFreak001 left a comment

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!

maxhaton commented May 10, 2021

Uh oh!

RazvanN7 commented May 11, 2021

Uh oh!

WebFreak001 commented May 11, 2021

Uh oh!

WebFreak001 commented May 11, 2021

Uh oh!

RazvanN7 commented May 11, 2021

Uh oh!

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

Uh oh!

Uh oh!

RazvanN7 commented May 20, 2021

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

RazvanN7 commented May 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

RazvanN7 May 7, 2021 •

edited

Loading

RazvanN7 commented May 11, 2021 •

edited

Loading

codecov bot commented May 20, 2021 •

edited

Loading