Fix issues with records and support instanceof final #124
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bkramer-relyance
changed the title
Contributor
Author
|
Requesting CI testing and review. Thank you. |
grammar.js
Outdated
| 'open', | ||
| 'module', | ||
| 'record' | ||
| 'record', |
Contributor
Author
There was a problem hiding this comment.
Accidentally left in this trailing ,. I'll remove this when I make an update.
Contributor
|
@bmahe is this related to your recent PR? |
aryx
approved these changes
Sep 18, 2022
Contributor
|
Could you rebase to solve the conflicts? |
bkramer-relyance
force-pushed
the
bkramer/records_and_instanceof_final
branch
from
2d8084f to
9fb5d40
Compare
bkramer-relyance
changed the title
bkramer-relyance
force-pushed
the
bkramer/records_and_instanceof_final
branch
from
9fb5d40 to
ee2d345
Compare
Contributor
Author
|
@aryx I rebased my branch on master and force pushed. This could use another round of testing before merging. Thank you. |
aryx
approved these changes
Sep 19, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR continues the work of supporting Java records, letting us:
See https://docs.oracle.com/en/java/javase/18/language/records.html.
Caveat: to reduce the complexity of the grammar/parser, I defined compact constructors to be accepted at any place one might find a normal constructor, making the tree-sitter parser more permissive than the spec. I'm going by the assumption that the philosophy of tree-sitter is to understand the correct code, not to enforce it. (I could have made the
compact_constructor_declarationrule by allowing methods to have no arguments, but felt that was taking it too far.) I'm open to some guidance on this topic.Because it was convenient for me (fewer versions of generated code to keep track of), I am also adding in this PR support for the optional
finalmodifier afterinstanceof. I can't find the spec for this, but here's some communication concerning it: checkstyle/checkstyle#9897All of these issues were discovered in the Elasticsearch repo.
Checklist:
Question: while I was working out the grammar updates for records, I noticed that there is the inclusion of
recordas a "reserved identifier," along with just a couple of others. This seems pretty arbitrary and I'm wondering if they are to deal with cases where the Java language permits these keywords to be repurposed as identifiers, like with lambda arguments. I've seen this in one of the corpus tests, as well as formodulein one of the Github repos we use to test. I also see a current parse failure in the Ghidra repo wheresealedappears as a variable name undergoing assignment. So what's the general story here? Are we just opportunistically adding reserved identifiers to deal with these discoveries, and if so, isn't the naming backward (we are not "reserving keywords" but rather "un-reserving them.")?