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

Duplicate bounds lint #88096

Closed

Conversation

ibraheemdev
Copy link
Member

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
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Aug 16, 2021
Comment on lines +1236 to 1237
#[cfg_attr(not(bootstrap), allow(duplicate_bounds))]
pub fn var<K: AsRef<OsStr> + AsRef<str>>(key: K) -> Result<String, VarError> {
Copy link
Member Author

@ibraheemdev ibraheemdev Aug 16, 2021

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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

@ibraheemdev
Copy link
Member Author

ibraheemdev commented Aug 16, 2021

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

@camsteffen
Copy link
Contributor

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.

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

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

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

@@ -2182,6 +2180,7 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) {
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");
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -2434,6 +2434,7 @@ pub trait Iterator {
/// assert!(result.is_err());
/// ```
#[inline]
#[cfg_attr(not(bootstrap), allow(duplicate_bounds))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the lint reported here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@petrochenkov
Copy link
Contributor

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?)

@petrochenkov 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
@bors
Copy link
Contributor

bors commented Aug 25, 2021

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

@JohnCSimon
Copy link
Member

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

@JohnCSimon
Copy link
Member

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

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021
@JohnCSimon JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Oct 31, 2021
@JohnCSimon
Copy link
Member

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.

@JohnCSimon JohnCSimon closed this Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint on duplicate trait bound
9 participants