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

Extend extra_unused_lifetimes to handle impl lifetimes #8737

Merged
merged 3 commits into from Apr 26, 2022

Conversation

smoelius
Copy link
Contributor

Fixes #6437 (cc: @carols10cents)

changelog: fix #6437

@rust-highfive
Copy link

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 23, 2022
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

Would these cases be false positive? I found these here:

// first case
use serde::de::Visitor;
pub trait Expected {
    fn fmt(&self, formatter: &mut std::fmt::Formatter);
}

impl<'de, T> Expected for T
where
    T: Visitor<'de>,
{
    fn fmt(&self, formatter: &mut std::fmt::Formatter) {}
}

// second case
pub trait Source {
    fn hey();
}

impl<'a, T: Source + ?Sized + 'a> Source for Box<T> {
    fn hey() {}
}

@smoelius
Copy link
Contributor Author

smoelius commented Apr 25, 2022

Would these cases be false positive?

The answer is "yes," of course. :) I think the just pushed commit fixes the problem.

Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks!

I made one comment for naming.

ImplItemKind, Item, ItemKind, LangItem, Lifetime, LifetimeName, ParamName, PolyTraitRef, TraitBoundModifier,
TraitFn, TraitItem, TraitItemKind, Ty, TyKind, WhereClause, WherePredicate,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::hir::nested_filter as mir_nested_filter;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why this is named mir. Can this be explained?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I misread the crate name. Maybe it's better now (middle_nested_filter)?

@giraffate
Copy link
Contributor

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Apr 26, 2022

📌 Commit c22bb06 has been approved by giraffate

@bors
Copy link
Collaborator

bors commented Apr 26, 2022

⌛ Testing commit c22bb06 with merge 94623ee...

@bors
Copy link
Collaborator

bors commented Apr 26, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 94623ee to master...

@bors bors merged commit 94623ee into rust-lang:master Apr 26, 2022
@smoelius smoelius deleted the extra-impl-lifetimes branch April 26, 2022 00:39
dario23 added a commit to dario23/rust-semverver that referenced this pull request May 20, 2022
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.

extra_unused_lifetimes doesn't warn for lifetimes declared on impl and never used
4 participants