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

new lint: implied_bounds_in_impls #11362

Merged
merged 4 commits into from
Aug 24, 2023
Merged

new lint: implied_bounds_in_impls #11362

merged 4 commits into from
Aug 24, 2023

Conversation

y21
Copy link
Member

@y21 y21 commented Aug 20, 2023

Closes #10849

A new lint that looks for explicitly specified bounds that are already implied by other bounds in impl Trait return position types.
One example, as shown in the linked example, would be

fn foo<T>(x: T) -> impl Deref<Target = T> + DerefMut<Target = T> {
    Box::new(x)
}

DerefMut<Target = T> requires Deref<Target = T> to be wellformed, so specifying Deref there is unnecessary.

This currently "ignores" (i.e., does not lint at all) transitive supertrait bounds (e.g. trait A {} trait B: A {} trait C: B {}, then having an impl A + C type), because that seems a bit more difficult and I think this isn't technically a blocker. It leads to FNs, but shouldn't bring any FPs

changelog: new lint [implied_bounds_in_impls]

@rustbot
Copy link
Collaborator

rustbot commented Aug 20, 2023

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 20, 2023
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Really solid start. I love how you documented the steps in the implementation. There are a few nits, mostly related to documentation and the diagnostic messages, but then it should be ready to be merged. The logic itself looks good to me :)

Sorry, for the delay w

clippy_lints/src/implied_bounds_in_impl.rs Outdated Show resolved Hide resolved
clippy_lints/src/implied_bounds_in_impl.rs Outdated Show resolved Hide resolved
clippy_lints/src/implied_bounds_in_impl.rs Outdated Show resolved Hide resolved
tests/ui/implied_bounds_in_impl.stderr Outdated Show resolved Hide resolved
clippy_lints/src/implied_bounds_in_impl.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Aug 23, 2023

☔ The latest upstream changes (presumably #11373) made this pull request unmergeable. Please resolve the merge conflicts.

@y21 y21 changed the title new lint: implied_bounds_in_impl new lint: implied_bounds_in_impls Aug 23, 2023
@xFrednet
Copy link
Member

LGTM, thank you for the new lint and swift update :)

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 24, 2023

📌 Commit 1227571 has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 24, 2023

⌛ Testing commit 1227571 with merge df68b71...

@bors
Copy link
Collaborator

bors commented Aug 24, 2023

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

@bors bors merged commit df68b71 into rust-lang:master Aug 24, 2023
8 checks passed
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 for unnecessary impl in return value
4 participants