Add `pkl:syntax` module by stackoverflow · Pull Request #1569 · apple/pkl

stackoverflow · 2026-04-30T15:27:10Z

This new stdlib module allows for full manipulation of Pkl source code in Pkl: parsing text to a Pkl CST and formatting a Pkl CST back to text.

odenix · 2026-04-30T17:54:01Z

Have you considered offering the equivalent Java library? Wrapping that library instead of implementing pkl:syntax in Pkl would also improve (especially cold/warm) performance of pkl:syntax.

HT154 · 2026-04-30T18:39:30Z

The major thing missing from this PR is the ability to whole-cloth construct and then format SyntaxNode instances without laboriously creating the underlying Nodes, especially given how common and important code generation is for the Pkl ecosystem. I'm not opposed to offering a Java API (pklpoet?) for this as long as the SyntaxNode classes remain instantiable (not external).

stackoverflow · 2026-05-04T08:10:15Z

The Java library is already Node. We still need the classes to be implemented in Pkl because we don't want to offer a generic API to users. As Jen mentioned, we don't want the classes to be external.

odenix · 2026-05-22T23:21:37Z

As Jen mentioned, we don't want the classes to be external.

As far as I know, external members of non-external classes can still be implemented in Java.

If this stdlib module is implemented in Pkl, it should not be cached (as it is now), as this is very likely to cause multithreading issues.

stackoverflow · 2026-05-27T15:31:12Z

As Jen mentioned, we don't want the classes to be external.

As far as I know, external members of non-external classes can still be implemented in Java.

I don't think there would be much difference performance wise in implementing it in java. All the parsing and formatting is already in java. The Pkl classes are just a sugar layer.

If this stdlib module is implemented in Pkl, it should not be cached (as it is now), as this is very likely to cause multithreading issues.

What do you mean by cached?

HT154 · 2026-04-30T16:11:12Z

+  let (result = parse("x: \(typeSource) = 0"))
+    (result as syntax.ModuleNode).properties.first.typeAnnotation!!.type
+
+examples {


Should these be facts instead since they're all booleans?

HT154 · 2026-05-28T18:07:59Z

+  private static final Identifier TYPE_ID = Identifier.get("type");
+  private static final Identifier CHILDREN_ID = Identifier.get("children");
+  private static final Identifier SPAN_ID = Identifier.get("span");
+  private static final Identifier LINE_START_ID = Identifier.get("lineStart");
+  private static final Identifier COL_START_ID = Identifier.get("colStart");
+  private static final Identifier LINE_END_ID = Identifier.get("lineEnd");
+  private static final Identifier COL_END_ID = Identifier.get("colEnd");


These should probably be members of Identifier with everything else.

HT154 · 2026-05-28T21:21:05Z

+/// Parse the string as a Pkl module, returning either a typed AST node or an error.
+function parse(source: String): ModuleNode | ParserError =
+  let (result = parseNodes(source))
+    if (result is ParserError)
+      result
+    else
+      new ModuleNode { node = result }
+
+/// Parse a resource as a Pkl module, returning either a typed AST node or an error.
+function parseResource(resourceURI: String): ModuleNode | ParserError =
+  parse(read(resourceURI).text)


I've found that it's better for parser APIs to accept Resource directly to avoid confusion when parseResource("./foo.pkl") doesn't work as expected because the read is relative to pkl:syntax (which won't work).

Suggested change

/// Parse the string as a Pkl module, returning either a typed AST node or an error.

function parse(source: String): ModuleNode | ParserError =

let (result = parseNodes(source))

if (result is ParserError)

result

else

new ModuleNode { node = result }

/// Parse a resource as a Pkl module, returning either a typed AST node or an error.

function parseResource(resourceURI: String): ModuleNode | ParserError =

parse(read(resourceURI).text)

/// Parse the string as a Pkl module, returning either a typed AST node or an error.

function parse(source: String | Resource): ModuleNode | ParserError =

let (src = if (source is String) source else source.text)

let (result = parseNodes(source))

if (result is ParserError)

result

else

new ModuleNode { node = result }

(or parseNodes should also accept a Resource as well and parse just passes it through)

HT154 · 2026-05-28T21:31:28Z

+  /// Affix nodes (comments, semicolons) to prepend before this node.
+  prefixes: List<Node>
+
+  /// Affix nodes (comments, semicolons) to append after this node.
+  suffixes: List<Node>


I'm not seeing if/how these properties are used

HT154 · 2026-05-28T21:33:10Z

+  suffixes: List<Node>
+
+  /// Build the typed syntax node.
+  abstract function build(): SyntaxNode


Shouldn't this be a fixed property instead?

Suggested change

abstract function build(): SyntaxNode

abstract fixed result: SyntaxNode

Not drawn to a specific name here. result and syntaxNode both seem reasonable to me.

HT154 · 2026-05-28T21:35:00Z

+    extractStringConstant(sc)
+
+/// Base class for all typed syntax nodes.
+abstract class SyntaxNode {


Why isn't toBuilder()/builder defined on SyntaxNode?

HT154 · 2026-05-28T21:37:36Z

+// Find first child of a given type within a node.
+local const function findChild(n: Node, t: NodeType): Node? =
+  n.children.findOrNull((c) -> c.type == t)
+
+// Find all children of a given type.
+local const function findChildren(n: Node, t: NodeType): List<Node> =
+  n.children.filter((c) -> c.type == t)


Any particular reason not to implement all of these as methods on node. I don't see a reason not to make them public API.

HT154 · 2026-05-28T22:00:42Z

+          children =
+            (if (self.declaration == null) List() else List(self.declaration.build().node))
+              + (
+                if (self.imports.isEmpty)
+                  List()
+                else
+                  List(new Node {
+                    type = "import_list"
+                    children = self.imports.map((i) -> i.build().node)
+                  })
+              )
+              + self.classes.map((c) -> c.build().node)
+              + self.typeAliases.map((t) -> t.build().node)
+              + self.properties.map((p) -> p.build().node)
+              + self.methods.map((m) -> m.build().node)


Nit: style. I ted to find List.filterNonNull makes things like this a little easier to read and maintain.

Suggested change

children =

(if (self.declaration == null) List() else List(self.declaration.build().node))

+ (

if (self.imports.isEmpty)

List()

else

List(new Node {

type = "import_list"

children = self.imports.map((i) -> i.build().node)

})

)

+ self.classes.map((c) -> c.build().node)

+ self.typeAliases.map((t) -> t.build().node)

+ self.properties.map((p) -> p.build().node)

+ self.methods.map((m) -> m.build().node)

List(

self.declaration?.build().node,

if (self.imports.isEmpty)

null

else

new Node {

type = "import_list"

children = self.imports.map((i) -> i.build().node)

},

).filterNonNull()

+ self.classes.map((c) -> c.build().node)

+ self.typeAliases.map((t) -> t.build().node)

+ self.properties.map((p) -> p.build().node)

+ self.methods.map((m) -> m.build().node)

Or better yet, throw together a helper method:

function buildList(items: List): List = items.flatMap((it) -> if (it is List) it else if (it == null) List() else List(it) )

Then

Suggested change

children =

(if (self.declaration == null) List() else List(self.declaration.build().node))

+ (

if (self.imports.isEmpty)

List()

else

List(new Node {

type = "import_list"

children = self.imports.map((i) -> i.build().node)

})

)

+ self.classes.map((c) -> c.build().node)

+ self.typeAliases.map((t) -> t.build().node)

+ self.properties.map((p) -> p.build().node)

+ self.methods.map((m) -> m.build().node)

children =

buildList(List(

self.declaration?.build().node,

if (self.imports.isEmpty)

null

else

new Node {

type = "import_list"

children = self.imports.map((i) -> i.build().node)

},

self.classes.map((c) -> c.build().node),

self.typeAliases.map((t) -> t.build().node),

self.properties.map((p) -> p.build().node),

self.methods.map((m) -> m.build().node),

))

Varargs would make this even nicer, but probably not worth it here since those are special-cased in AstBuilder.

Part of me also wonders if paying the object tax is worth it here for the readability benefit:

Suggested change

children =

(if (self.declaration == null) List() else List(self.declaration.build().node))

+ (

if (self.imports.isEmpty)

List()

else

List(new Node {

type = "import_list"

children = self.imports.map((i) -> i.build().node)

})

)

+ self.classes.map((c) -> c.build().node)

+ self.typeAliases.map((t) -> t.build().node)

+ self.properties.map((p) -> p.build().node)

+ self.methods.map((m) -> m.build().node)

children = new Listing {

when (self.declaration != null) {

self.declaration.build().node

}

when (self.imports.isNotEmpty) {

new Node {

type = "import_list"

children = self.imports.map((i) -> i.build().node)

}

}

...self.classes.map((c) -> c.build().node)

...self.typeAliases.map((t) -> t.build().node)

...self.properties.map((p) -> p.build().node)

...self.methods.map((m) -> m.build().node)

}.toList()

HT154 · 2026-05-28T22:32:25Z

+              + self.classes.map((c) -> c.build().node)
+              + self.typeAliases.map((t) -> t.build().node)
+              + self.properties.map((p) -> p.build().node)
+              + self.methods.map((m) -> m.build().node)


This will necessarily rearrange a module's members, producing all classes, then aliases, then properties, and finally methods. This seems less than ideal from a round-tripping perspective. As an alternative, it might be better to use an approach similar to object members for classes and modules.

HT154 · 2026-05-28T22:59:12Z

+local function formatExpr(builder: syntax.ExprBuilder): String =
+  syntax.format(builder.build().node)
+
+local function formatType(builder: syntax.TypeBuilder): String =
+  syntax.format(builder.build().node)
+
+local function formatBody(builder: syntax.ObjectBodyBuilder): String =
+  syntax.format(builder.build().node)
+
+local function formatModule(builder: syntax.ModuleBuilder): String =
+  syntax.format(builder.build().node)


Seems odd to repeat this verbatim:

Suggested change

local function formatExpr(builder: syntax.ExprBuilder): String =

syntax.format(builder.build().node)

local function formatType(builder: syntax.TypeBuilder): String =

syntax.format(builder.build().node)

local function formatBody(builder: syntax.ObjectBodyBuilder): String =

syntax.format(builder.build().node)

local function formatModule(builder: syntax.ModuleBuilder): String =

syntax.format(builder.build().node)

local function format(builder: syntax.Builder): String =

syntax.format(builder.build().node)

stackoverflow added 3 commits May 27, 2026 17:32

Add syntax package

8382806

Add support for full round-trip of Pkl code

129e02c

Add ast builders

89c02b4

stackoverflow force-pushed the pkl-syntax branch from 7227c15 to 89c02b4 Compare May 27, 2026 15:44

Fix build

56827fa

stackoverflow marked this pull request as ready for review May 27, 2026 15:59

HT154 reviewed May 29, 2026

View reviewed changes

	abstract function build(): SyntaxNode
	abstract fixed result: SyntaxNode

Uh oh!

Conversation

stackoverflow commented Apr 30, 2026

Uh oh!

odenix commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HT154 commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stackoverflow commented May 4, 2026

Uh oh!

odenix commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stackoverflow commented May 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

odenix commented Apr 30, 2026 •

edited

Loading

HT154 commented Apr 30, 2026 •

edited

Loading

odenix commented May 22, 2026 •

edited

Loading