Return errors instead of panicking for reachable failures by yihozhang · Pull Request #920 · egraphs-good/egglog

yihozhang · 2026-06-05T19:14:32Z

Summary

Audits ./src for panic!/unwrap/expect/todo!/unreachable! sites that are reachable from user input and converts them into proper recoverable errors, so malformed or edge-case programs report an error instead of aborting the process. This addresses a recurring class of bug reports where egglog panics instead of returning an error.

Of 324 panic-family sites in ./src, an audit classified 52 as reachable (MUST_FIX), 258 as type-checker-guaranteed invariants, and the rest as tests/needs-review. All reachable sites are fixed (plus ~12 more found during implementation); invariants are intentionally left.

What now returns an error instead of panicking

Malformed sort declarations → TypeError::BadPresortArguments: (sort S (Vec)), (sort S (UnstableFn)), (sort S (Pair i64)), etc. A real span is threaded through Presort::make_sort.
Partial primitives (return no result instead of crashing): out-of-bounds/negative vec-set/vec-remove, empty multiset-pick, count overflow in multiset ops, and the numeric primitives bigint <</>>, bigrat, log, cbrt, log2.
Command execution → Error: negative extract variant counts, duplicate rule names, bad input/print-function files & column types, output/extract of deleted/subsumed values.
unstable-fn: threaded Result through prim/query/resolve_function_container_target_with_context so unknown, non-literal, or mis-typed targets error instead of panic.
Parsing/desugar: (fail (include ...)), (fail (datatype*)), subsuming a non-call rewrite, and multi-command fail.
Proofs: new ProveExistsError variants; running prove/prove-exists without proofs enabled; graceful re-parse errors in the term-encoding chain.
CLI / scheduler / serialize: CLI write/path failures log and exit cleanly; scheduler no-free-var and unknown-ruleset edge cases no longer panic; serializing a sort whose name contains - (e.g. Math-Expr) no longer panics and round-trips correctly.

Intentionally left (genuine invariants)

Type-checker-guaranteed get_val/base_values().unwrap in primitives, post-typecheck resolution in core.rs, documented Rust-API preconditions (Match::get_value, empty-vars query), a wasm-only thread-count panic, and compile-time proc-macro guards.

Breaking change

EGraph::print_function now takes its output sink as Option<(File, PathBuf)> plus a Span, so write failures return Error::IoError instead of panicking.

Testing

Added regression tests (duplicate rule name, negative extract, missing input file, scheduler edge cases, dashed-sort serialization).
Verified 33 confirmed reproductions now error gracefully instead of crashing.
make test passes (958 tests); make nits passes (clippy -D warnings, fmt, doc).

🤖 Generated with Claude Code

Audit the egglog crate (./src) for panic!/unwrap/expect/todo!/unreachable! sites reachable from user input and convert them into recoverable errors so malformed or edge-case programs report an error instead of aborting. Highlights: - Malformed sort-constructor declarations (e.g. `(sort S (Vec))`, `(sort S (UnstableFn))`) now return `TypeError::BadPresortArguments`; a real span is threaded through `Presort::make_sort`. - Several primitives became partial (return no result instead of panicking): out-of-bounds/negative `vec-set`/`vec-remove`, empty `multiset-pick`, count overflow in multiset ops, and `bigint <<`/`>>`, `bigrat`, `log`, `cbrt`, `log2`. - Command execution now returns `Error` for negative `extract` counts, duplicate rule names, bad `input`/`print-function` files and types, and extraction of deleted/subsumed values. - `unstable-fn` resolution is threaded through `prim`/`query` so unknown, non-literal, or mis-typed targets error instead of panicking. - Parsing/desugar errors for `(fail (include ...))`, `(fail (datatype*))`, subsuming a non-call rewrite, and multi-command `fail`. - Proof subsystem gains `ProveExistsError` variants and graceful errors for running `prove`/`prove-exists` without proofs and for re-parsing edge cases. - CLI write/path failures log and exit cleanly; scheduler no-free-var and unknown-ruleset edge cases no longer panic; serializing a sort whose name contains `-` no longer panics and round-trips correctly. Breaking: `EGraph::print_function` now takes `Option<(File, PathBuf)>` plus a `Span` so write failures return `Error::IoError`. Adds regression tests; `make test` (958) and `make nits` pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

codecov-commenter · 2026-06-05T19:25:06Z

Codecov Report

❌ Patch coverage is 75.48544% with 101 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.17%. Comparing base (f0da95b) to head (a66a423).

Files with missing lines	Patch %	Lines
src/cli.rs	0.00%	33 Missing ⚠️
src/lib.rs	77.77%	28 Missing ⚠️
src/proofs/proof_extraction.rs	48.00%	13 Missing ⚠️
src/extract.rs	27.27%	8 Missing ⚠️
src/proofs/proof_encoding.rs	86.00%	7 Missing ⚠️
src/sort/fn.rs	83.33%	4 Missing ⚠️
src/ast/parse.rs	0.00%	3 Missing ⚠️
src/ast/desugar.rs	92.00%	2 Missing ⚠️
src/sort/multiset.rs	84.61%	2 Missing ⚠️
src/scheduler.rs	98.24%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #920      +/-   ##
==========================================
+ Coverage   87.07%   87.17%   +0.10%     
==========================================
  Files          88       88              
  Lines       26282    26473     +191     
==========================================
+ Hits        22885    23079     +194     
+ Misses       3397     3394       -3

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codspeed-hq · 2026-06-05T19:25:16Z

Merging this PR will not alter performance

✅ 26 untouched benchmarks
⏩ 216 skipped benchmarks¹

_{Comparing no-panic (a66a423) with main (f0da95b)}

216 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Port the ad-hoc verification harness into tests/no_panic.rs: a table-driven suite covering ~32 previously-panicking reproductions (malformed sort declarations, partial primitives, command-execution failures, unstable-fn resolution, desugar/parse errors, multiset index with a primitive function, prove-exists without proofs, and term-encoding of escaped strings). Each case returns a recoverable error or runs to completion; a real panic would abort the test binary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

saulshanabrook · 2026-06-17T15:40:22Z

This issue (and the comment on it) has a few places where we panic instead or raising errors. I had them included in other PRs and took them out to make reviewing them simpler. If you feel like handling those cases here feel free! #898

yihozhang · 2026-06-17T15:46:25Z

                    if !row.subsumed {
                        let target = &row.vals[output_idx];
-                        if *target == canonical_value {
-                            let cost = self.compute_cost_hyperedge(egraph, &row, func).unwrap();


Can this ever fail?

Yes, reachably. compute_cost_hyperedge returns None when a variant has a child e-class with no finite extraction (e.g. a purely cyclic child). The old .unwrap() was a latent panic; such a variant can never be part of a minimal extraction, so skipping it is correct and the target e-class still extracts via its costed variants. Kept the change and added a comment explaining why.

- Treat internally generated term-encoding programs as invariants again (revert the Result threading in proof_encoding{,_helpers}.rs to panics), and fix the actual reason they could fail to parse: `Display for Literal` now escapes `"` and `\` so strings round-trip through the lexer. - Revert bigrat `log`/`cbrt` of a non-trivial value back to `todo!`: `None` would wrongly claim e.g. `cbrt 8/1` has no rational root. - extract: keep skipping variants with no finite cost (reachable for cyclic children); document why. - scheduler: variable-free rules now collect a unit marker per match so they correctly apply their actions when scheduled, instead of silently doing nothing (or dividing by zero). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

oflatt

just reviewed proof related stuff

Following the same principle as the proof-encoding parse helpers, stop dressing up internal proof-machinery invariants as recoverable errors in `prove_exists`. Revert `ConstructorNotDeclared`, `NoProofRecorded`, `ProofTermExtractionFailed`, `NotProvableByRule`, and `ProofCheck` back to panics (they only fire if the proof code itself is wrong) and drop them from `ProveExistsError`. The one genuinely user-reachable case stays: a missing proof-function annotation means proofs were never enabled for the sort, surfaced as `ProveExistsError::ProofsNotEnabled`. That lookup is the correct gate (it is also satisfied when re-running already-lowered programs, where the `proofs_enabled` flag is false but the proof tables exist), so the check lives here rather than as an up-front `proofs_enabled` guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

yihozhang requested a review from a team as a code owner June 5, 2026 19:14

yihozhang requested review from oflatt and removed request for a team June 5, 2026 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Return errors instead of panicking for reachable failures#920

Return errors instead of panicking for reachable failures#920
yihozhang wants to merge 4 commits into
mainfrom
no-panic

yihozhang commented Jun 5, 2026

Uh oh!

codecov-commenter commented Jun 5, 2026 •

edited

Loading

Uh oh!

codspeed-hq Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

saulshanabrook commented Jun 17, 2026

Uh oh!

yihozhang Jun 17, 2026

Uh oh!

yihozhang Jun 17, 2026

Uh oh!

Uh oh!

oflatt left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

yihozhang commented Jun 5, 2026

Summary

What now returns an error instead of panicking

Intentionally left (genuine invariants)

Breaking change

Testing

Uh oh!

codecov-commenter commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codspeed-hq Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

saulshanabrook commented Jun 17, 2026

Uh oh!

yihozhang Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

yihozhang Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oflatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jun 5, 2026 •

edited

Loading

codspeed-hq Bot commented Jun 5, 2026 •

edited

Loading