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

Add/update categories to Rascal grammar #2083

Merged
merged 9 commits into from
Dec 9, 2024

Conversation

sungshik
Copy link
Contributor

@sungshik sungshik commented Nov 18, 2024

This PR:

  • Adds new category tags in the Rascal grammar to number literals, regular expression literals, and location literals, in accordance with the temporary patch in the LSP server.
  • Updates existing category tags in the Rascal grammar from legacy token types to LSP token types, in accordance with this table, as suggested by the unticked checkbox.

(Outside the scope of this PR: #1928)

Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 49%. Comparing base (bea8828) to head (6bf08d1).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/org/rascalmpl/ast/ShellCommand.java 0% 7 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              main   #2083   +/-   ##
=======================================
- Coverage       49%     49%   -1%     
+ Complexity    6330    6322    -8     
=======================================
  Files          666     666           
  Lines        59680   59689    +9     
  Branches      8668    8668           
=======================================
- Hits         29616   29607    -9     
- Misses       27831   27849   +18     
  Partials      2233    2233           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DavyLandman
Copy link
Member

I do not think we want the lsp strings in this grammar, I get where you are coming from, but I think in that PR I didn't understand all the concerns.

I think the #1928 issue would be more a preferred path.

Whats your take @jurgenvinju ? I remember you had a strong desire not to put more strings in the rascal grammar, and instead move to these ADTs instead. Or do you think it's wise to -- for now -- add the lsp categories in the rascal grammar, as an example for other users?

@jurgenvinju
Copy link
Member

jurgenvinju commented Nov 20, 2024

The issue is that we have not changed the Rascal grammar at all for fear of interaction with the bootstrapping of the compiler. In the time of the RVM compiler (Rascal Virtual Machine), this was essential as it already went through the second and third cycles in bootstrapping, and there production tags like @category end up in generated code. If you change them, then pattern matching won't work and the next first step of bootstrapping fails.

So my question would be to @PaulKlint: what is the current expected impact of productions in the Rascal bootstrap parser (RascalParser.java) changing their structural equality due to changed or new @category tags?

If it is low, then we can do this now, and we can do it again when we change the contract of highlight categories. If it is high, then we should consider waiting until the compiler is ready for such changes.

@jurgenvinju jurgenvinju self-requested a review November 20, 2024 15:27
@jurgenvinju
Copy link
Member

jurgenvinju commented Nov 20, 2024

This PR is missing:

  • regeneration of the AST classes (just to be sure)
  • regeneration of the bootstrap parser Java code

Both can be called directly from lang::rascal::grammar::Bootstrap using the bootstrap function. The parameter should be the root of the rascal project folder; which contains the pom.xml file. The function will take care of everything.

  • The diff in the RascalParser.java file can be large due to accidental reorderings of production rules in the hash trie sets.
  • The diff in the AST classes should be 0 (zero). If not then changes to the grammar were made without regenerating the AST classes and we have to do some excavating to find out who did that and why.

Copy link
Member

@jurgenvinju jurgenvinju left a comment

Choose a reason for hiding this comment

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

I do think this PR will help make the highlighting better.
See earlier comments about required parser and AST code (re)generation.

@sungshik
Copy link
Contributor Author

sungshik commented Dec 6, 2024

Thank you, @jurgenvinju, for the bootstrapping instructions 🙂

I regenerated the parser and the AST classes. The diff consists of 109 files, which was a bit unexpected, so I moved the commit to a separate branch (#2091). After looking at the changes in more detail, though, they seem harmless:

  • 1x the regenerated parser class. Expected.
  • 107x the insertion of @SuppressWarnings(value = {"unused"}) to an AST class. Unexpected but harmless.
  • 1x the insertion of an unused variable declaration. Unexpected, and I'm not sure where it comes from, but as the declaration is unused, it also seems harmless.

If you agree with this analysis, I'll merge that bootstrap branch into this one. Subsequently, this branch can be merged into main.

Copy link
Member

@jurgenvinju jurgenvinju left a comment

Choose a reason for hiding this comment

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

Yes. Your analysis looks good. Thanks

@DavyLandman DavyLandman merged commit 6ed70ad into main Dec 9, 2024
6 of 7 checks passed
@sungshik sungshik deleted the add-categories-to-rascal-grammar branch December 9, 2024 10:51
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.

3 participants