Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ANTLR] Various changes. #52293

Closed
wants to merge 3 commits into from
Closed

[ANTLR] Various changes. #52293

wants to merge 3 commits into from

Conversation

modulovalue
Copy link
Contributor

@modulovalue modulovalue commented May 7, 2023

This PR proposes the following changes:

Convert left-recursion in cascade to iteration.

Since one of the goals of the grammar is to be a reference for recursive-descent-style parsers, I believe this is a desirable change.

Convert right-recursion in unaryExpression to iteration.

In the spirit of the PR that added the previous changelog entry, this simplifies the grammar by making it explicit that unary expressions have a regular structure.

Remove the dependency on order for disambiguating <typeArguments> <arguments> and <arguments> in selectors.

Closes dart-lang/language#3039

Fix inconsistent order-dependent disambiguation between methods and external constructors.

The following program

class Bar {
  Bar();
  external Bar();
}

has the following parse tree:

<startSymbol>
┗━ <libraryDefinition>
  ┣━ <metadata>
  ┣━ <topLevelDefinition>
  ┃  ┗━ <classDeclaration>
  ┃    ┣━ <classModifiers>
  ┃    ┣━ 'class'
  ┃    ┣━ <typeWithParameters>
  ┃    ┃  ┗━ <typeIdentifier>
  ┃    ┃    ┗━ 'Bar'
  ┃    ┣━ '{'
  ┃    ┣━ <metadata>
  ┃    ┣━ <classMemberDeclaration>
  ┃    ┃  ┣━ <declaration>
  ┃    ┃  ┃  ┗━ <functionSignature>
  ┃    ┃  ┃    ┣━ <identifier>
  ┃    ┃  ┃    ┃  ┗━ 'Bar'
  ┃    ┃  ┃    ┗━ <formalParameterPart>
  ┃    ┃  ┃      ┗━ <formalParameterList>
  ┃    ┃  ┃        ┣━ '('
  ┃    ┃  ┃        ┗━ ')'
  ┃    ┃  ┗━ ';'
  ┃    ┣━ <metadata>
  ┃    ┣━ <classMemberDeclaration>
  ┃    ┃  ┣━ <declaration>
  ┃    ┃  ┃  ┣━ 'external'
  ┃    ┃  ┃  ┗━ <constructorSignature>
  ┃    ┃  ┃    ┣━ <constructorName>
  ┃    ┃  ┃    ┃  ┗━ <typeIdentifier>
  ┃    ┃  ┃    ┃    ┗━ 'Bar'
  ┃    ┃  ┃    ┗━ <formalParameterList>
  ┃    ┃  ┃      ┣━ '('
  ┃    ┃  ┃      ┗━ ')'
  ┃    ┃  ┗━ ';'
  ┃    ┗━ '}'
  ┗━ '<EOF>'

Notice how the external method is recognized as being a constructor. This is inconsistent with how non-external constructors are being recognized. This PR fixes this to treat external methods and constructors the way that non-external methods and constructors are being treated.

Minor formatting changes.

I think the changes related to formatting are self-explanatory. They are meant to make those parts match how the rest of the file is formatted.

See the added changelog entry.
@copybara-service
Copy link

Thank you for your contribution. This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/301728

Please wait for a developer to review your code review at the above link. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly. You can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code and they will help you. You can also push additional commits to this pull request to update the code review.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/301728 has been updated with the latest commits from this pull request.

1 similar comment
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/301728 has been updated with the latest commits from this pull request.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/301728 has been updated with the latest commits from this pull request.

2 similar comments
@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/301728 has been updated with the latest commits from this pull request.

@copybara-service
Copy link

https://dart-review.googlesource.com/c/sdk/+/301728 has been updated with the latest commits from this pull request.

@eernstg
Copy link
Member

eernstg commented May 16, 2023

Hi @modulovalue, thanks for the input!

This PR makes a lot of changes to the grammar, and this is usually something that involves discussions for quite a while. For instance, the <cascade> rules were adjusted at some point such that they would allow for a specification of the semantics of null shorting (the fact that a?.b.c evaluates a and then evaluates to null, rather than evaluating to null and then throwing because null.c fails). This means that it's going to be a rather involved discussion about the consequences (e.g., for the ability to keep track of the connection to the language specification, and to the implementation) if we're going to change the structure of the grammar, even in the case where it is rather easy to prove that it doesn't change the language which is derivable from that grammar.

Could you say something about your motivation for proposing these changes?

@modulovalue
Copy link
Contributor Author

To be clear, I don't intend for the cascade and unaryExpression-related changes to change the language that the grammar describes. If they do, then it's an oversight on my part.

for the ability to keep track of the connection to the language specification

I completely understand that this is important and should be considered. If you strongly feel that any of the changes proposed here diverge too much from the goals that you have for the specification, then I can remove them and perhaps open a separate issue for them.


Could you say something about your motivation for proposing these changes?

I've been making great progress on parsing Dart using a custom GLR-style parser generator and my goal is to have my grammar not diverge too much from the specification.

One of the issues that I need to solve to make that practical is to automatically transform parse trees into shallow ASTs. I plan to do this by transforming chains of composed expressions (e.g. ifNullExpression -> logicalOrExpression -> logicalAndExpression -> ...) into a single sum type. I plan to do this by allowing my grammar to specify spanning trees (in addition to the usual one level deep alternations). A spanning tree should then be able to break cycles that can't be removed manually (because converting their recursive rules into equivalent iterative ones is not possible).

Unfortunately, small cycles like the unaryExpression one or the cascade one make it much harder to assign a single spanning tree to the big expression cycle.

Also, it looks like the recursive view of awaitExpressions causes the compiler implementation to stack overflow (#52413). Treating await as a (syntactical) prefix operator might be able to fix that?

@devoncarew devoncarew requested a review from eernstg May 17, 2023 17:51
@devoncarew
Copy link
Member

(assigning a reviewer to remove this from the triage queue)

@mraleph
Copy link
Member

mraleph commented Oct 19, 2023

@eernstg ping?

@modulovalue
Copy link
Contributor Author

Quote:

This PR makes a lot of changes to the grammar [...]

I was planning to revisit this. I will close it until then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[grammar] selector/typeArgument-related ambiguity.
4 participants