-
Notifications
You must be signed in to change notification settings - Fork 8
SPICE-0021: Binary renderer and parser #23
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.
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
base: main
Are you sure you want to change the base?
Conversation
/// Imports are subject to the evaluator's configured allowed modules. | ||
/// | ||
/// Cannot decode [Function] values. | ||
external function parse(source: Resource|Bytes, context: Module): Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. I don't feel that we need this parameter. We should either:
- Use the enclosing module from the caller as "context" (we should be able to do this by special-casing
InvokeMethodVirtualNode
and passing in extra arguments) - Don't run trust level checks
The trust levels concept is a mechanism that is designed to prevent, say, an HTTPS module from importing a file-based module, with no opt-out. If we have this parameter here, I think the most likely outcome is that users will just keep trying to pass in another "context" until the parse
call works.
* | ||
* @return the encoded value | ||
*/ | ||
public Object decode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth clarifying: there's user-facing values (org.pkl.core.PClass
, org.pkl.core.TypeAlias
, etc), and there's in-language values (org.pkl.core.runtime.VmClass
, org.pkl.core.runtime.VmTypeAlias
, etc).
The in-language parser should decode to these VmValues, and the user-facing API should provide the exported value (e.g. PClass
).
We'll probably need two classes; a VmPklBinaryDecoder
(internal) and a PklBinaryDecoder
(user facing).
Also, we should think about how this plays into ConfigEvaluator
and Java/Kotlin codegen.
With codegen, you write this Java code:
try (var ev = ConfigEvaluator.preconfigured()) {
return ev.evaluate(mySource).as(Person.class);
}
How does this work when you are working with pkl-binary?
VmTypeAlias importTypeAlias(String name, URI moduleUri); | ||
} | ||
|
||
public PklBinaryDecoder(MessageUnpacker unpacker, Importer importer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of the binary encoding is so that we can eschew any evaluation. It's kind of strange that the binary decoder would still require an importer.
Also: it should receive either a Buffer
, byte[]
or ByteArrayInputStream
as a source, rather than MessageUnpacker
. Probably good enough to have:
I don't know if parse
needs to be an instance method, should be good enough to make these static, e.g.
public final class PklBinaryDecoder {
private PklBinaryDecoder() {}
public static Object decode(byte[] bytes) {
// impl
}
public static Object decode(ByteArrayInputStream inputStream) {
// impl
}
}
No description provided.