You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Ident's Deserialize impl fed the deserialized Vec<String> straight into Ident::from_path, which does path.pop().unwrap() — so an ident serialized as an empty array panicked instead of erroring. That path is reachable from user input via the public json::to_pl / json::to_rq entry points, and from there through the Python (pl_to_prql, pl_to_rq, rq_to_sql) and JS bindings, which contradicts CLAUDE.md's "never panic on user input".
This adds Ident::try_from_path, returning None on an empty path, and has Deserialize map that to serde::de::Error::invalid_length. from_path keeps its panicking signature for the ~25 internal call sites that build idents from static paths, now via .expect — a compiler-bug invariant rather than a user-input path.
Verified on the end-to-end public API: json::to_pl on a document containing {"Ident":[]} panicked at ident.rs:26 before, and now returns Error: invalid length 0, expected an ident with at least one part at line 1 column 118.
Verification
Regression test deserialize_empty_path in prqlc/prqlc-parser/src/parser/pr/ident.rs fails on the pre-fix code with the panic above.
task prqlc:pull-request could not run in the tend sandbox — cargo insta is not on PATH there, which is what #6144 addresses. The plain cargo test runs above cover the same two crates.
The reason will be displayed to describe this comment to others. Learn more.
One thing missing: this is a user-facing change — json::to_pl / json::to_rq and the Python and JS bindings go from a panic to an error on malformed input, and Ident::try_from_path is new public API — so it needs a CHANGELOG.md line under Fixes per development.md ("Contribution workflow → Commits"). #6221 is the same class of fix and carries one. Pushing the entry to this branch rather than leaving it for a maintainer.
Nothing else. Reviewed as a self-authored PR, so no approval — a maintainer's call.
What was checked
The interesting question is whether from_path's new .expect is a genuine compiler-bug invariant (CLAUDE.md allows .expect only for those) rather than a relocated user-input panic. Walked all of its remaining call sites; each builds a path that is non-empty by construction:
Ident::prepend — parts.extend(self), and an Ident always yields at least its name
parser/expr.rsident() — parts.push(last) immediately before the call
semantic/module.rsfind_main — one arm guarded by !path.is_empty(), the other pushes NS_MAIN
semantic/resolver/stmt.rs — self.current_module_path.push(ident.name) immediately before
semantic/lowering.rs — the relative path is guarded by relative_to_database.is_empty(); database_module_path defaults to [NS_DEFAULT_DB] in semantic/mod.rs
the rest are static literals (vec!["std", "select"] and similar)
Also checked for sibling panics on the same deserialization path: Span is the only other hand-written Deserialize in the workspace, and it already returns de::Error::custom rather than panicking, so Ident was the outlier.
cargo test -p prqlc-parser --lib — 101 passed, 0 failed, including the two new tests. That covers the parser crate only; the tests matrix is green on 13fcfa38 for the rest.
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
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.
Ident'sDeserializeimpl fed the deserializedVec<String>straight intoIdent::from_path, which doespath.pop().unwrap()— so an ident serialized as an empty array panicked instead of erroring. That path is reachable from user input via the publicjson::to_pl/json::to_rqentry points, and from there through the Python (pl_to_prql,pl_to_rq,rq_to_sql) and JS bindings, which contradicts CLAUDE.md's "never panic on user input".This adds
Ident::try_from_path, returningNoneon an empty path, and hasDeserializemap that toserde::de::Error::invalid_length.from_pathkeeps its panicking signature for the ~25 internal call sites that build idents from static paths, now via.expect— a compiler-bug invariant rather than a user-input path.Verified on the end-to-end public API:
json::to_plon a document containing{"Ident":[]}panicked atident.rs:26before, and now returnsError: invalid length 0, expected an ident with at least one part at line 1 column 118.Verification
Regression test
deserialize_empty_pathinprqlc/prqlc-parser/src/parser/pr/ident.rsfails on the pre-fix code with the panic above.task prqlc:pull-requestcould not run in the tend sandbox —cargo instais not on PATH there, which is what #6144 addresses. The plaincargo testruns above cover the same two crates.