(Preliminary) Results of building rust with clippy #278

Open
llogiq opened this Issue Sep 2, 2015 · 14 comments

Comments

Projects
None yet
2 participants
@llogiq
Collaborator

llogiq commented Sep 2, 2015

I was able to check the libfmt_macros crate with clippy and found one str_to_string, one while_let_loop and a few unneeded_return_statements.

Checking the libsyntax crate failed with the following stack trace:

thread '<unnamed>' panicked at 'index out of bounds: the len is 60 but the index is 190', src/libcollections/vec.rs:1047
stack backtrace:
   1:     0x7f21e6fc67c9 - sys::backtrace::tracing::imp::write::h9d995b4e2eebc457Mns
   2:     0x7f21e6fce74c - panicking::on_panic::h0af95cd615077cb32ox
   3:     0x7f21e6f925de - rt::unwind::begin_unwind_inner::ha19c9fa5f9b03821GRw
   4:     0x7f21e6f93327 - rt::unwind::begin_unwind_fmt::hff85cd541f1ded16MQw
   5:     0x7f21e6fce0e1 - rust_begin_unwind
   6:     0x7f21e701fbbf - panicking::panic_fmt::hd2a7f20260b38c91J7E
   7:     0x7f21e7019ed2 - panicking::panic_bounds_check::h270e000900cee620P6E
   8:     0x7f21e7531999 - parse::token::InternedString::new_from_name::h133415336b0a3dbbomT
   9:     0x7f21e88547ad - ast::Name.PartialEq<T>::eq::h8722861357916141903
  10:     0x7f21e886f3a3 - len_zero::LenZero.LintPass::check_item::h98dc7c5e93c3cd56WYc
  11:     0x7f21f47be707 - lint::context::Context<'a, 'tcx>::with_lint_attrs::h15905335786845705409
  12:     0x7f21f47c3a1e - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_mod::hbe9f346fd6f42bffkaw
  13:     0x7f21f47bedb4 - lint::context::Context<'a, 'tcx>::with_lint_attrs::h15905335786845705409
  14:     0x7f21f47c3a1e - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_mod::hbe9f346fd6f42bffkaw
  15:     0x7f21f47bedb4 - lint::context::Context<'a, 'tcx>::with_lint_attrs::h15905335786845705409
  16:     0x7f21f47c3a1e - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_mod::hbe9f346fd6f42bffkaw
  17:     0x7f21f47d0f09 - lint::context::check_crate::hf80c5adb00775b2ejuw
  18:     0x7f21f403a299 - driver::phase_3_run_analysis_passes::closure.20670
  19:     0x7f21f4034d8e - middle::ty::ctxt<'tcx>::create_and_enter::h18281789251420907216
  20:     0x7f21f402fd81 - driver::phase_3_run_analysis_passes::h17232049417500553259
  21:     0x7f21f4014440 - driver::compile_input::h37859d56374280caTba
  22:     0x7f21f40f4946 - run_compiler::h86a39f47860f70c7C7b
  23:     0x7f21f40f26c9 - boxed::F.FnBox<A>::call_box::h15702049526784006720
  24:     0x7f21f40f20a4 - rt::unwind::try::try_fn::h2747471524830943407
  25:     0x7f21f5b6e308 - __rust_try
  26:     0x7f21f5b60bc2 - rt::unwind::try::inner_try::ha3b130acdaf96544aWw
  27:     0x7f21f40f2248 - boxed::F.FnBox<A>::call_box::h2060115051421633638
  28:     0x7f21f5b6d6f3 - sys::thread::Thread::new::thread_start::h3819a1565b813297A5v
  29:     0x7f21f3a4a6a9 - start_thread
  30:     0x7f21f2e55eec - clone
  31:                0x0 - <unknown>

This may be due to a mismatch between the stage0 compiler and the compiler with which I built clippy, but I'm not sure.

Trying to check librustc with clippy causes a Segmentation fault on my system.

I'll try to check other rustc/std crates with clippy.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 2, 2015

Collaborator

This probably is a mismatch; the method doesn't look panicky.

Compile rustc-stage1, compile clippy with rustc-stage1 (use multirust), and then do RUSTFLAGS_STAGE2=-Z .... make rustc-stage2. Or something like that.

Collaborator

Manishearth commented Sep 2, 2015

This probably is a mismatch; the method doesn't look panicky.

Compile rustc-stage1, compile clippy with rustc-stage1 (use multirust), and then do RUSTFLAGS_STAGE2=-Z .... make rustc-stage2. Or something like that.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 2, 2015

Collaborator

Yeah, probably. I'm in the process of installing multirust. I'll report back once I have more results.

Collaborator

llogiq commented Sep 2, 2015

Yeah, probably. I'm in the process of installing multirust. I'll report back once I have more results.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 3, 2015

Collaborator

More results – it appears I've run into the same problem as Manish on servo (also some false positives that should be removed in the meantime). I'll update and run again.

Btw. if someone intends to replicate this, one first needs to make stage1, then use multirust run stage1 cargo build --release on clippy (I've linked a nightly cargo into my stage1 toolchain), then run VERBOSE=1 RUSTFLAGS_STAGE2='-L ~/projects/rust-clippy/target/release/ -Z extra-plugins=clippy' RUST_BACKTRACE=1 make rustc-stage2.

One thing I found very interesting:

src/libcore/hash/sip.rs:67:26: 152:47 error: the operation is ineffective. Consider reducing it to `t`, #[deny(identity_op)] on by default
src/libcore/hash/sip.rs:67             out |= ($buf[t+$i] as u64) << t*8;
src/libcore/hash/sip.rs:68             t += 1;
src/libcore/hash/sip.rs:69         }
src/libcore/hash/sip.rs:70         out
src/libcore/hash/sip.rs:71     });
src/libcore/hash/sip.rs:72 }
                           ...
src/libcore/hash/sip.rs:52:1: 72:2 note: in expansion of u8to64_le!
src/libcore/hash/sip.rs:152:30: 152:56 note: expansion site
src/libcore/hash/sip.rs:67:26: 152:47 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#identity_op

Isn't that a false positive here?

Collaborator

llogiq commented Sep 3, 2015

More results – it appears I've run into the same problem as Manish on servo (also some false positives that should be removed in the meantime). I'll update and run again.

Btw. if someone intends to replicate this, one first needs to make stage1, then use multirust run stage1 cargo build --release on clippy (I've linked a nightly cargo into my stage1 toolchain), then run VERBOSE=1 RUSTFLAGS_STAGE2='-L ~/projects/rust-clippy/target/release/ -Z extra-plugins=clippy' RUST_BACKTRACE=1 make rustc-stage2.

One thing I found very interesting:

src/libcore/hash/sip.rs:67:26: 152:47 error: the operation is ineffective. Consider reducing it to `t`, #[deny(identity_op)] on by default
src/libcore/hash/sip.rs:67             out |= ($buf[t+$i] as u64) << t*8;
src/libcore/hash/sip.rs:68             t += 1;
src/libcore/hash/sip.rs:69         }
src/libcore/hash/sip.rs:70         out
src/libcore/hash/sip.rs:71     });
src/libcore/hash/sip.rs:72 }
                           ...
src/libcore/hash/sip.rs:52:1: 72:2 note: in expansion of u8to64_le!
src/libcore/hash/sip.rs:152:30: 152:56 note: expansion site
src/libcore/hash/sip.rs:67:26: 152:47 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#identity_op

Isn't that a false positive here?

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 3, 2015

Collaborator

Isn't that a false positive here?

Yes. This is probably because we don't consider casts in our framework. But perhaps we should ignore this after verifying that casts indeed are the reason.

Collaborator

Manishearth commented Sep 3, 2015

Isn't that a false positive here?

Yes. This is probably because we don't consider casts in our framework. But perhaps we should ignore this after verifying that casts indeed are the reason.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 3, 2015

Collaborator

It might be possible to make use of Clippy's structured logging feature to output the lints as JSON and autofix them. For example, knowing that there's an elision issue on a range of line numbers, we may fix it via python or something.

Collaborator

Manishearth commented Sep 3, 2015

It might be possible to make use of Clippy's structured logging feature to output the lints as JSON and autofix them. For example, knowing that there's an elision issue on a range of line numbers, we may fix it via python or something.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 3, 2015

Collaborator

My lifetime-applying script is:

while read in; do
 IFS=: read file line <<< "$in";

#echo $file;
#echo $line;
(sed -n -e "${line}p" $file | grep "Pattern" >/dev/null) || (
sed  -i "${line}s/\&'a /\&/g" $file;
sed  -i "${line}s/\&'a/\&/g" $file;
sed  -i "${line}s/'a, //g" $file;
sed  -i "${line}s/'a,//g" $file;
sed  -i "${line}s/<'a>//g" $file;
(sed -n -e "${line}p" $file | grep "'a" >/dev/null)  && (echo $file; echo $line);
);
done <lifetimesln

where lifetimesln was created from cat run | grep -A 1 "#\[deny(needless_lifetimes)\] on by default" | grep -v deny | grep -v "\-\-" | awk '{print $1}' (and run is just the above logs),

Far from perfect, but it handles most cases, and prints out whatever it can't handle.

Collaborator

Manishearth commented Sep 3, 2015

My lifetime-applying script is:

while read in; do
 IFS=: read file line <<< "$in";

#echo $file;
#echo $line;
(sed -n -e "${line}p" $file | grep "Pattern" >/dev/null) || (
sed  -i "${line}s/\&'a /\&/g" $file;
sed  -i "${line}s/\&'a/\&/g" $file;
sed  -i "${line}s/'a, //g" $file;
sed  -i "${line}s/'a,//g" $file;
sed  -i "${line}s/<'a>//g" $file;
(sed -n -e "${line}p" $file | grep "'a" >/dev/null)  && (echo $file; echo $line);
);
done <lifetimesln

where lifetimesln was created from cat run | grep -A 1 "#\[deny(needless_lifetimes)\] on by default" | grep -v deny | grep -v "\-\-" | awk '{print $1}' (and run is just the above logs),

Far from perfect, but it handles most cases, and prints out whatever it can't handle.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 3, 2015

Collaborator

What about lifetimes with other names than 'a?

Collaborator

llogiq commented Sep 3, 2015

What about lifetimes with other names than 'a?

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 3, 2015

Collaborator

/me was lazy

Collaborator

Manishearth commented Sep 3, 2015

/me was lazy

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 3, 2015

Collaborator

😄

while read in; do
 IFS=: read file line <<< "$in";

#echo $file;
#echo $line;
(sed -n -e "${line}p" $file | grep "Pattern" >/dev/null) || (
sed  -i "${line}s/\&'\\w+ /\&/g" $file;
sed  -i "${line}s/\&'\\w+/\&/g" $file;
sed  -i "${line}s/'\\w+, //g" $file;
sed  -i "${line}s/'\\w+,//g" $file;
sed  -i "${line}s/<'\\w+>//g" $file;
(sed -n -e "${line}p" $file | grep "'\\w+[^\\w']" >/dev/null)  && (echo $file; echo $line);
);
done <lifetimesln

Might work, but i think there might be a clash with char literals. For example let a = &'a'. Be careful.

Collaborator

llogiq commented Sep 3, 2015

😄

while read in; do
 IFS=: read file line <<< "$in";

#echo $file;
#echo $line;
(sed -n -e "${line}p" $file | grep "Pattern" >/dev/null) || (
sed  -i "${line}s/\&'\\w+ /\&/g" $file;
sed  -i "${line}s/\&'\\w+/\&/g" $file;
sed  -i "${line}s/'\\w+, //g" $file;
sed  -i "${line}s/'\\w+,//g" $file;
sed  -i "${line}s/<'\\w+>//g" $file;
(sed -n -e "${line}p" $file | grep "'\\w+[^\\w']" >/dev/null)  && (echo $file; echo $line);
);
done <lifetimesln

Might work, but i think there might be a clash with char literals. For example let a = &'a'. Be careful.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 3, 2015

Collaborator

Ideally, we should add JSON logging of specific lints. Eg the lifetime elision will log fileline info (preferably notify if the FnDecl is multiline, but that's hard), and the elidable lifetimes.

Collaborator

Manishearth commented Sep 3, 2015

Ideally, we should add JSON logging of specific lints. Eg the lifetime elision will log fileline info (preferably notify if the FnDecl is multiline, but that's hard), and the elidable lifetimes.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 4, 2015

Collaborator

I'm also unsure if this could constitute a false positive:

src/libcore/slice.rs:971:31: 971:42 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#mut_mut
src/libcore/slice.rs:991:40: 991:51 error: this expression mutably borrows a mutable reference. Consider reborrowing, #[deny(mut_mut)] on by default
src/libcore/slice.rs:991                 let tmp = mem::replace(&mut self.v, &mut []);
                                                                ^~~~~~~~~~~

But this surely does:

src/libcore/str/mod.rs:94:29: 94:38 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#unit_cmp
src/libcore/str/mod.rs:94:29: 94:38 error: !=-comparison of unit values detected. This will always be false, #[deny(unit_cmp)] on by default
src/libcore/str/mod.rs:94 pub struct ParseBoolError { _priv: () }
                                                      ^~~~~~~~~

(it appears unit_cmp needs an in_macro check)

By the way, I revisited the identity_op false positive – it's actually not really a false positive. $i will be 0 during macro execution, and the implementation relies on LLVM optimizing the + 0 away. I think we should broaden our macro check to all macro expansions (not only external ones) but the usual for-, if/while let- and other language-level expansions to get rid of warnings due to this pattern.

Collaborator

llogiq commented Sep 4, 2015

I'm also unsure if this could constitute a false positive:

src/libcore/slice.rs:971:31: 971:42 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#mut_mut
src/libcore/slice.rs:991:40: 991:51 error: this expression mutably borrows a mutable reference. Consider reborrowing, #[deny(mut_mut)] on by default
src/libcore/slice.rs:991                 let tmp = mem::replace(&mut self.v, &mut []);
                                                                ^~~~~~~~~~~

But this surely does:

src/libcore/str/mod.rs:94:29: 94:38 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#unit_cmp
src/libcore/str/mod.rs:94:29: 94:38 error: !=-comparison of unit values detected. This will always be false, #[deny(unit_cmp)] on by default
src/libcore/str/mod.rs:94 pub struct ParseBoolError { _priv: () }
                                                      ^~~~~~~~~

(it appears unit_cmp needs an in_macro check)

By the way, I revisited the identity_op false positive – it's actually not really a false positive. $i will be 0 during macro execution, and the implementation relies on LLVM optimizing the + 0 away. I think we should broaden our macro check to all macro expansions (not only external ones) but the usual for-, if/while let- and other language-level expansions to get rid of warnings due to this pattern.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 5, 2015

Collaborator

I've tried to replicate my findings with a current Rust, but failed because I couldn't compile clippy with a stage1 toolchain, because it didn't find the syntax crate. I'll try and rebuild the toolchain.

Collaborator

llogiq commented Sep 5, 2015

I've tried to replicate my findings with a current Rust, but failed because I couldn't compile clippy with a stage1 toolchain, because it didn't find the syntax crate. I'll try and rebuild the toolchain.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 5, 2015

Collaborator

You need to first make check-stage1-rpass-full to get the stage1 libs to build (terminate it when it starts running tests). By default make rustc-stage1 does not build librustc and libsyntax IIRC>

Collaborator

Manishearth commented Sep 5, 2015

You need to first make check-stage1-rpass-full to get the stage1 libs to build (terminate it when it starts running tests). By default make rustc-stage1 does not build librustc and libsyntax IIRC>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment