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

Fix more false positives for extra_unused_type_parameters #10392

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

mkrasnitski
Copy link
Contributor

Builds on #10321. All empty functions are no longer linted, instead of just those that have trait bounds on them. Also, if a trait bound contains a non-public trait (un-exported, but still potentially reachable), then the corresponding type parameter isn't linted.

Finally, added support for the avoid_breaking_exported_api config option.

r? @flip1995
changelog: none

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 22, 2023
@mkrasnitski
Copy link
Contributor Author

mkrasnitski commented Feb 23, 2023

@flip1995 would be great if this can make it in before today's sync. A lintcheck may or may not be necessary. I can atleast confirm the FPs you brought up no longer happen.

Copy link
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

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

Quick review. Let's see if we can get this in today.

clippy_lints/src/extra_unused_type_parameters.rs Outdated Show resolved Hide resolved
clippy_lints/src/extra_unused_type_parameters.rs Outdated Show resolved Hide resolved
@Jarcho
Copy link
Contributor

Jarcho commented Feb 23, 2023

Going to merge this now and save @flip1995 from having to wait a little for it.

Thank you. @bors r+

@bors
Copy link
Contributor

bors commented Feb 23, 2023

📌 Commit 528bb63 has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 23, 2023

⌛ Testing commit 528bb63 with merge f9adb83...

@bors
Copy link
Contributor

bors commented Feb 23, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing f9adb83 to master...

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

@mkrasnitski Awesome work, thanks for implementing this so quickly. Really a pleasure working with you!

I have two more comments, but they are just NITs. This unblocks the sync. Thanks for helping review @Jarcho


/// Don't lint external macros or functions with empty bodies. Also, don't lint public items if
/// the `avoid_breaking_exported_api` config option is set.
fn check_false_positive(&self, cx: &LateContext<'_>, span: Span, def_id: LocalDefId, body_id: BodyId) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Weird function name IMO. But the best thing I can come up with is: is_exported_macro_or_empty.

Copy link
Contributor Author

@mkrasnitski mkrasnitski Feb 23, 2023

Choose a reason for hiding this comment

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

yeah, names are hard... I couldn't really come up with anything satisfactory either. Maybe is_empty_exported_or_macro is the least ambiguous.

fn unused_with_priv_trait_bound<T: private::Private, U>() {
unimplemented!();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I missed it, but I think there is a test for a pub function missing (should not get linted).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, that relies on the config option, right? Does uitest try different values of config vars as listed in clippy.toml?

Copy link
Member

Choose a reason for hiding this comment

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

Have a look in the test/ui-toml dir on how to test different config values.

@mkrasnitski
Copy link
Contributor Author

Glad to hear this made it in time. I have plans to try using span_lint_and_sugg() like you recommended previously, so these nits can be included along with those changes, when they happen.

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.

5 participants