Replace ANTLR with hand-rolled parser #917
Conversation
stackoverflow
force-pushed
the
new-parser
branch
from
50f636e to
c9d76f5
Compare
pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilderNew.java
Outdated
Show resolved
Hide resolved
pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilderNew.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public ExpressionNode visitAmendsExpr(Amends expr) { | ||
| // parentExpr is always New, Amends or Parenthesized. The parser makes sure of it |
There was a problem hiding this comment.
I like this comment, but it would be useful to link through to the method where this behaviour is expected in the parser.
pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilderNew.java
Outdated
Show resolved
Hide resolved
pkl-core/src/test/kotlin/org/pkl/core/newparser/ParserComparisonTest.kt
Outdated
Show resolved
Hide resolved
pkl-core/src/test/files/LanguageSnippetTests/input/basic/amendsChains.pkl
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
stackoverflow
force-pushed
the
new-parser
branch
from
3afe063 to
8b440d4
Compare
| children.addAll(pars); | ||
| children.addAll(members); | ||
| return children; | ||
| } |
There was a problem hiding this comment.
Should we make this allocation free?
One option is to just make children a field on the class.
Alternatively, maybe this should instead be visitChildren. In that case, this becomes:
<T> @Nullable T visitChildren(ParserVisitor<T> visitor) {
for (var parameter : parameters) {
parameter.accept(visitor);
}
for (var member : members) {
member.accept(visitor);
}
}There was a problem hiding this comment.
I kept the children() method there but refactored the visitor not call it anymore. So no allocations are being made in our visitors.
There was a problem hiding this comment.
Okay; I think that's fine. But maybe let's add a comment to Node#children() that it allocates memory and to prefer using a visitor.
There was a problem hiding this comment.
I just refactored the ast to store everything in the children's list. Calling children() or using the visitor doesn't allocate anything.
pkl-core/src/main/java/org/pkl/core/parser/AbstractAstBuilder.java
Outdated
Show resolved
Hide resolved
| public Span getStartDelimiterSpan() { | ||
| return startDelimiterSpan; | ||
| } | ||
|
|
||
| public Span getEndDelimiterSpan() { | ||
| return endDelimiterSpan; | ||
| } |
There was a problem hiding this comment.
These are ununsed. Remove?
Alternatively, maybe add @SuppressWarnings("unused")
There was a problem hiding this comment.
The warning is gone for me after the refactor.
|
|
||
| public Span getStartDelimiterSpan() { | ||
| return startDelimiterSpan; | ||
| } |
There was a problem hiding this comment.
Same as above.
| import org.pkl.core.util.Nullable; | ||
|
|
||
| public final class ObjectBody implements Node { | ||
| private final List<Parameter> parameters; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| Unexpected character `{0}`. Did you mean {1}? | ||
|
|
||
| invalidCharacter=\ | ||
| Invalid identifier `{0}`. |
| block comments also | ||
| */ | ||
| /// doc continuation | ||
| foo = 1 |
There was a problem hiding this comment.
We're parsing this incorrectly; change this test to:
import "pkl:reflect"
/// doc comment
// line comments can appear between doc comments
/*
block comments also
*/
/// doc continuation
foo = 1
theComment = reflect.Class(module.getClass()).properties["foo"].docCommentOutput:
foo = 1
theComment = """
doc comment
// line comments can appear between doc comments
/*
block comments also
*/
doc continuation
"""| @@ -0,0 +1,6 @@ | |||
| –– Pkl Error –– | |||
| Invalid token at position. Expected class, typealias, method or property. | |||
There was a problem hiding this comment.
Nit: it would be good to customize error messages for doc comments.
Something like:
Dangling documentation comment.
x | /// doc comment continuation
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Documentation comments must be attached to modules, classes, typealiases, methods, or properties.
| import org.pkl.core.parser.antlr.PklParser | ||
|
|
||
| @Execution(ExecutionMode.CONCURRENT) | ||
| interface ParserComparisonTestInterface { |
There was a problem hiding this comment.
Why create an interface? These methods can just go directly into ParserComparisonTest.
| interface ParserComparisonTestInterface { | ||
| @Test | ||
| @Execution(ExecutionMode.CONCURRENT) | ||
| fun compareSnippetTests() { |
There was a problem hiding this comment.
Nit: this would be a good candidate for @ParameterizedTest.
But, this is temporary code anyways, so, it's not a big deal.
Fixes #906
Fixes #888
Fixes #927
Fixes #931
Fixes #932
Incompatible changes:
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.