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

Jump to bottom

Replace ANTLR with hand-rolled parser #917

Merged
merged 69 commits into from
Feb 12, 2025
Merged

Replace ANTLR with hand-rolled parser #917

merged 69 commits into from
Feb 12, 2025

Conversation

stackoverflow
Copy link
Contributor

@stackoverflow stackoverflow commented Jan 29, 2025 •
edited
Loading

Fixes #906
Fixes #888
Fixes #927
Fixes #931
Fixes #932

Incompatible changes:

  • Object entries in the same line now require a semicolon (properties and elements are unaffected):
// not valid anymore
foo { ["1"] = 1 ["2"] = 2 }

// requires ;
foo { ["1"] = 1; ["2"] = 2 }

Reason: not requiring semicolons or newlines requires too much backtracking in the parser, degrading performance. Also both our IntelliJ plugin and the LSP don't allow this syntax and show an error.

Our current ANTLR parser is quite slow, and, depending on the codebase, can be the slowest part of running Pkl.

Some results from parsing all snippet tests in pkl-core showing ~100x performance improvement:
ANTLR elapsed: 7122ms (7.1s)
New parser elapsed: 78.73ms

This is still a draft, many tests are still failing and repl parsing is still using the old parser.

KushalP
KushalP reviewed Jan 30, 2025
View reviewed changes
pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilderNew.java Outdated

@Override
public ExpressionNode visitAmendsExpr(Amends expr) {
// parentExpr is always New, Amends or Parenthesized. The parser makes sure of it
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this comment, but it would be useful to link through to the method where this behaviour is expected in the parser.

@sgammon sgammon mentioned this pull request Feb 3, 2025
Closed
@sgammon

This comment was marked as resolved.

Sign in to view
stackoverflow and others added 20 commits February 5, 2025 16:58
@stackoverflow
Start new parser
f37a904
@KushalP @stackoverflow
ParserComparisonTest inherits from ParserComparisonTestInterface (#1
724219a 
)

This allows us to create specialisms that implement custom
`getSnippets()` behaviour whilst sharing the core test plumbing.
@stackoverflow
Start parser visitor
d1fc7b9
@stackoverflow
Spotless apply and rebase main
b1be0be
@KushalP @stackoverflow
Assert that compare doesn't throw, and annotate the path with as (#3
6f5520e 
)

This will make it easier to understand which file has the specific `ParserError`.
@KushalP @stackoverflow
Collect all test failures using SoftAssertions (#4)
05c147a
@stackoverflow
Fix bug in parser and lexer
e1fc451
@stackoverflow
Fix bug in operator resolution
0afd454
@stackoverflow
Add support for legacy syntax
95234f8
@stackoverflow
Change spans to be faster
4e02b30
@stackoverflow
Add parent to CST
761c41f
@stackoverflow
Fix CST toString and equals
9277aed
@stackoverflow
finish AstBuilderNew
7b9bae0
@stackoverflow
switch parsers
1deb01c
@stackoverflow
rebase main
3c8fb93
@stackoverflow
improve string parsing
adfd54f
@stackoverflow
check object elements are separated
ef6ccd6
@stackoverflow
fix more parsing errors
fc8d945
@stackoverflow
add children to nodes
b5a4ab7
@stackoverflow
add partial result to parser
7cd101e
bioball
bioball requested changes Feb 7, 2025
View reviewed changes
Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

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

Did another pass!

pkl-core/src/main/java/org/pkl/core/parser/cst/Expr.java Outdated
Comment on lines 487 to 493
public Span getStartDelimiterSpan() {
return startDelimiterSpan;
}

public Span getEndDelimiterSpan() {
return endDelimiterSpan;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These are ununsed. Remove?

Alternatively, maybe add @SuppressWarnings("unused")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning is gone for me after the refactor.

pkl-core/src/main/java/org/pkl/core/parser/cst/Expr.java Outdated

public Span getStartDelimiterSpan() {
return startDelimiterSpan;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unused. Remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

pkl-core/src/main/java/org/pkl/core/parser/cst/ObjectBody.java Outdated
import org.pkl.core.util.Nullable;

public final class ObjectBody implements Node {
private final List<Parameter> parameters;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be consistent and either get rid of org.pkl.core.parser.cst.ParameterList, or make this also a ParameterList.

If we choose to get rid of ParameterList, we should also get rid of TypeParameterList, and ArgumentList.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The *List classes keep around the span of their delimiters which we use for error reporting.
This cannot be a ParameterList because this is not surrounded by parentheses.

pkl-core/src/test/kotlin/org/pkl/core/parser/ParserComparisonTestInterface.kt
import org.pkl.core.parser.antlr.PklParser

@Execution(ExecutionMode.CONCURRENT)
interface ParserComparisonTestInterface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why create an interface? These methods can just go directly into ParserComparisonTest.

pkl-core/src/test/kotlin/org/pkl/core/parser/ParserComparisonTestInterface.kt
interface ParserComparisonTestInterface {
@Test
@Execution(ExecutionMode.CONCURRENT)
fun compareSnippetTests() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this would be a good candidate for @ParameterizedTest.

But, this is temporary code anyways, so, it's not a big deal.

bioball
bioball reviewed Feb 11, 2025
View reviewed changes
Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

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

BTW: we should also remove "Grammar Definition" from the language spec, because after this change, ANTLR is no longer the source of truth. But, we also don't really have another spec to point people at yet.

bioball
bioball reviewed Feb 12, 2025
View reviewed changes
bioball
bioball approved these changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

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

Awesome work!! This is going to be a massive improvement for users.

@stackoverflow stackoverflow merged commit b526902 into apple:main Feb 12, 2025
4 checks passed
@stackoverflow stackoverflow deleted the new-parser branch February 12, 2025 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KushalP KushalP KushalP left review comments

@bioball bioball bioball approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@stackoverflow @sgammon @KushalP @bioball