Skip to content

Conversation

@HT154
Copy link
Contributor

@HT154 HT154 commented Sep 12, 2025

This PR is composed of two commits: the first adds the new functionality and the second regenerates all changed snippet outputs. Please review just the first commit.

Implements SPICE-0021

Resolves #994

@HT154 HT154 force-pushed the binary-renderer branch 4 times, most recently from b7285fb to bee69cc Compare September 12, 2025 23:11
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR extracts String-specific functionality to a new AbstractStringRenderer subclass so that binary renderers can still benefit from the converter logic here in AbstractRenderer. This is a breaking change for any out-of-tree inheritors from AbstractRenderer but the fix is as simple as s/AbstractRenderer/AbstractStringRenderer/g.

@HT154 HT154 force-pushed the binary-renderer branch 6 times, most recently from 3a9b196 to 9b4415b Compare September 17, 2025 22:09
@HT154 HT154 marked this pull request as ready for review September 17, 2025 22:20
@HT154 HT154 changed the title Implement Pkl binary renderer and parser SPICE-0021 Implement Pkl binary renderer and parser Sep 17, 2025
@sin-ack
Copy link

sin-ack commented Oct 2, 2025

Tested this out, and hit an edge case where if the moduleUri for a Typed binary-encoded value is the empty string, the following exception is thrown:

java.lang.NullPointerException

–– Pkl Error ––
None (cause has no message)

42 | external function parse(source: Resource|Bytes, context: Module): Any
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
at pkl.encoding#PklBinaryEncodingParser.parse (https://github.com/apple/pkl/blob/40bcfb18/stdlib/encoding.pkl#L42)

-- (snip) --

Pkl 0.30.0-dev+40bcfb18 (Linux 6.16.3-cachyos, native)

java.lang.NullPointerException
        at org.pkl.core.SecurityManagers.getDefaultTrustLevel(SecurityManagers.java:80)
        at org.pkl.core.SecurityManagers$Standard.checkImportModule(SecurityManagers.java:165)
        at org.pkl.core.stdlib.encoding.PklBinaryEncodingParserNodes$parse$Importer.importModule(PklBinaryEncodingParserNodes.java:126)
        at org.pkl.core.stdlib.encoding.PklBinaryEncodingParserNodes$parse$Importer.importClass(PklBinaryEncodingParserNodes.java:80)
        at org.pkl.core.PklBinaryDecoder.decodeObject(PklBinaryDecoder.java:230)
        at org.pkl.core.PklBinaryDecoder.decodeNonPrimitive(PklBinaryDecoder.java:162)
        at org.pkl.core.PklBinaryDecoder.doDecode(PklBinaryDecoder.java:141)
        at org.pkl.core.PklBinaryDecoder.decode(PklBinaryDecoder.java:109)
        at org.pkl.core.stdlib.encoding.PklBinaryEncodingParserNodes$parse.doParse(PklBinaryEncodingParserNodes.java:62)
        ...

@HT154
Copy link
Contributor Author

HT154 commented Oct 2, 2025

@sin-ack How did you produce the encoded data with the empty module URI string? Was that something you edited by hand?

@sin-ack
Copy link

sin-ack commented Oct 2, 2025

@HT154 It's from outside Pkl itself. We have a custom Pkl binary encoder/Pkl types representation in the host language that previously encoded Dynamics with an empty module URI string (although I'm pretty sure pkl server also returns Dynamics that way, but don't quote me on that). We now use "pkl:base" which works around this.

@HT154
Copy link
Contributor Author

HT154 commented Oct 3, 2025

Gotcha! Both pkl-server's prior implementation and this new one do indeed encode Dynamic's type info with module URI pkl:base and name Dynamic. I'm adding some error handling in place of the unhelpful NPE, but encoding the module URI as an empty string will remain invalid. I'm also improving the binary encoding spec to explicitly document the specifics of type naming for Typed, Dynamic, Class, and TypeAlias values.

@HT154 HT154 force-pushed the binary-renderer branch 9 times, most recently from 616ccd1 to fb54506 Compare October 8, 2025 16:22
Copy link
Member

@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 a first pass!

@HT154 HT154 force-pushed the binary-renderer branch 2 times, most recently from 135ceff to b50d453 Compare October 9, 2025 20:22
@HT154 HT154 force-pushed the binary-renderer branch 3 times, most recently from 25fbab1 to f3b2ad6 Compare October 10, 2025 01:01
@HT154 HT154 requested a review from bioball October 10, 2025 01:08
Copy link
Member

@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.

This is getting pretty close! Made another pass.

@HT154 HT154 requested a review from bioball October 15, 2025 19:04
@HT154 HT154 force-pushed the binary-renderer branch 2 times, most recently from b997b65 to 494f6b1 Compare October 15, 2025 21:20
Copy link
Member

@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.

Some minor comments. LGTM to merge after they're addressed!

@HT154 HT154 force-pushed the binary-renderer branch 2 times, most recently from 5f07d28 to c260192 Compare October 15, 2025 23:03
@HT154
Copy link
Contributor Author

HT154 commented Oct 16, 2025

Merging this now, any additional feedback or issues will be addressed in a follow-up PR

@bioball
Copy link
Member

bioball commented Oct 16, 2025

@sin-ack FYI: we've moved the in-language parser out of this PR because it executes imports. This goes against the principle that module imports should be statically analyzable (which you can get with pkl analyze imports).

We're considering introducing a new language operator instead to load binary data; something like:

data = load("path/to/bytes.msgpack")

Where load becomes a keyword much like read() and import(). But, we need some time to think through this feature and we're punting it out of the SPICE.

@sin-ack
Copy link

sin-ack commented Oct 17, 2025

That's kind of unfortunate, because having it helped with a use-case we had where we were able to preserve type information for user configuration passed with Pkl binary encoding into the module. Without this, I guess an alternative could be to have a client resource reader expose the data at a relative path and use the relative load() as a pseudo-"dynamic" import. Thanks for letting me know.

@bioball bioball merged commit 6c036bf into apple:main Oct 20, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Binary renderer/parser for Pkl

3 participants