Skip to content

fix: return an error instead of panicking on an empty ident path - #6223

Merged
max-sixty merged 2 commits into
mainfrom
fix/ident-deserialize-empty-path
Aug 23, 2026
Merged

fix: return an error instead of panicking on an empty ident path#6223
max-sixty merged 2 commits into
mainfrom
fix/ident-deserialize-empty-path

Conversation

@prql-bot

Copy link
Copy Markdown
Collaborator

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.

cargo test -p prqlc-parser -p prqlc     # 701 passed, 0 failed
cargo clippy -p prqlc-parser --all-targets -- -D warnings   # clean
cargo fmt --check -p prqlc-parser       # clean

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.

@prql-bot prql-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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::prependparts.extend(self), and an Ident always yields at least its name
  • parser/expr.rs ident()parts.push(last) immediately before the call
  • semantic/module.rs find_main — one arm guarded by !path.is_empty(), the other pushes NS_MAIN
  • semantic/resolver/stmt.rsself.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.

@max-sixty
max-sixty merged commit 4b05af7 into main Aug 23, 2026
37 checks passed
@max-sixty
max-sixty deleted the fix/ident-deserialize-empty-path branch August 23, 2026 19:10
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.

2 participants