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 *Error types that don't implement core::error::Error #14291

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

SpecificProtagonist
Copy link

@SpecificProtagonist SpecificProtagonist commented Feb 25, 2025

fixes #1291


See the linked issue.
Running linkcheck, this lint triggered 6 3 times

  • 2 of which are genuine missing error impls
  • 3 of which are triggered because the crate supports Rust 1.80 or older
  • 1 of which is because of public item in a private module, which should probably be fixed regardless of this lint.

I could see this lint generating a false positives if there is e.g. a pub struct CalcError(f32); that describes the amount of error in some calculation, but I that's the exception.

changelog: [missing_error_implementations]: New lint that checks for exported types that should likely implement Error, but don't.

@rustbot
Copy link
Collaborator

rustbot commented Feb 25, 2025

r? @blyxyas

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 25, 2025
@SpecificProtagonist SpecificProtagonist changed the title Lint *Error types that don't implement std::error::Error Lint *Error types that don't implement core::error::Error Feb 25, 2025
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

Very good first patch! I have some comments. Thanks for the contribution and sorry for the delay. I didn't see that you were a first-time contributor, I like to prioritize first-timers here 🤠

@SpecificProtagonist
Copy link
Author

SpecificProtagonist commented Mar 12, 2025

Thanks for the review ❤️

Noticed this suggestion, should I add that too?
#1291 (comment)

Additionally, we could lint this for types which are used for associated Error and Err types on traits. For example, someone might have an error type called BadValue and use type Err = BadValue on a FromStr impl, and we'd still want to catch that too.

@alice-i-cecile
Copy link

Hey, just wanted to chime in as a user: thanks a ton for implementing and reviewing this!

// Identify types which likely represent errors
&& {
let name: &str = item.ident.name.as_str();
name.ends_with("Error") && name != "Error"

Choose a reason for hiding this comment

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

There should be a comment here about why an item named "Error" shouldn't be caught by this lint.

Choose a reason for hiding this comment

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

Originally added that because I didn't want clippy to recommend a change that triggers another lint, but thinking about it I'll remove that check.

use rustc_session::impl_lint_pass;
use rustc_span::sym;

declare_clippy_lint! {

Choose a reason for hiding this comment

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

Should the docs for this lint link back to the error-impl-error lint?

@blyxyas
Copy link
Member

blyxyas commented Mar 18, 2025

Additionally, we could lint this for types which are used for associated Error and Err types on traits

I can see how this would be very useful, but there can also exist error types unrelated. An associated Error type could be an i32 with an error code and I'm not sure if we can really detect those.

@SpecificProtagonist
Copy link
Author

I also want to add a check that skips the lint if there's a impl<T: Error> From<T> because that would result in a conflicting implementations error, but I haven't yet figured out how to do that.

@blyxyas
Copy link
Member

blyxyas commented Mar 26, 2025

For that, you can use implements_trait, the From<T> trait has a diagnostic item associated, so we can use tcx.get_diagnostic_item with sym::From (because the trait also has an associated symbol).

imeplements_trait would collect a single GenericArg, we need to check that it implements Error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint *Error types that don't implement std::error::Error
5 participants