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.

Sign up for GitHub

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

Upgrade chevrotain #268

Merged
merged 9 commits into from
Sep 27, 2019
Merged

Upgrade chevrotain #268

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
53422cc
WIP: update version
clementdessoude Sep 24, 2019
a0cd147
make tests pass
clementdessoude Sep 24, 2019
40e81a1
update printer with following Chevrotain update
clementdessoude Sep 24, 2019
c3d349f
only wrap BACKTRACK_LOOKAHEAD in ACTION
clementdessoude Sep 25, 2019
43dbd20
revert changes on isModuleCompilationUnit
clementdessoude Sep 25, 2019
89d25fa
refactor function to not exit early during recording phase
clementdessoude Sep 26, 2019
c7f2dda
remove ignoredIssues config
clementdessoude Sep 26, 2019
5a620d9
remove gate in binaryExpression
clementdessoude Sep 27, 2019
4a70840
optimization by extracting logic to compute firstForUnaryExpressionNo…
clementdessoude Sep 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/java-parser/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"repository": "https://github.com/jhipster/prettier-java/tree/master/packages/java-parser",
"license": "Apache-2.0",
"dependencies": {
"chevrotain": "4.7.0",
"chevrotain": "6.5.0",
"lodash": "4.17.15"
},
"scripts": {
Expand Down
125 changes: 18 additions & 107 deletions packages/java-parser/src/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,97 +41,6 @@ class JavaParser extends Parser {
// TODO: performance: maxLookahead = 1 may be faster, but could we refactor the grammar to it?
// and more importantly, do we want to?
maxLookahead: 2,
// ambiguities resolved by backtracking
ignoredIssues: {
binaryExpression: {
OR: true
},
lambdaParameterType: {
OR: true
},
annotation: {
OR: true
},
localVariableType: {
OR: true
},
annotationTypeMemberDeclaration: {
OR: true
},
typeDeclaration: {
OR: true
},
typeArgument: {
OR: true
},
type: {
OR: true
},
referenceType: {
OR: true
},
compilationUnit: {
OR: true
},
classBodyDeclaration: {
OR: true
},
classMemberDeclaration: {
OR: true
},
unannReferenceType: {
OR: true
},
formalParameter: {
OR: true
},
interfaceMemberDeclaration: {
OR: true
},
blockStatement: {
OR: true
},
forStatement: {
OR: true
},
newExpression: {
OR: true
},
arrayCreationExpression: {
OR: true,
OR2: true
},
expression: {
OR: true
},
lambdaParameterList: {
OR: true
},
lambdaParameter: {
OR: true
},
primaryPrefix: {
OR: true
},
castExpression: {
OR: true
},
referenceTypeCastExpression: {
OR: true
},
elementValue: {
OR: true
},
resource: {
OR: true
},
forInit: {
OR: true
},
interfaceDeclaration: {
OR: true
}
},
nodeLocationTracking: "full"
});

Expand Down Expand Up @@ -172,23 +81,25 @@ class JavaParser extends Parser {
}

BACKTRACK_LOOKAHEAD(production, errValue = false) {
this.isBackTrackingStack.push(1);
// TODO: "saveRecogState" does not handle the occurrence stack
const orgState = this.saveRecogState();
try {
// hack to enable outputting none CST values from grammar rules.
this.outputCst = false;
return production.call(this);
} catch (e) {
if (isRecognitionException(e)) {
return errValue;
return this.ACTION(() => {
this.isBackTrackingStack.push(1);
// TODO: "saveRecogState" does not handle the occurrence stack
const orgState = this.saveRecogState();
try {
// hack to enable outputting none CST values from grammar rules.
this.outputCst = false;
return production.call(this);
} catch (e) {
if (isRecognitionException(e)) {
return errValue;
}
throw e;
} finally {
this.outputCst = true;
this.reloadRecogState(orgState);
this.isBackTrackingStack.pop();
}
throw e;
} finally {
this.outputCst = true;
this.reloadRecogState(orgState);
this.isBackTrackingStack.pop();
}
});
}

setIgnoredComments(comments) {
Expand Down
15 changes: 11 additions & 4 deletions packages/java-parser/src/productions/blocks-and-statements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,16 @@ function defineRules($, t) {
const isLocalVariableDeclaration = this.BACKTRACK_LOOKAHEAD(
$.isLocalVariableDeclaration
);

const isClassDeclaration = this.BACKTRACK_LOOKAHEAD($.isClassDeclaration);

$.OR([
{
GATE: () => isLocalVariableDeclaration,
ALT: () => $.SUBRULE($.localVariableDeclarationStatement)
},
{
GATE: () => isClassDeclaration,
clementdessoude marked this conversation as resolved.
Show resolved Hide resolved
ALT: () => $.SUBRULE($.classDeclaration)
},
{ ALT: () => $.SUBRULE($.statement) }
Expand All @@ -57,10 +61,13 @@ function defineRules($, t) {

// https://docs.oracle.com/javase/specs/jls/se11/html/jls-14.html#jls-LocalVariableType
$.RULE("localVariableType", () => {
$.OR([
{ ALT: () => $.SUBRULE($.unannType) },
{ ALT: () => $.CONSUME(t.Var) }
]);
$.OR({
DEF: [
{ ALT: () => $.SUBRULE($.unannType) },
{ ALT: () => $.CONSUME(t.Var) }
],
IGNORE_AMBIGUITIES: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to enforce not using 'var' in unannType

Copy link
Contributor

Choose a reason for hiding this comment

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

can var be used as an identifier? is that the cause of the ambiguity?

Copy link
Contributor

Choose a reason for hiding this comment

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

If that is the case (I have some fuzzy memory of limitations for the var keyword.
You may be able to define a IdentifierButNotVar Token using token categories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right. I think I will try to improve this in a following PR, adding a token and change some parser logic seems out of the scope of this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes definitively out of scope

});
});

// https://docs.oracle.com/javase/specs/jls/se11/html/jls-14.html#jls-Statement
Expand Down
8 changes: 7 additions & 1 deletion packages/java-parser/src/productions/classes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,13 +653,15 @@ function defineRules($, t) {
});

$.RULE("isClassDeclaration", () => {
let isEmptyTypeDeclaration = false;

if (
$.OPTION(() => {
$.CONSUME(t.Semicolon);
})
) {
// an empty "TypeDeclaration"
return false;
isEmptyTypeDeclaration = true;
}

try {
Expand All @@ -683,6 +685,10 @@ function defineRules($, t) {
}
clementdessoude marked this conversation as resolved.
Show resolved Hide resolved
}

if (isEmptyTypeDeclaration) {
return false;
}

const nextTokenType = this.LA(1).tokenType;
return (
tokenMatcher(nextTokenType, t.Class) ||
Expand Down
48 changes: 28 additions & 20 deletions packages/java-parser/src/productions/expressions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,13 @@ function defineRules($, t) {
});

$.RULE("lambdaParameterType", () => {
$.OR([
{ ALT: () => $.SUBRULE($.unannType) },
{ ALT: () => $.CONSUME(t.Var) }
]);
$.OR({
DEF: [
{ ALT: () => $.SUBRULE($.unannType) },
{ ALT: () => $.CONSUME(t.Var) }
],
IGNORE_AMBIGUITIES: true
});
});

$.RULE("lambdaBody", () => {
Expand Down Expand Up @@ -166,6 +169,8 @@ function defineRules($, t) {
}
},
{
// Remove ambiguity with first OR option
GATE: () => !tokenMatcher($.LA(1).tokenType, t.Instanceof),
clementdessoude marked this conversation as resolved.
Show resolved Hide resolved
ALT: () => {
$.CONSUME(t.BinaryOperator);
$.SUBRULE3($.unaryExpression);
Expand Down Expand Up @@ -604,21 +609,19 @@ function defineRules($, t) {
return true;
});

let firstForUnaryExpressionNotPlusMinus = undefined;
$.RULE("isReferenceTypeCastExpression", () => {
if (firstForUnaryExpressionNotPlusMinus === undefined) {
const firstUnaryExpressionNotPlusMinus = this.computeContentAssist(
"unaryExpressionNotPlusMinus",
[]
);
const nextTokTypes = firstUnaryExpressionNotPlusMinus.map(
x => x.nextTokenType
);
// uniq
firstForUnaryExpressionNotPlusMinus = nextTokTypes.filter(
(v, i, a) => a.indexOf(v) === i
);
}
const firstUnaryExpressionNotPlusMinus = this.computeContentAssist(
clementdessoude marked this conversation as resolved.
Show resolved Hide resolved
"unaryExpressionNotPlusMinus",
[]
);
const nextTokTypes = firstUnaryExpressionNotPlusMinus.map(
x => x.nextTokenType
);
// uniq
const firstForUnaryExpressionNotPlusMinus = nextTokTypes.filter(
(v, i, a) => a.indexOf(v) === i
);

$.CONSUME(t.LBrace);
$.SUBRULE($.referenceType);
$.MANY(() => {
Expand All @@ -635,6 +638,7 @@ function defineRules($, t) {
});

$.RULE("isRefTypeInMethodRef", () => {
let result = undefined;
$.SUBRULE($.typeArguments);

// arrayType
Expand All @@ -644,12 +648,12 @@ function defineRules($, t) {

const firstTokTypeAfterTypeArgs = this.LA(1).tokenType;
if (tokenMatcher(firstTokTypeAfterTypeArgs, t.ColonColon)) {
return true;
result = true;
}
// we must be at the end of a "referenceType" if "dims" were encountered
// So there is not point to check farther
else if (hasDims) {
return false;
result = false;
}

// in the middle of a "classReferenceType"
Expand All @@ -658,6 +662,10 @@ function defineRules($, t) {
$.SUBRULE($.classOrInterfaceType);
});

if (result !== undefined) {
return result;
}

const firstTokTypeAfterRefType = this.LA(1).tokenType;
return tokenMatcher(firstTokTypeAfterRefType, t.ColonColon);
});
Expand Down
28 changes: 18 additions & 10 deletions packages/java-parser/src/productions/interfaces.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ function defineRules($, t) {
const detectedType = this.BACKTRACK_LOOKAHEAD(
$.identifyInterfaceBodyDeclarationType
);

$.OR([
{
GATE: () => detectedType === InterfaceBodyTypes.constantDeclaration,
Expand Down Expand Up @@ -170,6 +171,7 @@ function defineRules($, t) {
const detectedType = this.BACKTRACK_LOOKAHEAD(
$.identifyAnnotationBodyDeclarationType
);

$.OR([
{
GATE: () =>
Expand Down Expand Up @@ -239,17 +241,22 @@ function defineRules($, t) {
// https://docs.oracle.com/javase/specs/jls/se11/html/jls-9.html#jls-MarkerAnnotation
$.OPTION(() => {
$.CONSUME(t.LBrace);
$.OR([
// normal annotation - https://docs.oracle.com/javase/specs/jls/se11/html/jls-9.html#jls-NormalAnnotation
{ ALT: () => $.SUBRULE($.elementValuePairList) },
// Single Element Annotation - https://docs.oracle.com/javase/specs/jls/se11/html/jls-9.html#jls-SingleElementAnnotation
{ ALT: () => $.SUBRULE($.elementValue) },
{
ALT: () => {
/* empty normal annotation contents */
$.OR({
DEF: [
// normal annotation - https://docs.oracle.com/javase/specs/jls/se11/html/jls-9.html#jls-NormalAnnotation
{ ALT: () => $.SUBRULE($.elementValuePairList) },
// Single Element Annotation - https://docs.oracle.com/javase/specs/jls/se11/html/jls-9.html#jls-SingleElementAnnotation
{
ALT: () => $.SUBRULE($.elementValue)
},
{
ALT: () => {
/* empty normal annotation contents */
}
}
}
]);
],
IGNORE_AMBIGUITIES: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ambiguous Alternatives Detected: <1 ,2> in inside Rule,
<Identifier, '='> may appears as a prefix path in all these alternatives.

It is because an elementValue could be a binaryExpression and could start with Identifier and '='. Do you think we must use a GATE or backtracking here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you use 3 or 4 tokens lookahead would that resolve in this case?
because you can increase the maxLookahead for this specific rule...
I don't remember the specific grammar rules, does the list alternative always have a comma somewhere? how far ahead is that comma?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, it is not enough to increase the maxLookahead for this rule. The list alternative always has a comma somewhere, but as the elementValue could be an expression, it could have several annotations and thus we cannot look ahead at a specific step.

});
$.CONSUME(t.RBrace);
});
});
Expand All @@ -274,6 +281,7 @@ function defineRules($, t) {
const isSimpleElementValueAnnotation = this.BACKTRACK_LOOKAHEAD(
$.isSimpleElementValueAnnotation
);

$.OR([
// Spec Deviation: "conditionalExpression" replaced with "expression"
// Because we cannot differentiate between the two using fixed lookahead.
Expand Down
3 changes: 3 additions & 0 deletions packages/java-parser/src/productions/packages-and-modules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ function defineRules($, t) {
$.RULE("compilationUnit", () => {
// custom optimized backtracking lookahead logic
const isModule = $.BACKTRACK_LOOKAHEAD($.isModuleCompilationUnit);

$.OR([
{
GATE: () => isModule === false,
Expand Down Expand Up @@ -98,6 +99,7 @@ function defineRules($, t) {
$.RULE("typeDeclaration", () => {
// TODO: consider extracting the prefix modifiers here to avoid backtracking
const isClassDeclaration = this.BACKTRACK_LOOKAHEAD($.isClassDeclaration);

$.OR([
{
GATE: () => isClassDeclaration,
Expand Down Expand Up @@ -243,6 +245,7 @@ function defineRules($, t) {
throw e;
}
}

const nextTokenType = this.LA(1).tokenType;
return (
tokenMatcher(nextTokenType, t.Open) ||
Expand Down
Loading