Grammar fixes identified during attempt to convert it to tree-sitter by CyberShadow · Pull Request #3019 · dlang/dlang.org

CyberShadow · 2021-06-11T08:42:13Z

The conversion program: https://github.com/CyberShadow/tree-sitter-d/tree/generated/generator

TODO:

Drop a7ddca3
Drop a182054
Rebase 4663823

dlang-bot · 2021-06-11T08:42:16Z

Thanks for your pull request, @CyberShadow!

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.

CyberShadow · 2021-06-18T23:38:16Z

OK, I think this is a good spot for a review/merge of everything so far.

The work above is what was needed to produce a working (albeit lightly tested) tree-sitter grammar. Most of these are fixes to DDoc syntax, grammar syntax, improvements to make the grammar more machine-readable and such. More fixes will follow in the future, but they will likely be of a different kind than most of the above (i.e. fixes in the actual grammar by clarifying ambiguous definitions or adding missing syntax variants).

CC @WalterBright @RazvanN7 @mdparker (looking at the git log I believe you are most involved in maintaining the spec lately).

mdparker · 2021-06-23T11:14:25Z

I don't consider myself qualified to review the actual grammar changes, but the rest of it (syntax, English), LGTM. If @WalterBright doesn't check in, maybe @MoonlightSentinel or @Geod24 can?

MoonlightSentinel · 2021-06-23T11:58:16Z

spec/function.dd


 $(GNAME FunctionLiteralBody):
-    $(GLINK SpecifiedFunctionBody)
+    $(GLINK BlockStatement)


This yields a dead link on the preview becauseBlockStatement is defined in statement.dd

On a side note, this rule should probably be moved to expression.d because it's only used in the function literal section

MoonlightSentinel · 2021-06-23T12:07:25Z

spec/lex.dd


+$(GRAMMAR
+$(GNAME SourceFile):
+    $(GLINK ByteOrderMark) $(GLINK Module)$(OPT)


$(GLINK Module) yields a dead link.

Does a leading byte-order mark really preclude a following shebang?
Otherwise this rule could be simplified to

ByteOrderMark (opt) Shebang (opt) Module (opt)

Yes, both the byte order mark and shebang want to be the very first bytes in the physical file. So, you can't have both.

MoonlightSentinel · 2021-06-23T12:13:12Z

spec/lex.dd

    $(GLINK HexString)
    $(GLINK DelimitedString)
-    $(GLINK TokenString)
+    $(GLINK TokenString))


The closing brace should appear on the next line s.t. it's consistent with the other rules.

MoonlightSentinel · 2021-06-23T12:18:49Z

spec/lex.dd

+    $(B q") $(GLINK Delimiter) $(GLINK WysiwygCharacters)$(OPT) $(GLINK MatchingDelimiter) $(B ")
+
+$(GNAME Delimiter):
+    $(B $(LPAREN))
+    $(B {)
+    $(B [)
+    $(B <)
+    $(GLINK Identifier)
+
+$(GNAME MatchingDelimiter):
+    $(B $(RPAREN))
+    $(B })
+    $(B ])
+    $(B >)
+    $(GLINK Identifier))


This rule could be modified to enforce the correct delimiters for almost all combinations except Identifier.

RazvanN7 · 2021-06-24T10:24:05Z

spec/class.dd

 )

-    $(P $(I ClassInvariant) specify the relationships among the members of a class instance.
+    $(P Class $(I Invariant) specify the relationships among the members of a class instance.


Suggested change

$(P Class $(I Invariant) specify the relationships among the members of a class instance.

$(P Class $(I Invariant)s specify the relationships among the members of a class instance.

RazvanN7 · 2021-06-24T10:28:51Z

spec/class.dd

 $(B Note): Class allocators are deprecated in D2.
 $(GRAMMAR
 $(GNAME Allocator):
-    $(D new) $(GLINK2 function, Parameters) $(D ;)


Even though class allocators are obsolete it seems that the parser does not complain about the following:

class A { new(size_t size); }

This deletion comes from this commit:

spec: Remove definitions redundant with MissingFunctionBody $(D ;) is already a possible FunctionBody (as MissingFunctionBody).

RazvanN7 · 2021-06-24T10:37:17Z

spec/class.dd

 $(GRAMMAR
 $(GNAME Deallocator):
-    $(D delete) $(GLINK2 function, Parameters) $(D ;)
    $(D delete) $(GLINK2 function, Parameters) $(GLINK2 function, FunctionBody)


Hm, it seems that delete declarations are no longer accepted by the parser.

class A { delete(void *p) {} }

This fails with a parser error since 2.091.1.
Since class allocators/deallocators both end up with compilation errors, maybe it's just better if we
remove them altoghether from the spec?

For this specific line deletion, please see above.

For the general topic of describing old language syntax, we can use GDEPRECATED.

There are some use cases to allow the grammar specify also previous versions of the language. If the grammar is used for anything other than develop a conforming implementation, such as code analysis tooling or (as is my case) syntax highlighting / editor support. Such tools have reasons to support previous versions of the language too.

We can use CSS or such to make removed / deprecated parts of the grammar clearly display as such (e.g. using 50% transparency or strikethrough).

adamdruppe · 2021-06-24T11:56:27Z

On Thu, Jun 24, 2021 at 03:40:16AM -0700, Razvan Nitu wrote: > @@ -1009,7 +1006,6 @@ $(H2 $(LNAME2 allocators, Class Allocators)) $(B Note): Class allocators are deprecated in D2. $(GRAMMAR $(GNAME Allocator): - $(D new) $(GLINK2 function, Parameters) $(D ;) Even though class allocators are obsolete it seems that the parser does not complain about the following:

note that it is specifically still allowed to do ***@***.*** new(...);`

adamdruppe · 2021-06-24T19:28:46Z

omg that message got wtf'd

but it is the "allocator relic" thing, with at disable new.

CyberShadow · 2021-06-25T21:22:04Z

@MoonlightSentinel @RazvanN7 Thank you for the review. I updated the PR with these changes:

Rebased to reorder and fold away reverts
Separated changes which actually change the grammar and moved them to Grammar fixes identified while validating the grammar against the DMD "compilable" test suite #3021, leaving only changes which fix syntax and other obvious errors
Changed more occurrences of consecutive $(D ...) to represent contiguous characters to use a single $(D ...)
Added a commit which expands the definition of DelimitedString to describe its nested structure (when using ()/{}/[]/<> delimiters) within the grammar (in Grammar fixes identified while validating the grammar against the DMD "compilable" test suite #3021)
Added a commit which consistently puts the closing ) of GRAMMAR blocks on its own line (and adjusted all commits here to do the same)
Added commits which repair broken links (and fixed new broken links in the commits here)
$(I Invariant)s

Rebase branch of the dlang.org repository which we're tracking in the generator/dlang.org submodule to address pull request review comments and for other cleanup. Summary of changes: dlang/dlang.org#3019 (comment) Old submodule refs referenced by previous commits in this repository will remain reachable via this branch: https://github.com/CyberShadow/d-programming-language.org/commits/grammar-v1

Enforce dlang/dlang.org#3019 (comment)

In all respects synonymous to GRAMMAR. To be used for grammar blocks which are not part of the formal definition of D's grammar.

Allow denoting grammar elements as such without ad-lib text annotations.

CyberShadow · 2021-06-29T22:54:56Z

Rebased to fix merge conflict with #3025.

Geod24

Very nice! A few things:

In commit "spec/lex.dd: Don't use $(D ...) for Unicode characters", there is the following message:

$(I ...) makes the backslash look weird, so use $(B ...) (freed up by the previous commit) instead.
I guess it's slightly outdate ?

"spec: Use $(D ...) instead of $(COMMA)": Why ? Simple curiosity here, as I'm the author of the code.
There's a lot of consistency fixes. Do you think you could amend/create a README or CONTRIBUTING so that all this knowledge is more prominently displayed / accessible ? Can be a new PR.
A previous version of this had parenthesis fixes. Was there bug reports opened for DMD's accept-invalid ?

Feel free to self merge when ready (I assume you might want to fix the commit message).

CyberShadow · 2021-06-30T06:34:10Z

In commit "spec/lex.dd: Don't use $(D ...) for Unicode characters", there is the following message:

Well spotted, yes, it is outdated. I will remove it.

"spec: Use $(D ...) instead of $(COMMA)": Why ? Simple curiosity here, as I'm the author of the code.

$(COMMA) is necessary to insert a literal comma in cases where it would otherwise be interpreted as an argument separator for multi-argument macros. As neither GRAMMAR nor D are multi-argument macros (i.e. they use $0 and not $1, $2 etc.), it would be synonymous with just using a comma. Furthermore, we would still need to wrap it in $(D ...) to indicate that it is a token.

There's a lot of consistency fixes. Do you think you could amend/create a README or CONTRIBUTING so that all this knowledge is more prominently displayed / accessible ? Can be a new PR.

I think the less documentation contributors need to read, the better. We can check some things in CI, and I hope that by making everything consistent once, it becomes easier to remain consistent simply by following the existing conventions. Some things like $(B/D/I ...) distinctions are more subtle though, maybe separate macros could be used instead of those.

A previous version of this had parenthesis fixes. Was there bug reports opened for DMD's accept-invalid ?

I'm guessing you mean #3017 . I believe DMD does warn on unbalanced parentheses if you run it with both -D and -w, however, nobody looks at the warnings.

Unlike almost all other uses of $(D ...) in grammar blocks, there are not verbatim D tokens, but *descriptions* of tokens (in the same way that "any Unicode character" is). $(I ...) makes the backslash look weird, so use $(B ...) for this purpose instead.

Disambiguate $(I ...) from other meanings.

We can use $(D ...) for tokens which may be surrounded by whitespace, and $(B ...) for character sequences which must be contiguous.

These "short-hands" complicate parsing, and probably shouldn't even exist at all.

The grammar definition is at the start of the section, so there is little change in functionality.

Be consistent about this.

As opposed to linking to grammar definitions, which is what GLINK/GLINK2 is mostly used for. These are not part of the grammar, so use the syntax/formatting used for verbatim D tokens instead. As LINK2 / RELATIVE_LINK2 is already used for this purpose in the spec, this further improves consistency (in addition to the primary goal of improving machine readability).

Consistently indicate that these form a single token, with no interleaved whitespace/comments.

Consistently indicate where whitespace/comments may appear between tokens.

To be used for blocks describing sequences of characters (which are contiguous), as opposed to sequences of tokens (which may have whitespace / comments between them).

A few blocks needed to be split up into lexical (characters) and regular (tokens) blocks.

…okens As the q and { in q{ must be contiguous, the two characters here act as a token.

These are all linking to the current page.

When the definition is not in the same file, we need to use GLINK2 and point at the correct file.

Geod24 · 2021-06-30T07:23:00Z

Bad bot!

CyberShadow force-pushed the grammar branch 2 times, most recently from f97b664 to 2473628 Compare June 12, 2021 06:59

CyberShadow mentioned this pull request Jun 14, 2021

Fix Issue 22019 - case 1,: allowed by grammar but not DMD dlang/dmd#12677

Merged

CyberShadow marked this pull request as ready for review June 18, 2021 23:38

CyberShadow requested review from WalterBright and andralex as code owners June 18, 2021 23:38

CyberShadow mentioned this pull request Jun 19, 2021

Grammar fixes identified while validating the grammar against the DMD "compilable" test suite #3021

Closed

MoonlightSentinel reviewed Jun 23, 2021

View reviewed changes

RazvanN7 reviewed Jun 24, 2021

View reviewed changes

CyberShadow force-pushed the grammar branch from 8f36e9b to 01511b2 Compare June 25, 2021 21:15

CyberShadow added a commit to CyberShadow/tree-sitter-d that referenced this pull request Jun 25, 2021

[generator] generator: Verify whitespace at the end of GRAMMAR nodes

211fda1

Enforce dlang/dlang.org#3019 (comment)

dlang-bot added the Needs Rebase label Jun 27, 2021

RazvanN7 approved these changes Jun 29, 2021

View reviewed changes

CyberShadow added 9 commits June 29, 2021 22:50

dlang.org.ddoc: Add GRAMMAR_INFORMATIVE

0cf5a39

In all respects synonymous to GRAMMAR. To be used for grammar blocks which are not part of the formal definition of D's grammar.

spec: Use GRAMMAR_INFORMATIVE where appropriate

eea2c27

spec: Add missing colons at the end of GNAME lines

a06318c

spec/iasm.dd: Fix indentation

5046819

spec/statement.dd: Replace mis-use of $(GRAMMAR) with link

61cb552

spec: Replace ad-hoc repetition "syntax" with formal syntax

20218bb

dlang.org.ddoc: Introduce GDEPRECATED, GRESERVED

11e6045

Allow denoting grammar elements as such without ad-lib text annotations.

spec/lex.dd: Use GDEPRECATED, GRESERVED

9ab3be2

spec: Add missing $(D ...) around verbatim characters (tokens)

08fdd32

CyberShadow force-pushed the grammar branch from 01511b2 to 780324b Compare June 29, 2021 22:53

Geod24 approved these changes Jun 30, 2021

View reviewed changes

CyberShadow added 23 commits June 30, 2021 06:36

dlang.org.ddoc: Introduce GSELF

7681426

Disambiguate $(I ...) from other meanings.

spec: Replace $(I ...) with $(GSELF ...) where appropriate

d929736

spec: Replace $(I ...) with $(GLINK ...) where appropriate

85008e8

spec: Replace $(GLINK ...) with $(GSELF ...) in self-references

08db23f

spec/lex.dd: Consistently use $(B ...) for character sequences

3a3ff86

We can use $(D ...) for tokens which may be surrounded by whitespace, and $(B ...) for character sequences which must be contiguous.

spec: Don't use CODE_... macros in grammar blocks

1f9f3af

These "short-hands" complicate parsing, and probably shouldn't even exist at all.

spec: Use GLINK instead of RELATIVE_LINK2

e275036

The grammar definition is at the start of the section, so there is little change in functionality.

spec: Use $(D ...) instead of $(COMMA)

f66d1ed

spec: Move $(RELATIVE_LINK2 ...) out of $(D ...)

b8aa323

Be consistent about this.

spec: Use single $(D ...) for multi-character tokens

8590244

Consistently indicate that these form a single token, with no interleaved whitespace/comments.

spec: Add spaces between tokens where appropriate

6a470cf

Consistently indicate where whitespace/comments may appear between tokens.

dlang.org.ddoc: Introduce GRAMMAR_LEX

e970d23

To be used for blocks describing sequences of characters (which are contiguous), as opposed to sequences of tokens (which may have whitespace / comments between them).

spec: Use GRAMMAR_LEX where appropriate

f090a1f

A few blocks needed to be split up into lexical (characters) and regular (tokens) blocks.

spec/lex.dd: Use $(D ...) for character sequences where they act as t…

599d110

…okens As the q and { in q{ must be contiguous, the two characters here act as a token.

spec/lex.dd: Consistently place the closing ) on its own line

6513cad

spec: Replace redundant $(GLINK2 ...) with $(GLINK ...)

5d8ebd2

These are all linking to the current page.

spec: Fix broken GLINK2 links

08e3f08

spec: Fix broken GLINK links

b08927b

When the definition is not in the same file, we need to use GLINK2 and point at the correct file.

spec/type.dd: Fix link to TraitsExpression

ecf8ae5

spec/function.dd: Fix self-identical rule in ParameterAttributes

9943fbc

spec/statement.dd: Remove redundant $(OPT)

eae13f5

CyberShadow force-pushed the grammar branch from 780324b to eae13f5 Compare June 30, 2021 06:38

RazvanN7 added the auto-merge label Jun 30, 2021

Geod24 merged commit 6b03aa3 into dlang:master Jun 30, 2021

	$(P Class $(I Invariant) specify the relationships among the members of a class instance.
	$(P Class $(I Invariant)s specify the relationships among the members of a class instance.

Uh oh!

Comments

Conversation

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

Uh oh!

Uh oh!

dlang-bot commented Jun 11, 2021

Bugzilla references

Uh oh!

CyberShadow commented Jun 18, 2021

Uh oh!

mdparker commented Jun 23, 2021

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!

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!

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!

adamdruppe commented Jun 24, 2021 via email

Uh oh!

adamdruppe commented Jun 24, 2021

Uh oh!

CyberShadow commented Jun 25, 2021

Uh oh!

CyberShadow commented Jun 29, 2021

Uh oh!

Geod24 left a comment

Choose a reason for hiding this comment

Uh oh!

CyberShadow commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Geod24 commented Jun 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

CyberShadow commented Jun 11, 2021 •

edited

Loading

CyberShadow commented Jun 30, 2021 •

edited

Loading