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

Use diagnostic or language items instead of paths #6823

Merged
merged 2 commits into from
Mar 7, 2021

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Mar 2, 2021

I think that gets everything except ones used in a list of paths to check.

changelog: none

@rust-highfive
Copy link

r? @phansch

(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 Mar 2, 2021
@Jarcho Jarcho force-pushed the diagnostic_items branch from b9b1620 to f21320f Compare March 2, 2021 04:11
clippy_lints/src/lifetimes.rs Outdated Show resolved Hide resolved
clippy_lints/src/types.rs Outdated Show resolved Hide resolved
@phansch
Copy link
Member

phansch commented Mar 2, 2021

Minus the comments from @camsteffen, this looks good to me, thanks!

@phansch phansch closed this Mar 6, 2021
@phansch
Copy link
Member

phansch commented Mar 6, 2021

woops, misclicked 😅

@phansch phansch reopened this Mar 6, 2021
@phansch
Copy link
Member

phansch commented Mar 6, 2021

@bors r+ thanks!

@bors
Copy link
Contributor

bors commented Mar 6, 2021

📌 Commit 11375b5 has been approved by phansch

@bors
Copy link
Contributor

bors commented Mar 6, 2021

⌛ Testing commit 11375b5 with merge f01d655...

bors added a commit that referenced this pull request Mar 6, 2021
Use diagnostic or language items instead of paths

I think that gets everything except ones used in a list of paths to check.

changelog: none
@bors
Copy link
Contributor

bors commented Mar 6, 2021

💔 Test failed - checks-action_test

@Jarcho Jarcho force-pushed the diagnostic_items branch from 11375b5 to e4ffff9 Compare March 6, 2021 18:03
@phansch
Copy link
Member

phansch commented Mar 7, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 7, 2021

📌 Commit e4ffff9 has been approved by phansch

@bors
Copy link
Contributor

bors commented Mar 7, 2021

⌛ Testing commit e4ffff9 with merge 5945e85...

@bors
Copy link
Contributor

bors commented Mar 7, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch
Pushing 5945e85 to master...

@matthiaskrgr
Copy link
Member

Is this expected to have a direct effect on lint results?
I'm asking because the change causes ~160 expl_impl_clone_on_copy warnings to appear in the lintcheck logs.

Most of them seem to be caused by macro usage

target/lintcheck/sources/libc-0.2.81/src/macros.rs:84:9

#[allow(unused_macros)]
macro_rules! s {
    ($($(#[$attr:meta])* pub $t:ident $i:ident { $($field:tt)* })*) => ($(
        s!(it: $(#[$attr])* pub $t $i { $($field)* });
    )*);
    (it: $(#[$attr:meta])* pub union $i:ident { $($field:tt)* }) => (
        compile_error!("unions cannot derive extra traits, use s_no_extra_traits instead");
    );
    (it: $(#[$attr:meta])* pub struct $i:ident { $($field:tt)* }) => (
        __item! {
            #[repr(C)]
            #[cfg_attr(feature = "extra_traits", derive(Debug, Eq, Hash, PartialEq))]
            #[allow(deprecated)]
            $(#[$attr])*
            pub struct $i { $($field)* }
        }
        #[allow(deprecated)]
        impl ::Copy for $i {}
        #[allow(deprecated)]
        impl ::Clone for $i { // <- HERE
            fn clone(&self) -> $i { *self }
        }
    );
}
warning: you are implementing `Clone` explicitly on a `Copy` type
    --> src/macros.rs:84:9
     |
84   | /         impl ::Clone for $i {
85   | |             fn clone(&self) -> $i { *self }
86   | |         }
     | |_________^
     |
    ::: src/unix/linux_like/linux/mod.rs:3502:1
     |
3502 |   expand_align!();
     |   ---------------- in this macro invocation
     |
note: consider deriving `Clone` or removing `Copy`
    --> src/macros.rs:84:9
     |
84   | /         impl ::Clone for $i {
85   | |             fn clone(&self) -> $i { *self }
86   | |         }
     | |_________^
     |
    ::: src/unix/linux_like/linux/mod.rs:3502:1
     |
3502 |   expand_align!();
     |   ---------------- in this macro invocation
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#expl_impl_clone_on_copy
     = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

warning: you are implementing `Clone` explicitly on a `Copy` type
    --> src/macros.rs:120:9
     |
120  | /         impl ::Clone for $i {
121  | |             fn clone(&self) -> $i { *self }
122  | |         }
     | |_________^
     |
    ::: src/unix/linux_like/linux/mod.rs:3502:1
     |
3502 |   expand_align!();
     |   ---------------- in this macro invocation
     |
note: consider deriving `Clone` or removing `Copy`
    --> src/macros.rs:120:9
     |
120  | /         impl ::Clone for $i {
121  | |             fn clone(&self) -> $i { *self }
122  | |         }
     | |_________^
     |
    ::: src/unix/linux_like/linux/mod.rs:3502:1
     |
3502 |   expand_align!();
     |   ---------------- in this macro invocation
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#expl_impl_clone_on_copy
     = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

warning: you are implementing `Clone` explicitly on a `Copy` type
    --> src/macros.rs:120:9
     |
120  | /         impl ::Clone for $i {
121  | |             fn clone(&self) -> $i { *self }
122  | |         }
     | |_________^
     |
    ::: src/unix/linux_like/linux/mod.rs:3502:1
     |
3502 |   expand_align!();
     |   ---------------- in this macro invocation
     |
note: consider deriving `Clone` or removing `Copy`
    --> src/macros.rs:120:9
     |
120  | /         impl ::Clone for $i {
121  | |             fn clone(&self) -> $i { *self }
122  | |         }
     | |_________^
     |
    ::: src/unix/linux_like/linux/mod.rs:3502:1
     |
3502 |   expand_align!();
     |   ---------------- in this macro invocation
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#expl_impl_clone_on_copy
     = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

warning: you are implementing `Clone` explicitly on a `Copy` type
    --> src/macros.rs:120:9
     |
120  | /         impl ::Clone for $i {
121  | |             fn clone(&self) -> $i { *self }
122  | |         }
     | |_________^
     |
    ::: src/unix/linux_like/linux/mod.rs:3502:1
     |
3502 |   expand_align!();
     |   ---------------- in this macro invocation
     |
note: consider deriving `Clone` or removing `Copy`
    --> src/macros.rs:120:9
     |
120  | /         impl ::Clone for $i {
121  | |             fn clone(&self) -> $i { *self }
122  | |         }
     | |_________^
     |
    ::: src/unix/linux_like/linux/mod.rs:3502:1
     |
3502 |   expand_align!();
     |   ---------------- in this macro invocation
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#expl_impl_clone_on_copy
     = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

@@ -293,7 +293,7 @@ fn check_ord_partial_ord<'tcx>(

/// Implementation of the `EXPL_IMPL_CLONE_ON_COPY` lint.
fn check_copy_clone<'tcx>(cx: &LateContext<'tcx>, item: &Item<'_>, trait_ref: &TraitRef<'_>, ty: Ty<'tcx>) {
if match_path(&trait_ref.path, &paths::CLONE_TRAIT) {
if cx.tcx.lang_items().clone_trait() == trait_ref.trait_def_id() {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe comparing def_ids isn't equivalent to the match_path method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lang item might not be defined at the point. I didn't think that would actually happen though. I'll have a pr in a bit.

@Jarcho
Copy link
Contributor Author

Jarcho commented Mar 9, 2021

Turns out this did fix expl_impl_clone_on_copy, which is causing all those warnings. #6868 (comment)

@flip1995
Copy link
Member

flip1995 commented Mar 9, 2021

Yeah, I agree. Triggering in the above example makes sense to me. I also think triggering in local macros is fine.

bors added a commit that referenced this pull request Mar 9, 2021
Don't assume lang items will exist.

~~Should fix lintcheck warnings caused by #6823~~
See below

changelog: None
@Jarcho Jarcho deleted the diagnostic_items branch April 6, 2021 16:09
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.

7 participants