Skip to content

[error] Implement std::error::Error on errors#1298

Merged
joshlf merged 1 commit intomainfrom
error-trait
Jun 26, 2024
Merged

[error] Implement std::error::Error on errors#1298
joshlf merged 1 commit intomainfrom
error-trait

Conversation

@joshlf
Copy link
Member

@joshlf joshlf commented May 18, 2024

While we're here, also relax Dispaly for AlignmentError<Src, Dst> to permit Dst: ?Sized in exchange for Dst: KnownLayout. This is an important relaxation since our APIs permit performing conversions into unsized destination types with runtime alignment checking.

Makes progress on #1297

@joshlf joshlf requested a review from jswrenn May 18, 2024 16:54
@joshlf joshlf enabled auto-merge May 18, 2024 16:55
jswrenn
jswrenn requested changes May 18, 2024
View reviewed changes
@joshlf joshlf mentioned this pull request May 18, 2024
Closed
@joshlf joshlf force-pushed the error-trait branch 3 times, most recently from 42e9149 to 650df6a Compare May 18, 2024 17:33
@joshlf
Copy link
Member Author

joshlf commented May 18, 2024

I added a SendSyncPhantomData type and tests to make sure that the errors are Send + Sync only dependent upon Src: Send + Sync, but regardless of Dst, which only exists at the type level.

@joshlf joshlf requested a review from jswrenn May 18, 2024 17:37
@joshlf joshlf force-pushed the error-trait branch 4 times, most recently from 227763c to f8bd5db Compare May 18, 2024 18:38
@codecov-commenter
Copy link

codecov-commenter commented May 18, 2024 •
edited
Loading

Codecov Report

❌ Patch coverage is 86.62791% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.31%. Comparing base (1bf5d4a) to head (fdde58e).
⚠️ Report is 472 commits behind head on main.

Files with missing lines Patch % Lines
src/error.rs 87.95% 20 Missing ⚠️
src/util.rs 50.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1298      +/-   ##
==========================================
- Coverage   87.42%   87.31%   -0.11%     
==========================================
  Files          15       15              
  Lines        5152     5289     +137     
==========================================
+ Hits         4504     4618     +114     
- Misses        648      671      +23

☔ View full report in Codecov by Sentry.
📢 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.

jswrenn
jswrenn requested changes May 18, 2024
View reviewed changes
Copy link
Collaborator

@jswrenn jswrenn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I went ahead and committed an example of how having a KnownLayout bound can be used to improve our diagnostics. Generally speaking, I think we should consistently have Src: Deref and Dst: ?Sized + KnownLayout on our Display and Error impls. We can always relax the bounds later.

I can't think, right now, of how it might be useful for ValidityError, but who knows — depending on how we evolve that trait, an opportunity might arise.

@joshlf
Copy link
Member Author

joshlf commented May 30, 2024 •
edited
Loading

Discussed offline with @jswrenn. Here's the plan:

  • Debug impl is always verbose
  • Conditional on debug_assertions:
    • On: Display impls are verbose (assume that this is in a non-prod context where there's no concern of data leak
    • Off: Display impls attempt to leak as little information as is reasonable
  • Have some way of configuring this behavior (Permit configuring the verbosity of error type Display impls #1457); I can see two use cases:
    • Users enable debug_assertions in prod
    • Users want the output of Display to be stable regardless of debug_assertions
  • Document this behavior at the crate root. Suggest that users who need a way of changing it chime in on Permit configuring the verbosity of error type Display impls #1457 so we know how much demand there is.

@joshlf joshlf mentioned this pull request Jun 24, 2024
Open
@joshlf joshlf force-pushed the error-trait branch 3 times, most recently from 9155b18 to bd81c69 Compare June 24, 2024 19:54
@joshlf joshlf requested a review from jswrenn June 24, 2024 19:58
@joshlf @jswrenn
[error] Implement std::error::Error on errors
fdde58e 
While we're here, also relax `Dispaly for AlignmentError<Src, Dst>` to
permit `Dst: ?Sized` in exchange for `Dst: KnownLayout`. This is an
important relaxation since our APIs permit performing conversions into
unsized destination types with runtime alignment checking.

Also make all errors `Send + Sync` regardless of `Dst`, which only
exists at the type level, but is never instantiated.

Finally, make `Display`'s verbosity conditional on `debug_assertions`.

Makes progress on #1297

Co-authored-by: John Wrenn <jswrenn@amazon.com>
@joshlf joshlf added this pull request to the merge queue Jun 26, 2024
Merged via the queue into main with commit 7dab322 Jun 26, 2024
@joshlf joshlf deleted the error-trait branch June 26, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jswrenn jswrenn jswrenn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@joshlf @codecov-commenter @jswrenn