fix: report the missing lsp feature instead of panicking - #6221
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
Self-review — the fix looks right, with two notes.
The dispatch reasoning holds: run's match handles Watch, ListTargets, Format, ShellCompletion, Debug(Ast), Debug(JsonSchema) and now both Lsp arms before the _ => self.run_io_command() catch-all, so the set falling through is exactly Parse | Lex | Collect | Compile | Debug(Annotate|Lineage) | Experimental(GenerateDocs|Highlight) — the same set the extracted io_args() matches, with no arm lost in the merge from the two old matches.
Missing changelog entry. This changes user-facing behaviour of a released binary, and development.md under Contribution workflow → Commits asks for a CHANGELOG.md line for user-facing changes. The Fixes section of [unreleased] is currently empty. Pushing an entry.
Optional cleanup. read_input and write_output both turn io_args()'s None into the same anyhow!("internal error: command does not take input & output") literal. Having io_args() return Result<&mut IoArgs> (bail! in the _ arm) would collapse both call sites to self.io_args()? and leave one copy of the message. Left as-is since the Option accessor is the more honest signature and the duplication is two lines — noting it rather than changing it.
Coverage: the new test does run on PR CI
The description's coverage note names test-rust-main, but that job is gated on if: needs.rules.outputs.main == 'true', so it's SKIPPED here — it only runs post-merge. The job that actually exercises the test on this PR is measure-code-cov, which runs cargo llvm-cov --no-default-features --features=default,test-dbs — no lsp, so the #[cfg(not(feature = "lsp"))] gate is satisfied. Its log carries test cli::test::lsp_without_feature ... ok, and the job is green, so the hand-written snapshot is confirmed by CI rather than only by the local run. Description updated to say so.
codecov/patch is red: of the new lines, the Command::Lsp arm is covered by the test, but the two internal error: command does not take input & output branches aren't — they're unreachable by construction, which is the point of them. codecov isn't among check-ok-to-merge's needs.
lspis an off-by-default feature, but theLspsubcommand was registered unconditionally — so on a default build (cargo install prqlc, the release binaries)prqlc lspfell throughrun's_ => self.run_io_command()catch-all intoread_input, whose match has no arm for it, and panicked onunreachable!(). This adds a#[cfg(not(feature = "lsp"))]arm that returns a plain error instead, and replaces the twounreachable!()s inread_input/write_outputwith a sharedio_args()accessor that returnsOption, so a future command added withoutIoArgserrors rather than panicking. Verified withcargo test -p prqlcandcargo clippy --bin prqlc --all-targets -- -D warnings, each under both default andlspfeature sets.Before:
After:
The variant stays registered rather than being
#[cfg]-gated, so that--helpand the shell-completion snapshots don't vary with the feature set — gating it madecli::test::shell_completionpass only inlspbuilds.Regression test and coverage notes
cli::test::lsp_without_featureis gated on#[cfg(not(feature = "lsp"))]. Against the parent commit it fails with the panic above; with the fix it passes.The
tests→test-rustmatrix runs Linux withdefault,test-dbs-external,lsp, so the new test is skipped there. On a PR it runs inmeasure-code-cov, which uses--no-default-features --features=default,test-dbs— that job's log for this branch carriestest cli::test::lsp_without_feature ... ok. It also runs post-merge intest-rust-mainon macOS (default,test-dbs) and Windows (default) — that job is gated onneeds.rules.outputs.main == 'true', so it's skipped on PRs — and in the localtask prqlc:test/task prqlc:pull-requestloops, which use default features.cargo instaisn't available in the tend sandbox (#6144), so the inline snapshot was written by hand and checked withcargo test.