Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint idea: suggest std Error trait for custom errors #6409

Open
woodruffw opened this issue Dec 1, 2020 · 1 comment
Open

Lint idea: suggest std Error trait for custom errors #6409

woodruffw opened this issue Dec 1, 2020 · 1 comment
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@woodruffw
Copy link

What it does

Libraries regularly implement their own error types, frequently as enum wrappers around dependency-specific errors.

Other libraries like anyhow exist to make error handling in application code simpler, at least for error types that implement std::error::Error.

However, when wrapping/writing custom error types for a library, it's easy to forget to implement std::error::Error. It would be nice to have a clippy lint that detects custom error types and suggests that the user add an impl std::error::Error for MyError {} implementation.

Categories (optional)

Potentially clippy::style.

What is the advantage of the recommended code over the original code

The recommended code transitively simplifies error handling in other codebases by ensuring that the standard Error trait is implemented. This enables more direct use of libraries like anyhow.

Drawbacks

This lint should only be run on crates that are specified as libraries, not as application code (i.e., producing binaries).

It might also be somewhat difficult to determine which types are custom error types. Some ideas:

  • Look for Result<T, X>, where X is a crate-local type
  • Look for types within crate::*::error that match a naming pattern (like *Error*)
  • Look for enum types whose variants' associated datas contain one or more implementations of std::error::Error

Example

pub enum MyError {
  Io(std::io::Error),
  Other(SomeOtherError),
}

Could be written as:

pub enum MyError {
  Io(std::io::Error),
  Other(SomeOtherError),
}

impl std::error::Error for MyError {
  // trait impl...
}
@woodruffw woodruffw added the A-lint Area: New lints label Dec 1, 2020
@giraffate
Copy link
Contributor

A little difference but similar to #1291.

@flip1995 flip1995 added L-suggestion Lint: Improving, adding or fixing lint suggestions E-medium Call for participation: Medium difficulty level problem and requires some initial experience. labels Dec 2, 2020
bors added a commit that referenced this issue Aug 30, 2022
Initial implementation `result_large_err`

This is a shot at #6560, #4652, and #3884. The lint checks for `Result` being returned from functions/methods where the `Err` variant is larger than a configurable threshold (the default of which is 128 bytes). There has been some discussion around this, which I'll try to quickly summarize:

* A large `Err`-variant may force an equally large `Result` if `Err` is actually bigger than `Ok`.
* There is a cost involved in large `Result`, as LLVM may choose to `memcpy` them around above a certain size.
* We usually expect the `Err` variant to be seldomly used, but pay the cost every time.
* `Result` returned from library code has a high chance of bubbling up the call stack, getting stuffed into `MyLibError { IoError(std::io::Error), ParseError(parselib::Error), ...}`, exacerbating the problem.

This PR deliberately does not take into account comparing the `Ok` to the `Err` variant (e.g. a ratio, or one being larger than the other). Rather we choose an absolute threshold for `Err`'s size, above which we warn. The reason for this is that `Err`s probably get `map_err`'ed further up the call stack, and we can't draw conclusions from the ratio at the point where the `Result` is returned. A relative threshold would also be less predictable, while not accounting for the cost of LLVM being forced to generate less efficient code if the `Err`-variant is _large_ in absolute terms.

We lint private functions as well as public functions, as the perf-cost applies to in-crate code as well.

In order to account for type-parameters, I conjured up `fn approx_ty_size`. The function relies on `LateContext::layout_of` to compute the actual size, and in case of failure (e.g. due to generics) tries to come up with an "at least size". In the latter case, the size of obviously wrong, but the inspected size certainly can't be smaller than that. Please give the approach a heavy dose of review, as I'm not actually familiar with the type-system at all (read: I have no idea what I'm doing).

The approach does, however flimsy it is, allow us to successfully lint situations like

```rust
pub union UnionError<T: Copy> {
    _maybe: T,
    _or_perhaps_even: (T, [u8; 512]),
}

// We know `UnionError<T>` will be at least 512 bytes, no matter what `T` is
pub fn param_large_union<T: Copy>() -> Result<(), UnionError<T>> {
    Ok(())
}
```

I've given some refactoring to `functions/result_unit_err.rs` to re-use some bits. This is also the groundwork for #6409

The default threshold is 128 because of #4652 (comment)

`lintcheck` does not trigger this lint for a threshold of 128. It does warn for 64, though.

The suggestion currently is the following, which is just a placeholder for discussion to be had. I did have the computed size in a `span_label`. However, that might cause both ui-tests here and lints elsewhere to become flaky wrt to their output (as the size is platform dependent).

```
error: the `Err`-variant returned via this `Result` is very large
  --> $DIR/result_large_err.rs:36:34
   |
LL | pub fn param_large_error<R>() -> Result<(), (u128, R, FullyDefinedLargeError)> {
   |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ The `Err` variant is unusually large, at least 128 bytes
```

changelog: Add [`result_large_err`] lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

No branches or pull requests

3 participants