Skip to content

feat: improve error printing#517

Open
m4tx wants to merge 2 commits into
masterfrom
improve-errors
Open

feat: improve error printing#517
m4tx wants to merge 2 commits into
masterfrom
improve-errors

Conversation

@m4tx
Copy link
Copy Markdown
Member

@m4tx m4tx commented Mar 22, 2026

When using Result::expect(), (e.g. in the default cot::main) errors are printed using Debug printer. This causes very ugly errors to be produced. Instead, when the alternate mode is not being used, just format the error nicely, along with the error source chain.

This roughly duplicates the anyhow::Error error printing logic: https://docs.rs/anyhow/latest/anyhow/struct.Error.html#method.chain

The change turns this:
before

into this:
after

Copilot AI review requested due to automatic review settings March 22, 2026 15:27
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates cot-core::Error’s Debug formatting so that {:?} produces a human-friendly error message plus its source chain (similar to anyhow), while {:#?} retains the original structured debug output. This improves the default panic output from Result::unwrap() / expect() in cot::main.

Changes:

  • Implement non-alternate Debug formatting to print the error via Display plus a “caused by” source chain.
  • Preserve the prior Debug output behind alternate formatting ({:#?}).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cot-core/src/error/error_impl.rs Outdated
Comment thread cot-core/src/error/error_impl.rs Outdated
Comment thread cot-core/src/error/error_impl.rs
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 22, 2026

🐰 Bencher Report

Branchimprove-errors
Testbedgithub-ubuntu-latest
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
microseconds (µs)
(Result Δ%)
Upper Boundary
microseconds (µs)
(Limit %)
empty_router/empty_router📈 view plot
🚷 view threshold
6,480.70 µs
(+6.36%)Baseline: 6,093.20 µs
7,677.76 µs
(84.41%)
json_api/json_api📈 view plot
🚷 view threshold
1,053.70 µs
(-1.92%)Baseline: 1,074.37 µs
1,323.65 µs
(79.61%)
nested_routers/nested_routers📈 view plot
🚷 view threshold
971.58 µs
(-2.09%)Baseline: 992.35 µs
1,204.08 µs
(80.69%)
single_root_route/single_root_route📈 view plot
🚷 view threshold
939.38 µs
(-1.62%)Baseline: 954.82 µs
1,167.50 µs
(80.46%)
single_root_route_burst/single_root_route_burst📈 view plot
🚷 view threshold
16,969.00 µs
(-4.26%)Baseline: 17,724.56 µs
21,190.21 µs
(80.08%)
🐰 View full continuous benchmarking report in Bencher

Comment thread cot-core/src/error/error_impl.rs Outdated
Comment on lines +182 to +184
writeln!(f)?;
writeln!(f)?;
writeln!(f, "caused by:")?;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should be within the while loop to show the chain of causality

Copy link
Copy Markdown
Member Author

@m4tx m4tx Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Rust stack traces are printed with 0:, 1:, 2:, etc. at the beginning of each frame. Maybe that would do? I don't think I'd like to inflate the error messages with excessive newlines.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 22, 2026

Codecov Report

❌ Patch coverage is 87.80488% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cot-core/src/error/error_impl.rs 87.80% 0 Missing and 5 partials ⚠️
Flag Coverage Δ
rust 90.31% <87.80%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cot-core/src/error/error_impl.rs 95.60% <87.80%> (-0.88%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@seqre seqre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make cargo insta test for this functionality

When using `Result::expect()`, (e.g. in the default `cot::main`)
errors are printed using `Debug` printer. This causes very ugly
errors to be produced. Instead, when the alternate mode is not being
used, just format the error nicely, along with the error source chain.

This roughly duplicates the `anyhow::Error` error printing logic:
https://docs.rs/anyhow/latest/anyhow/struct.Error.html#method.chain
@m4tx m4tx force-pushed the improve-errors branch from add338d to 0eaa5ff Compare May 18, 2026 16:53
@github-actions github-actions Bot added C-core A-deps Area: Dependencies labels May 18, 2026
@m4tx m4tx force-pushed the improve-errors branch from f46a88f to 4fdec56 Compare May 18, 2026 19:33
@m4tx m4tx requested review from a team, Copilot and seqre May 18, 2026 19:34
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.

Comment on lines +180 to +183
let error = self.inner();
Display::fmt(error, f)?;

if let Some(source) = error.source() {
Comment on lines +177 to +178
if f.alternate() {
Debug::fmt(&self.repr, f)
Comment on lines +187 to +193
let mut source = Some(source);
let mut i = 0;
while let Some(e) = source {
writeln!(f, "{i:4}: {e}")?;

source = e.source();
i += 1;
Comment on lines +175 to +176
// `Debug` formatting normally. If not, (which is the case when using
// `Result::unwrap()` or `Result::expect()`) pretty print the error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-deps Area: Dependencies C-core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants