[check_result_unit_err]: Disable on no_std

[FawazTirmizi]

This lint suggests returning a type implementing the trait std::error::Error (doc) for the Result, but this trait is not available in no_std crates, so it's not possible for them to fix this lint.

This is my first time working in Clippy's codebase, so I may have missed something. Please let me know and I'll fix it.

Example:

Running Clippy on a project with the following lib.rs:

#![no_std]

use core::result::Result;

pub fn ret_err() -> Result<(),()> {
    if true { Ok(()) } else { Err(()) }
}

will yield the following warning:

warning: this returns a `Result<_, ()>`
 --> src/lib.rs:6:1
  |
6 | pub fn ret_err() -> Result<(),()> {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: use a custom `Error` type instead
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_unit_err
  = note: `#[warn(clippy::result_unit_err)]` on by default

After the fix, Clippy passes.

---

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: [check_result_unit_err]: No longer warns on no_std projects

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @giraffate (or someone else) soon.

Please see the contribution instructions for more information.

[PatchMixolydic]

In my eyes, the lint as implemented seems to be correct, even on no_std, but the rationale given by the lint documentation is wrong. On one hand, () is almost never useful as a Result's E type. On the other hand, this lint doesn't actually seem to target using types that don't implement Error at all. For example, this snippet doesn't trip the lint even though MyError doesn't implement Error (playground):

#![no_std]

use core::result::Result;

pub struct MyError;

pub fn ret_err() -> Result<(), MyError> {
    if true { Ok(()) } else { Err(MyError) }
}

In my opinion, since MyError is a local type, this should result in a different lint proclaiming that MyError should implement Error (missing_error_impls?), as changing result_unit_err to lint on the use of any E: !Error in Result could produce unresolveable lints when working with error types from other crates. Because of this, result_unit_err's docs/suggestion should be modified to remove references to the Error trait.

As an aside, it's probably worth noting that on nightly, the Error trait is available in core::error::Error with the error_in_core feature gate.

[FawazTirmizi]

In my eyes, the lint as implemented seems to be correct, even on no_std, but the rationale given by the lint documentation is wrong. On one hand, () is almost never useful as a Result's E type. On the other hand, this lint doesn't actually seem to target using types that don't implement Error at all. ... In my opinion, since MyError is a local type, this should result in a different lint proclaiming that MyError should implement Error (missing_error_impls?) ...

This makes sense. Is this something you'd also like to see in this PR?

... changing result_unit_err to lint on the use of any E: !Error in Result could produce unresolveable lints when working with error types from other crates. Because of this, result_unit_err's docs/suggestion should be modified to remove references to the Error trait.

So are you saying that result_unit_err should instead suggest returning a Result where E != ()?

As an aside, it's probably worth noting that on nightly, the Error trait is available in core::error::Error with the error_in_core feature gate.

Thanks, I was unaware of this.

[PatchMixolydic]

This makes sense. Is this something you'd also like to see in this PR?

I should probably emphasize that I'm just a bystander, not a Clippy maintainer 😅 Nonetheless, I feel that a missing_error_impls lint would be big enough to warrant a separate PR.

So are you saying that result_unit_err should instead suggest returning a Result where E != ()?

Technically, it already does, though perhaps the suggestion could be reworded slightly:

  warning: this returns a `Result<_, ()>`
   --> src/lib.rs:6:1
    |
  6 | pub fn ret_err() -> Result<(),()> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: use a custom `Error` type instead
+   = help: use a custom error type instead
    = help: for further information visit https://rust-lang.github.io/rust- 
 clippy/master/index.html#result_unit_err
    = note: `#[warn(clippy::result_unit_err)]` on by default

I feel that the primary issue is that the documentation focuses on the Error trait when the lint doesn't really have much to do with it:

rust-clippy/clippy_lints/src/functions/mod.rs

Lines 171 to 178 in 09e4659

	/// ### What it does
	/// Checks for public functions that return a `Result`
	/// with an `Err` type of `()`. It suggests using a custom type that
	/// implements `std::error::Error`.
	///
	/// ### Why is this bad?
	/// Unit does not implement `Error` and carries no
	/// further information about what went wrong.

Ideally, this would be reworded to discuss how a custom error type, especially an enum, could allow one to provide more information about why the error occurred. The fact that a custom error type can implement Error is certainly useful, but probably shouldn't be highlighted as the focus of this lint.

[FawazTirmizi]

This makes sense. Is this something you'd also like to see in this PR?

I should probably emphasize that I'm just a bystander, not a Clippy maintainer sweat_smile ...

oops :P

Nonetheless, I feel that a missing_error_impls lint would be big enough to warrant a separate PR.

Agreed.

So are you saying that result_unit_err should instead suggest returning a Result where E != ()?

Technically, it already does, though perhaps the suggestion could be reworded slightly:

  warning: this returns a `Result<_, ()>`
   --> src/lib.rs:6:1
    |
  6 | pub fn ret_err() -> Result<(),()> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-   = help: use a custom `Error` type instead
+   = help: use a custom error type instead
    = help: for further information visit https://rust-lang.github.io/rust- 
 clippy/master/index.html#result_unit_err
    = note: `#[warn(clippy::result_unit_err)]` on by default

I feel that the primary issue is that the documentation focuses on the Error trait when the lint doesn't really have much to do with it:

rust-clippy/clippy_lints/src/functions/mod.rs

Lines 171 to 178 in 09e4659

	/// ### What it does
	/// Checks for public functions that return a `Result`
	/// with an `Err` type of `()`. It suggests using a custom type that
	/// implements `std::error::Error`.
	///
	/// ### Why is this bad?
	/// Unit does not implement `Error` and carries no
	/// further information about what went wrong.

Ideally, this would be reworded to discuss how a custom error type, especially an enum, could allow one to provide more information about why the error occurred. The fact that a custom error type can implement Error is certainly useful, but probably shouldn't be highlighted as the focus of this lint.

That makes a lot more sense, thanks for the clarification. I'll wait until a maintainer chimes in, but what you're suggesting makes more sense than what I had put out. Thanks!

[giraffate]

Thanks for the discussion!

I feel that the primary issue is that the documentation focuses on the Error trait when the lint doesn't really have much to do with it:

That looks good to me. If no_std, it might be better to suggest core::error::Error with feature gate. I would suggest creating an issue first and then a PR.

Then, can this issue be closed?

[FawazTirmizi]

That's fine, we can continue this discussion on the issue.