interpreter: load data files with an explicit ?format= override - #1429
Open
daandemeyer wants to merge 1 commit into
Open
interpreter: load data files with an explicit ?format= override#1429daandemeyer wants to merge 1 commit into
?format= override#1429daandemeyer wants to merge 1 commit into
Conversation
Loading static data with `load()` required the file to be named `.json` or
`.toml`, because the extension was the only thing selecting a parser. That rules
out the files people actually want to read: `Cargo.lock` is TOML, a
`package.json.tmpl` is JSON, and neither can be renamed.
Add a `?format=` query on the load path (`load(":deps.lock?format=toml", ...)`)
accepting `bzl`, `json`, or `toml`. It cannot be a `load()` kwarg: `format =
"toml"` is already valid syntax meaning "alias the local name `format` to the
exported symbol `toml`", so taking that spelling would need a grammar change and
would silently change the meaning of existing code. The `?` was already reserved
by `ImportPath`.
The format is stored on `ImportPath` and participates in its hash and equality,
so it is part of the DICE key rather than a side channel. That makes the loaded
file a tracked dependency of the parse and gets invalidation for free, and the
extension check is skipped exactly when a format was given. Every site that
dispatched on the extension now asks `ImportPath::load_format()` instead.
A format that merely restates the extension collapses to `None`, so
`:a.bzl?format=bzl` and `:a.bzl` stay one module. Otherwise the same file would
be evaluated under two keys and hand out two copies of its providers and
transitive sets, breaking the pointer-identity checks that `resolve_load`
already warns about.
An unparseable file stays a load-time error, which fails the whole package.
That is inherent: a parse that is not at load time is not a tracked dependency.
Since the extension no longer implies the parser, the error now names the format
and where it came from. The blast radius is documented, with a pointer to
reading the file in an action when failure needs to be scoped to its consumers.
Incidentally fixes prelude `.toml` files being evaluated as Starlark: the
prelude branch special-cased only `.json`, and now shares the format dispatch.
Covered by unit tests for format inference, the relaxed extension check, the
redundant-format collapse and display round-tripping, plus e2e tests for the
three formats, invalidation on edit, and each error path.
Signed-off-by: Daan De Meyer <daan@amutable.com>
Contributor
|
This pull request has been imported. If you are a Meta employee, you can view this in D114348139. (Because this pull request was imported automatically, there will not be any future comments.) |
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
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.
Loading static data with
load()required the file to be named.jsonor.toml, because the extension was the only thing selecting a parser. That rulesout the files people actually want to read:
Cargo.lockis TOML, apackage.json.tmplis JSON, and neither can be renamed.Add a
?format=query on the load path (load(":deps.lock?format=toml", ...))accepting
bzl,json, ortoml. It cannot be aload()kwarg:format = "toml"is already valid syntax meaning "alias the local nameformatto theexported symbol
toml", so taking that spelling would need a grammar change andwould silently change the meaning of existing code. The
?was already reservedby
ImportPath.The format is stored on
ImportPathand participates in its hash and equality,so it is part of the DICE key rather than a side channel. That makes the loaded
file a tracked dependency of the parse and gets invalidation for free, and the
extension check is skipped exactly when a format was given. Every site that
dispatched on the extension now asks
ImportPath::load_format()instead.A format that merely restates the extension collapses to
None, so:a.bzl?format=bzland:a.bzlstay one module. Otherwise the same file wouldbe evaluated under two keys and hand out two copies of its providers and
transitive sets, breaking the pointer-identity checks that
resolve_loadalready warns about.
An unparseable file stays a load-time error, which fails the whole package.
That is inherent: a parse that is not at load time is not a tracked dependency.
Since the extension no longer implies the parser, the error now names the format
and where it came from. The blast radius is documented, with a pointer to
reading the file in an action when failure needs to be scoped to its consumers.
Incidentally fixes prelude
.tomlfiles being evaluated as Starlark: theprelude branch special-cased only
.json, and now shares the format dispatch.Covered by unit tests for format inference, the relaxed extension check, the
redundant-format collapse and display round-tripping, plus e2e tests for the
three formats, invalidation on edit, and each error path.
Signed-off-by: Daan De Meyer daan@amutable.com