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

joshlf · 2024-05-18T16:54:54Z

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

jswrenn · 2024-05-18T17:02:21Z

Cargo.toml

@@ -60,10 +60,11 @@ alloc = []
 derive = ["zerocopy-derive"]
 simd = []
 simd-nightly = ["simd"]
+std-error = ["alloc"]


I kinda think we should just call this feature std, for symmetry with alloc.

jswrenn · 2024-05-18T17:05:11Z

src/error.rs

+impl<Src, Dst: ?Sized> fmt::Display for AlignmentError<Src, Dst>
 where
    Src: Deref,
+    Dst: KnownLayout,


Perhaps we should have this bound on our Display impls for the other error types, too?

To what end? We only need it here because of its use in the body.

@joshlf Improved diagnostics. I went ahead and added a KnownLayout bound to SizeError: https://github.com/google/zerocopy/compare/f8bd5db3e615ba1c93451e2cce6789c73788b2ca..235cd25c1b93f85e90287ada15d5a4fb2d5770c5

I'm sure we can use this bound to provide better diagnostics in the DST case too.

I suspect this may also be considered too much data leak for some users. Size often correlates with content, which in turn can be used to violate confidentiality.

jswrenn · 2024-05-18T17:07:03Z

src/error.rs

@@ -139,14 +149,22 @@ where
        f.write_str("the conversion failed because the address of the source (a multiple of ")?;
        addr_align.fmt(f)?;
        f.write_str(") is not a multiple of the alignment (")?;
-        core::mem::align_of::<Dst>().fmt(f)?;
+        <Dst as KnownLayout>::LAYOUT.align.get().fmt(f)?;


I really like the technique of using KnownLayout to enrich the error messages! We could also apply this to SizeError, I think, to revalidate the size at runtime from src in Display, rather than add another field to SizeError.

Not sure I understand what you're suggesting here.

Incidentally, it turns out that some of the Src: Deref bounds were unnecessary (ie, not actually used), so I removed them.

joshlf · 2024-05-18T17:34:55Z

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.

codecov-commenter · 2024-05-18T18:54:08Z

Codecov Report

Attention: Patch coverage is 70.31250% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 87.55%. Comparing base (1c77a9d) to head (235cd25).

Files	Patch %	Lines
src/error.rs	72.41%	16 Missing ⚠️
src/util.rs	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1298      +/-   ##
==========================================
- Coverage   87.79%   87.55%   -0.24%     
==========================================
  Files          15       15              
  Lines        5171     5224      +53     
==========================================
+ Hits         4540     4574      +34     
- Misses        631      650      +19

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

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. Makes progress on #1297

jswrenn

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.

jswrenn · 2024-05-18T21:24:37Z

src/error.rs

+impl<Src, Dst: ?Sized> fmt::Display for AlignmentError<Src, Dst>
 where
    Src: Deref,
+    Dst: KnownLayout,


@joshlf Improved diagnostics. I went ahead and added a KnownLayout bound to SizeError: https://github.com/google/zerocopy/compare/f8bd5db3e615ba1c93451e2cce6789c73788b2ca..235cd25c1b93f85e90287ada15d5a4fb2d5770c5

I'm sure we can use this bound to provide better diagnostics in the DST case too.

jswrenn · 2024-05-18T21:25:42Z

src/error.rs

@@ -206,17 +224,26 @@ impl<Src, Dst: ?Sized> fmt::Debug for SizeError<Src, Dst> {
 /// Produces a human-readable error message.
 impl<Src, Dst: ?Sized> fmt::Display for SizeError<Src, Dst>
 where
-    Src: Deref,


I think we should keep this bound, too. Once we have size_of_val, we can use this bound to provide information about the length of the source.

What do you mean "once we have size_of_val"? We already do.

(Same concern about leaking size, btw.)

We already do.

D'oh. :-)

I hear your concerns about security in some contexts, but this would be a substantial improvement in the error message in other contexts. Perhaps we could keep it off-by-default, and only display the additional info under a cfg or in alternate formatting mode?

Hmmm yeah I could see that. Maybe a "rich-error-text" feature or something? We could probably gate a lot behind that so we're super anal without it but pretty verbose with it.

joshlf · 2024-05-30T18:53:24Z

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
File an issue to track: have some way of configuring this behavior; 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 this new issue so we know how much demand there is.

joshlf requested a review from jswrenn May 18, 2024 16:54

joshlf enabled auto-merge May 18, 2024 16:55

joshlf force-pushed the error-trait branch from a688425 to 6545c02 Compare May 18, 2024 17:02

jswrenn requested changes May 18, 2024

View reviewed changes

joshlf mentioned this pull request May 18, 2024

Make error types usable in more contexts #1297

Open

joshlf force-pushed the error-trait branch 3 times, most recently from 42e9149 to 650df6a Compare May 18, 2024 17:33

joshlf force-pushed the error-trait branch from 650df6a to 3b786ab Compare May 18, 2024 17:37

joshlf requested a review from jswrenn May 18, 2024 17:37

joshlf force-pushed the error-trait branch 4 times, most recently from 227763c to f8bd5db Compare May 18, 2024 18:38

jswrenn force-pushed the error-trait branch from f8bd5db to 235cd25 Compare May 18, 2024 21:22

jswrenn requested changes May 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joshlf commented May 18, 2024

jswrenn May 18, 2024

joshlf May 18, 2024

jswrenn May 18, 2024

joshlf May 18, 2024

jswrenn May 18, 2024

joshlf May 18, 2024

jswrenn May 18, 2024

joshlf May 18, 2024

joshlf commented May 18, 2024

codecov-commenter commented May 18, 2024 •

edited

jswrenn left a comment •

edited

jswrenn May 18, 2024

jswrenn May 18, 2024

joshlf May 18, 2024

jswrenn May 21, 2024

joshlf May 21, 2024

joshlf commented May 30, 2024

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

Are you sure you want to change the base?

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

Conversation

joshlf commented May 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshlf commented May 18, 2024

codecov-commenter commented May 18, 2024 • edited

Codecov Report

jswrenn left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshlf commented May 30, 2024

codecov-commenter commented May 18, 2024 •

edited

jswrenn left a comment •

edited