Add pkl:syntax stdlib module - #1821
Open
stackoverflow wants to merge 20 commits into
Open
Conversation
HT154
reviewed
Aug 13, 2026
Contributor
There was a problem hiding this comment.
RE #1569 (comment): I'm now wondering if we need to (also?) handle this in SyntaxNodes.convertVmToNode as it's possible to construct generic nodes without proper parens that won't round-trip.
HT154
reviewed
Aug 13, 2026
bioball
reviewed
Aug 14, 2026
bioball
reviewed
Aug 20, 2026
Comment on lines
+169
to
+177
| public abstract static class transform extends ExternalMethod1Node { | ||
| @Specialization | ||
| protected VmTyped eval(VmTyped self, VmFunction operator) { | ||
| // the operator is applied to a child only when that child is read, it is never applied to | ||
| // `self`, so an operator that recurses with `node.transform(operator)` terminates | ||
| return SyntaxNodes.createView( | ||
| new ViewData(self, new SyntaxNodes.OperatorRewriter(operator), null)); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
The transform method basically does this, right?
(node) {
children = node.children
.map((it) ->
let (transformed = transform.apply(it))
(it) { parent = node) }
)
}I feel like this method can be written in a more straightforward way; this still keeps most of the values lazy (text, children), but eagerly sets its own children; no returning new objects with extra storage attached.
This lines up with how our other collection methods work (e.g. List.map)
Suggested change
| public abstract static class transform extends ExternalMethod1Node { | |
| @Specialization | |
| protected VmTyped eval(VmTyped self, VmFunction operator) { | |
| // the operator is applied to a child only when that child is read, it is never applied to | |
| // `self`, so an operator that recurses with `node.transform(operator)` terminates | |
| return SyntaxNodes.createView( | |
| new ViewData(self, new SyntaxNodes.OperatorRewriter(operator), null)); | |
| } | |
| } | |
| public abstract static class transform extends ExternalMethod1Node { | |
| @Specialization | |
| @ExplodeLoop | |
| protected VmObject eval(VmTyped self, VmFunction operator) { | |
| var children = (VmList) VmUtils.readMember(self, Identifier.CHILDREN); | |
| if (children.isEmpty()) { | |
| return self; | |
| } | |
| var newChildren = VmList.EMPTY.builder(); | |
| var wasChanged = false; | |
| for (var child : children) { | |
| var transformed = (VmTyped) operator.apply(child); | |
| if (transformed == child) { | |
| newChildren.add(child); | |
| continue; | |
| } | |
| wasChanged = true; | |
| var transformedWithParent = | |
| new VmObjectBuilder(1).addProperty(Identifier.PARENT, self).buildAmending(transformed); | |
| newChildren.add(transformedWithParent); | |
| } | |
| if (!wasChanged) { | |
| return self; | |
| } | |
| return new VmObjectBuilder(1) | |
| .addProperty(Identifier.CHILDREN, newChildren.build()) | |
| .buildAmending(self); | |
| } | |
| } |
This suggestion implies this method be added to VmObjectBuilder:
public VmObject buildAmending(VmObject parent) {
if (members.isEmpty()) {
return parent;
}
var frame = VmUtils.createEmptyMaterializedFrame();
if (parent instanceof VmTyped vmTyped) {
return new VmTyped(frame, vmTyped, parent.getVmClass(), members);
}
if (parent instanceof VmMapping vmMapping) {
return new VmMapping(frame, vmMapping, members);
}
if (parent instanceof VmListing vmListing) {
return new VmListing(frame, vmListing, members, elementCount);
}
var vmDynamic = (VmDynamic) parent;
return new VmDynamic(frame, vmDynamic, members, elementCount);
}
Comment on lines
+136
to
+146
| VmList children() { | ||
| var childNodes = node.children; | ||
| if (childNodes.isEmpty()) { | ||
| return VmList.EMPTY; | ||
| } | ||
| var children = new Object[childNodes.size()]; | ||
| for (var i = 0; i < children.length; i++) { | ||
| children[i] = createNode(new GenericNodeData(childNodes.get(i), source, sourceUri, selfVm)); | ||
| } | ||
| return VmList.create(children); | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| VmList children() { | |
| var childNodes = node.children; | |
| if (childNodes.isEmpty()) { | |
| return VmList.EMPTY; | |
| } | |
| var children = new Object[childNodes.size()]; | |
| for (var i = 0; i < children.length; i++) { | |
| children[i] = createNode(new GenericNodeData(childNodes.get(i), source, sourceUri, selfVm)); | |
| } | |
| return VmList.create(children); | |
| } | |
| @ExplodeLoop | |
| VmList children() { | |
| var childNodes = node.children; | |
| if (childNodes.isEmpty()) { | |
| return VmList.EMPTY; | |
| } | |
| var builder = VmList.EMPTY.builder(); | |
| for (var childNode : childNodes) { | |
| builder.add(createNode(new GenericNodeData(childNode, source, sourceUri, selfVm))); | |
| } | |
| return builder.build(); | |
| } |
| startColumn, | ||
| endLine, | ||
| 6DB6 endColumn, | ||
| VmContext.get(null).getFrameTransformer()); |
Member
There was a problem hiding this comment.
We should propagate the node into VmContext.get(); this is important because Truffle will optimize away the context lookup when in compiled code.
| endLine, | ||
| endColumn, | ||
| VmContext.get(null).getFrameTransformer()); | ||
| return transformed.equals(sourceUri) ? sourceUri + "#" + fragment : transformed; |
Member
There was a problem hiding this comment.
Thinking about this some more: I actually think we shouldn't apply add a fragment and just leave the URI as-is, because this is the most consistent with the rest of our APIs. Nothing else creates URIs like file:///path/to/foo.pkl#L1-L3.
I think the URI for nodes parsed from text can just be the empty string.
Comment on lines
+66
to
+67
| /// The verbatim source text this node spans, or [null] for a node that was not parsed | ||
| /// from source code. |
Member
There was a problem hiding this comment.
Suggested change
| /// The verbatim source text this node spans, or [null] for a node that was not parsed | |
| /// from source code. | |
| /// The verbatim source text for this node. | |
| /// | |
| /// [FormattingRenderer] only renders this text if this is a [terminal][isTerminal] node. |
|
|
||
| /// Tells whether any parent's [type] is [type]. | ||
| external function hasParentOfType(type: NodeType): Boolean | ||
| } |
Member
There was a problem hiding this comment.
Thought about the method names and signature some more, I'm thinking the signatures should look something like this.
The key differences are are:
- No "where" in method name
- Offer "find" and "findOrNull" variants; where one throws and the other returns
nullif no nodes match the predicate
external function findChild(predicate: (GenericNode) -> Boolean): GenericNode
external function findChildOrNull(predicate: (GenericNode) -> Boolean): GenericNode?
external function findChildOfType(type: NodeType): GenericNode
external function findChildOfTypeOrNull(type: NodeType): GenericNode?
external function replaceChildren(
predicate: (GenericNode) -> Boolean,
replacer: (GenericNode) -> GenericNode,
): GenericNode
external function replaceChildrenOfType(
type: NodeType,
replacer: (GenericNode) -> GenericNode,
): GenericNode
external function findParent(predicate: (GenericNode) -> Boolean): GenericNode
external function findParentOrNull(predicate: (GenericNode) -> Boolean): GenericNode?
external function findParentOfType(type: NodeType): GenericNode
external function findParentOfTypeOrNull(type: NodeType): GenericNode?
HT154
reviewed
Aug 20, 2026
Comment on lines
+63
to
+64
| /// The parent node, or [null] for the root and for nodes not attached to a tree. | ||
| hidden parent: GenericNode? |
Contributor
There was a problem hiding this comment.
What happens if a user tries to set this? It looks readily possible to create invalid trees (well, graphs). Should this be external instead?
| return new Node(nodeTypeForTerminal(tk.token), tk.span); | ||
| } | ||
|
|
||
| private NodeType nodeTypeForTerminal(Token token) { |
Contributor
There was a problem hiding this comment.
This is only ever called from makeTerminal, does it need to be a separate method?
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| at syntaxParseExpression#res (file:///$snippetsDir/input/errors/syntaxParseExpression.pkl) | ||
|
|
||
| Unexpected end of file. at (1:4 - 1:4) |
Contributor
There was a problem hiding this comment.
Thoughts on updating GenericParserError.toString() to detect when the start and end are the same and produce nicer output?
Suggested change
| Unexpected end of file. at (1:4 - 1:4) | |
| Unexpected end of file. at (1:4) |
| static final class OperatorRewriter implements Rewriter { | ||
| private final VmFunction operator; | ||
|
|
||
| OperatorRewriter(VmFunction operator) { |
Contributor
There was a problem hiding this comment.
Should this also accept an apply node?
| private final NodeSet targets; | ||
| private final VmFunction replacer; | ||
|
|
||
| TargetRewriter(NodeSet targets, VmFunction replacer) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #1569.
This PR is just the generic API of the old PR. The typed API will come as a separate PR due to size.