-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
chore: upgrade to Chevrotain 11 #616
chore: upgrade to Chevrotain 11 #616
Conversation
6272113
to
3a19698
Compare
Rebased and resolved merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting PR ! I have a few questions I put in comments
@@ -79,11 +80,11 @@ const sampleRepos = [ | |||
{ | |||
repoUrl: "https://github.com/jhipster/jhipster-sample-app-react", | |||
branch: "v8.0.0-rc.1" | |||
}, | |||
} /*, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you need to comment this one ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest commit (nipafx/demo-java-x@a84483f) added examples that use the JEP 430 String Templates feature, which is a preview feature in Java 21 (and not yet supported by Prettier Java). This is currently causing CI failures on our main branch: https://github.com/jhipster/prettier-java/actions/runs/6833694201
Unfortunately, the repo doesn't have any branches/tags other than main
. The "correct" fix would likely be to make it possible to lock a repo by commit SHA as an alternative to branch/tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't spot the string template issue. I started working on supporting string templates, but still had some issues with it.
I think we can specify a specific commit (we did it in the past, I will look at it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can have a look at
function cloneRepo({ repoUrl, branch, commitHash }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just copied this implementation over and leveraged it. 🎉
|
||
const { printDocToString } = doc.printer; | ||
const { readFileSync, existsSync, removeSync, copySync } = fs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was import { readFileSync, existsSync, removeSync, copySync } from "fs-extra";
not working anymore ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Because of the conversion to ESM, and because fs-extra
is a CJS module, I got the following error:
import { readFileSync, existsSync, removeSync, copySync } from "fs-extra";
^^^^^^^^
SyntaxError: Named export 'copySync' not found. The requested module 'fs-extra' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:
import pkg from 'fs-extra';
const { readFileSync, existsSync, removeSync, copySync } = pkg;
I tried upgrading fs-extra
to its latest version (11.1.1), but received the same error.
In the future we should be able to replace this usage of fs-extra
with fs
, since it contains equivalent functions to these 4, but for this PR I figured that would be inappropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, we can create an issue to follow this one
"use strict"; | ||
|
||
const javaParserChev = require("../src/index"); | ||
import javaParserChev from "../src/index"; | ||
|
||
const input = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I run this script with the following code snippet, I have the following error, do you have the same ?
import javaParserChev from "../src/index.js";
const input = `
public class T {
void t() {
var a = "toto";
}
}
`;
javaParserChev.parse(input, "compilationUnit");
Error:
Ambiguous Alternatives Detected: <0, 1> in inside Rule,
<'var', Identifier, '='> may appears as a prefix path in all these alternatives.
See: https://chevrotain.io/docs/guide/resolving_grammar_errors.html#AMBIGUOUS_ALTERNATIVES
For Further details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem to appear in the tests also: https://github.com/jhipster/prettier-java/actions/runs/6835796470/job/18589989957?pr=616#step:6:13722
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we ignore the logging message in the LLStarLookaheadStrategy ?
new LLStarLookaheadStrategy({ logging: () => {}}),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I noticed these ambiguity logs in the test output, forgot to ensure they wouldn't be logged during production (I thought this would be covered by skipValidations
, but I guess that only covers initialization validations). It would be nice if we could disable these logs for production, but leave them enabled for our tests (since it would be good to resolve them in the future). Perhaps with something like this?
new LLStarLookaheadStrategy({
logging: getSkipValidations() ? () => {} : undefined
}),
If @bd82 or @msujew could have a look, it would be great, as they are way more experts on chevrotain's parsers' internals than I am !! If they cannot, we can merge it next week, maybe. I propose to leave this out in the next release (which would include #611 and #614), so that we can release it today |
3a19698
to
1c82352
Compare
@clementdessoude Thanks for the review! I agree with waiting to merge this until after the next release -- wouldn't want to hold up those Java 21 feature additions for this upgrade. I just rebased (to resolve merge conflicts with |
Sorry @jtkiesel, it seems that the merge of the other PR created some conflicts on this one :/ |
a43ce80
to
38afbfe
Compare
38afbfe
to
0c65c51
Compare
@clementdessoude No worries! Rebased and resolved conflicts. 👍 |
@msujew fyi - usage of https://github.com/TypeFox/chevrotain-allstar |
@clementdessoude Apologies for the mention, but can you give this another look when you have a minute? Should be good to merge. |
@jtkiesel so sorry for the delay, and thanks for the mention (and once again for the great work ;)) |
What changed with this PR:
java-parser
andprettier-plugin-java
are converted from CJS to ESM, to facilitate the upgrade to Chevrotain 11 (which is now ESM only).This PR's net performance change to parsing seems to be around 47% faster. It should be possible to achieve even further performance gains by removing the few remaining instances of backtracking. However, doing so appeared to be nontrivial, so for now I stopped after removing the ones that gave us easy wins.
Relative issues or prs:
Closes #423
Obsoletes #486