Duplicate bounds lint #88096

ibraheemdev · 2021-08-16T22:41:38Z

Deprecate clippy::trait_duplication_in_bounds and add a rustc built-in lint duplicate_bounds that handles more cases:

fn foo<T: Foo + Foo>() {}
fn foo<T: Foo>() where T: Foo {}
fn foo<'a, 'b: 'a + 'a>() {}
fn foo<'a, 'b: 'a>() where 'b: 'a {}
fn foo<'a, T: 'a>() where T: 'a {}
(and the same for structs, impl blocks, etc.)

Resolves #88013.

rust-highfive · 2021-08-16T22:41:40Z

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

rust-highfive · 2021-08-16T22:41:41Z

r? @petrochenkov

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

ibraheemdev · 2021-08-16T22:49:47Z

library/proc_macro/src/lib.rs

This seems to be a false positive from the old clippy lint as well:

#![deny(clippy::pedantic)] fn foo<T: AsRef<str>>() where T: AsRef<String>, { }

error: this trait bound is already specified in the where clause --> src/lib.rs:3:11 | 3 | fn foo<T: AsRef<str>>() | ^^^^^^^^^^ |

I guess I would have to compare Path::segments instead of Res to handle this case?

camsteffen · 2021-08-16T23:27:19Z

Can you add a Clippy test for the renamed lint in tests/ui/rename.rs?

ibraheemdev · 2021-08-16T23:39:43Z

In addition to the test in src/tools/clippy/tests/ui/deprecated.rs?

camsteffen · 2021-08-17T01:11:34Z

In addition to the test in src/tools/clippy/tests/ui/deprecated.rs?

Oh, no that's fine, since other renames are also tested there. But there should be more output in deprecated.stderr.

…same syntax

rust-log-analyzer · 2021-08-17T02:44:05Z

The job x86_64-gnu-llvm-10 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling rustc_mir_build v0.0.0 (/checkout/compiler/rustc_mir_build)
   Compiling rustc_mir v0.0.0 (/checkout/compiler/rustc_mir)
   Compiling rustc_typeck v0.0.0 (/checkout/compiler/rustc_typeck)
   Compiling rustc_plugin_impl v0.0.0 (/checkout/compiler/rustc_plugin_impl)
error: this trait bound has already been specified
    |
    |
222 |         F: Float + Into<Scalar<M::PointerTag>> + FloatConvert<Single> + FloatConvert<Double>,
    |
    |
    = note: `-D duplicate-bounds` implied by `-D warnings`
    = help: consider removing this trait bound
error: could not compile `rustc_mir` due to previous error
warning: build failed, waiting for other jobs to finish...
error: build failed
Build completed unsuccessfully in 0:08:50

petrochenkov · 2021-08-21T18:40:17Z

src/tools/clippy/clippy_lints/src/lib.rs

    ls.register_renamed("clippy::panic_params", "non_fmt_panics");
    ls.register_renamed("clippy::unknown_clippy_lints", "unknown_lints");
    ls.register_renamed("clippy::invalid_atomic_ordering", "invalid_atomic_ordering");
+    ls.register_renamed("clippy::trait_duplication_in_bounds", "trait_duplication_in_bounds");


Suggested change

ls.register_renamed("clippy::trait_duplication_in_bounds", "trait_duplication_in_bounds");

ls.register_renamed("clippy::trait_duplication_in_bounds", "duplicate_bounds");

petrochenkov · 2021-08-21T18:42:45Z

library/core/src/iter/traits/iterator.rs

    /// assert!(result.is_err());
    /// ```
    #[inline]
+    #[cfg_attr(not(bootstrap), allow(duplicate_bounds))]


Why is the lint reported here?

Because Try and TryV2 are the same thing now. cc @scottmcm

Oh, good point, I never cleaned that alias up after the bootstrap compiler updated. New PR: #88223

petrochenkov · 2021-08-21T18:57:07Z

I'm sufficiently sure that the check should not be HIR-based.

Before type checking all bounds are converted to type representation (struct Predicate or something like that).
IIRC, they are "normalized" and interned in process (with aliases removed etc.), making semantic bound comparisons easy.
The lint should use that type checker representation and semantic comparisons, IMO.
The Trait<A> + Trait<B> false positive in particular is unacceptable.

(Could you also add a test for T: Trait + TraitAlias where trait TraitAlias = Trait; after migrating to type checker representation?)

bors · 2021-08-25T19:05:28Z

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

JohnCSimon · 2021-09-12T07:21:35Z

Ping from triage:
@ibraheemdev Can you please resolve the merge conflict?

JohnCSimon · 2021-09-27T06:32:04Z

Ping again from triage:
@ibraheemdev Can you please resolve the merge conflict?

JohnCSimon · 2021-10-31T07:55:50Z

Ping from triage:
@ibraheemdev - this PR hasn't seen any movement in over a month, so I'm closing this as inactive. Please feel free to reopen when you are ready to continue.

rust-highfive assigned petrochenkov Aug 16, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2021

ibraheemdev commented Aug 16, 2021

View reviewed changes

ibraheemdev added 3 commits August 16, 2021 21:59

uplift trait_duplication_in_bounds from clippy to rustc

c90f42a

trait_duplication_in_bounds: warn on duplicate trait bounds of the …

9bc2526

…same syntax

deprecate clippy::trait_duplication_in_bounds

d560008

ibraheemdev force-pushed the dup-trait-bound-lint branch from 2e804ea to 9e7c714 Compare August 17, 2021 02:02

ibraheemdev added 4 commits August 16, 2021 22:02

trait_duplication_in_bounds => duplicate_bounds

46868a7

extend duplicate_bounds to track lifetimes

ab0a962

extend duplicate_bounds to handle region where predicates

9e7c714

add clippy::trait_duplication_in_bounds to deprecated output

c416b99

This comment has been minimized.

Sign in to view

remove duplicate trait bound in rustc_data_structures

e644826

This comment has been minimized.

Sign in to view

remove duplicate trait bound in rustc_ast::walk_variant

ccc2b18

petrochenkov reviewed Aug 21, 2021

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021

JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Oct 31, 2021

JohnCSimon closed this Oct 31, 2021

	ls.register_renamed("clippy::trait_duplication_in_bounds", "trait_duplication_in_bounds");
	ls.register_renamed("clippy::trait_duplication_in_bounds", "duplicate_bounds");

Duplicate bounds lint #88096

Duplicate bounds lint #88096

Uh oh!

Conversation

ibraheemdev commented Aug 16, 2021

Uh oh!

rust-highfive commented Aug 16, 2021

Uh oh!

rust-highfive commented Aug 16, 2021

Uh oh!

ibraheemdev Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camsteffen commented Aug 16, 2021

Uh oh!

ibraheemdev commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camsteffen commented Aug 17, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-log-analyzer commented Aug 17, 2021

Uh oh!

petrochenkov Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

petrochenkov Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

ibraheemdev Aug 21, 2021

Choose a reason for hiding this comment

Uh oh!

scottmcm Aug 22, 2021

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Aug 21, 2021

Uh oh!

bors commented Aug 25, 2021

Uh oh!

JohnCSimon commented Sep 12, 2021

Uh oh!

JohnCSimon commented Sep 27, 2021

Uh oh!

JohnCSimon commented Oct 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ibraheemdev Aug 16, 2021 •

edited

Loading

ibraheemdev commented Aug 16, 2021 •

edited

Loading