Skip to content

Resolve numerous ANTLR issues #261

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

Merged
merged 2 commits into from
May 5, 2021
Merged

Conversation

RexJaeschke
Copy link
Contributor

@RexJaeschke RexJaeschke commented Apr 15, 2021

This PR addresses the issues raised in Issue #230 as well as Issue #37, Proposal 3. Specifically, it

  • Replaces all lexer rule alternatives of the form '<...>' with correct content
  • Uses semantic predicates to resolve numerous issues, especially w.r.t identifier spelling
  • Uses ranges

@BillWagner, to support the semantic predicates, from a validation pov, we'll need to get the following text into the grammar file that we extract/generate, immediately following the current grammar directive in tools/validate-grammar.sh:

echo "grammar CSGrammar;" > $OUTPUT_FILE
@lexer::members {
bool IsLetterCharacter()       { return true; }
bool IsCombiningCharacter()    { return true; }
bool IsDecimalDigitCharacter() { return true; }
bool IsConnectingCharacter()   { return true; }
bool IsFormattingCharacter()   { return true; }
bool IsNotAKeyword()           { return true; }
bool IsNotTrueOrFalse()        { return true; }
}

Note that these are simply place-holder methods to allow validation. Clearly, they are quite incomplete from a lexer pov.

For now, I'm made this a DRAFT PR pending pushback on the two final uses of semantic predicates I proposed in Issue 230.

@RexJaeschke RexJaeschke added this to the C# 6 milestone Apr 15, 2021
@RexJaeschke RexJaeschke requested a review from Nigel-Ecma April 15, 2021 13:54
@RexJaeschke RexJaeschke self-assigned this Apr 15, 2021
@RexJaeschke RexJaeschke removed the request for review from Nigel-Ecma April 15, 2021 13:56
@BillWagner
Copy link
Member

@RexJaeschke I think the best way to proceed is for me to make a PR against this branch that adds the changes to the grammar validator. That way, they are in one place.

Does that make sense, or do you need those changes for other work?

@RexJaeschke
Copy link
Contributor Author

@BillWagner. OK; go ahead and make the grammar extractor changes a separate PR.

@BillWagner
Copy link
Member

@RexJaeschke

I just created #310 with the updates you mention. It's not working correctly, so we need to figure that out.

@RexJaeschke
Copy link
Contributor Author

@BillWagner, while I don't have a copy of the grammar file you extracted, my guess is these messages are coming from the pseudo-extensions to the grammar from unsafe.md, where an ellipses (...) is used to begin each grammar fragment. If we can get @Nigel-Ecma to review/merge PR ##233, all the ... lines (and their parent rules) will go away, and the grammar left in unsafe.md will all be needed and correct as is and no longer be considered as an add-on. This probably will fix the "missing ;" as well, as that is in a rule that is completely removed by that PR.

And you should change this PR (261) from Draft, and merge it as well, as a final test.

@BillWagner
Copy link
Member

@RexJaeschke #310 had a very obvious shell scripting mistake (obvious, at least, as soon as I saw it).

The results of the grammar validator on that PR is the same as this one now. Is that what you expected, or should we be expecting different diagnostics from ANTLR?

* Add lexer rules to grammar validation

Include the stub definitions for lexer members into the grammar file.

* Fix a script error. > vs. >>
@RexJaeschke RexJaeschke marked this pull request as ready for review May 4, 2021 20:39
@RexJaeschke RexJaeschke requested a review from BillWagner May 4, 2021 20:39
@RexJaeschke
Copy link
Contributor Author

@BillWagner I removed Draft status from this PR and assigned you as a reviewer.

If you want to test further without Nigel having reviewed/merged PR 233 to integrate the unsafe grammar changes, you could omit the inclusion of unsafe.md in your script.

@BillWagner BillWagner mentioned this pull request May 4, 2021
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big improvement @RexJaeschke

Let's merge it.

You can decide if you want to take #311 into this branch, or wait for those kinds of changes for #233

@BillWagner BillWagner merged commit d27b477 into draft-v6 May 5, 2021
@BillWagner BillWagner deleted the Rex-resolve-ANTLR-issues branch May 5, 2021 13:28
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.

2 participants