Run clippy on all the codebase #555

macisamuele · 2019-07-26T21:09:15Z

The main objective of this PR is to ensure that:

clippy is executed on all the codebase (test included)
address reported warnings

I would like to enable more pedantic checks like doc_markdown, use_self and if_not_else but they would generate a pretty high amount of changes (and so potential merge conflicts), so I'm not publishing a PR for now but limiting on having a clear environment such that restrictions can be added without having un-needed noise.

Side note: I've updated .travis.yml to reflect the min rust nightly version specified in build.rs

konstin

Thank you for pull request!

I agree about the more pendantic checks. I thinks it's best to merge this first and then tackle the others in a new pull request.

For reference, this fixes #365.

rust-toolchain

src/buffer.rs

src/types/complex.rs

codecov · 2019-07-26T21:40:23Z

Codecov Report

Merging #555 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
+ Coverage   87.61%   87.64%   +0.02%     
==========================================
  Files          65       65              
  Lines        3449     3449              
==========================================
+ Hits         3022     3023       +1     
+ Misses        427      426       -1

Impacted Files	Coverage Δ
src/buffer.rs	`69.56% <ø> (ø)`	⬆️
src/types/floatob.rs	`95.23% <ø> (ø)`	⬆️
src/types/complex.rs	`100% <100%> (ø)`	⬆️
src/lib.rs	`100% <0%> (+100%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20534ec...fa911e3. Read the comment docs.

codecov · 2019-07-26T21:40:24Z

Codecov Report

Merging #555 into master will increase coverage by 31.81%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #555       +/-   ##
===========================================
+ Coverage    55.8%   87.61%   +31.81%     
===========================================
  Files          83       65       -18     
  Lines        5767     3449     -2318     
  Branches     1022        0     -1022     
===========================================
- Hits         3218     3022      -196     
+ Misses       1757      427     -1330     
+ Partials      792        0      -792

Impacted Files	Coverage Δ
src/buffer.rs	`69.56% <ø> (+27.31%)`	⬆️
src/types/complex.rs	`100% <ø> (+16.78%)`	⬆️
src/types/floatob.rs	`95.23% <ø> (+24.5%)`	⬆️
src/exceptions.rs	`64.7% <0%> (-10.05%)`	⬇️
src/ffi3/objimpl.rs	`0% <0%> (ø)`	⬆️
src/types/any.rs	`100% <0%> (ø)`	⬆️
src/ffi3/pystate.rs
src/ffi3/structseq.rs
pyo3cls/src/lib.rs
src/ffi3/weakrefobject.rs
... and 87 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e752bd2...f15fa21. Read the comment docs.

konstin · 2019-07-26T21:44:25Z

ci/travis/setup.sh

@@ -7,7 +7,7 @@ set -e
 curl https://sh.rustup.rs -sSf | sh -s -- -y --default-toolchain=$TRAVIS_RUST_VERSION
 export PATH=$PATH:$HOME/.cargo/bin
 if [ "$TRAVIS_JOB_NAME" = "Minimum nightly" ]; then
-    rustup component add clippy
+    rustup component add clippy || cargo install --git https://github.com/rust-lang/rust-clippy/ --force clippy


The code is fine, I just wanted to note that I only pinned nightlies in .travis.yml that have clippy.

Correction: I actually think we should revert the change in .travis.yml because the master of clippy is not compatible with that rustc version.

@konstin I saw the issue and you're right ... we cannot build the latest clippy available with the min rust compiler supported by the project.
The major issue here is that instally clippy on nightly-2019-06-22 does not work anymore even if it should be present (according to static.rust-lang.org).
An example of build failure is available here).

What would be your suggestion in this case?

My current idea would be to modify travis such that Minimum nightly task runs only tests and a new task (ie. Lints) will be added to uniquely run cargo fmt and cargo clippy.
Waiting for your feedback before continuing

My suggestion is to revert the changes in .travis.yml and to setup.sh. That way ci should pass again. If that clippy version has bugs fixed in newer versions, we could bump the minimum nightly to e.g. nightly-2019-08-01, which also has clippy and rustfmt. (See https://rust-lang.github.io/rustup-components-history/)

kngwyu · 2019-07-27T12:35:32Z

I don't think we should check tests/examples codes...

Alexander-N · 2019-08-04T10:48:24Z

I don't think we should check tests/examples codes...

Why not? Shouldn't the test code also be in good shape so that it can serve as example on how to use PyO3?

kngwyu · 2019-08-04T12:54:19Z

Yeah, but I feel #[allow(clippy::new_ret_no_self)] noisy.
Clippy doesn't fit some of our DSLs like #[new].

Alexander-N · 2019-08-04T22:38:10Z

I see, it's definitely noisy. But as long as the constructor has the #new attribute, we could call it however we like. How about we use some other name, maybe __new__? I think it would be a good idea to also change this in the guide since users following the conventions they find there also have to deal with this issue.

konstin · 2019-08-05T09:23:59Z

Imo the problem is that the signature that pyo3 requires is wrong; There's no reason why the new can't return an instance of the rust object and the proc macro calls obj.init(instance) as part of the generated wrapper. I've already made some changes simplifying the constructor syntax, but never got to completely converting the syntax. I'd be for leaving the clippy annotations until that's fixed.

macisamuele · 2019-08-17T11:35:19Z

I'm going to rebase this PR out off the current master (to keep a cleaner history).
As from the comments looks like there is some friction around new_ret_no_self I'll make sure that clippy will ignore them.

Add fallback to install clippy from github repo if rustup component add fails

macisamuele · 2019-08-17T13:08:45Z

Test compilation issues with latest nightly version are happening on master as well (https://travis-ci.com/macisamuele/pyo3/jobs/226035909#L789).

I did check proc-macro-hack builds (used under the hood by indoc) and look like their build are failing with today's nightly build

GREEN yesterday: rustc 1.39.0-nightly (f7af19c27 2019-08-15)
RED today: rustc 1.39.0-nightly (bdfd698f3 2019-08-16)

I was not able to trace the specific cause of the issue.

kngwyu · 2019-08-18T03:38:42Z

@macisamuele
Thank you for reporting that!
I'll check it.

kngwyu · 2019-08-18T04:36:14Z

I found the cause dtolnay/proc-macro-hack#40
So maybe it will be fixed in nightly-2019-08-18

konstin · 2019-08-19T11:23:12Z

Thank you!

konstin reviewed Jul 26, 2019

View reviewed changes

rust-toolchain Outdated Show resolved Hide resolved

src/buffer.rs Outdated Show resolved Hide resolved

src/types/complex.rs Outdated Show resolved Hide resolved

konstin reviewed Jul 26, 2019

View reviewed changes

kngwyu assigned konstin Jul 27, 2019

macisamuele added 12 commits August 17, 2019 14:10

Add dedicated makefile target for lint, clippy, fmt execution

1cdd602

Enhance .travis.yml documentation related to nightly min-version

8e4204b

Add fallback to install clippy from github repo if rustup component add fails

Address clippy::unreadable_literal issues

6ae27f9

Address clippy::inconsistent_digit_grouping issues

7034332

Address clippy::redundant_static_lifetimes issues

5e0168b

Address clippy::dead_code issues

8f57d9f

Address clippy::into_iter_on_array issues

0aa02f7

Address clippy::or_fun_call issues

1ed7460

Address clippy::map_clone issues

247c8b5

Address clippy::identity_conversion issues

0de22c6

Address clippy::trivially_copy_pass_by_ref issues

f008c56

Address clippy::float_cmp issues

6b4a221

macisamuele force-pushed the maci-run-clippy-on-all-the-codebase branch from b00c776 to d409cde Compare August 17, 2019 12:41

macisamuele added 2 commits August 17, 2019 15:27

Address clippy::into_iter_on_ref issues

f7ac0a2

Remove unnecessary trailing semicolon

f15fa21

macisamuele force-pushed the maci-run-clippy-on-all-the-codebase branch from d409cde to f15fa21 Compare August 17, 2019 13:27

konstin merged commit 0774334 into PyO3:master Aug 19, 2019

macisamuele deleted the maci-run-clippy-on-all-the-codebase branch August 19, 2019 11:23

Run clippy on all the codebase #555

Run clippy on all the codebase #555

Uh oh!

Conversation

macisamuele commented Jul 26, 2019

Uh oh!

konstin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 26, 2019

Codecov Report

Uh oh!

codecov bot commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

konstin Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

konstin Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

macisamuele Jul 27, 2019

Choose a reason for hiding this comment

Uh oh!

konstin Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

kngwyu commented Jul 27, 2019

Uh oh!

Alexander-N commented Aug 4, 2019

Uh oh!

kngwyu commented Aug 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alexander-N commented Aug 4, 2019

Uh oh!

konstin commented Aug 5, 2019

Uh oh!

macisamuele commented Aug 17, 2019

Uh oh!

macisamuele commented Aug 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kngwyu commented Aug 18, 2019

Uh oh!

kngwyu commented Aug 18, 2019

Uh oh!

konstin commented Aug 19, 2019

Uh oh!

Uh oh!

codecov bot commented Jul 26, 2019 •

edited

Loading

kngwyu commented Aug 4, 2019 •

edited

Loading

macisamuele commented Aug 17, 2019 •

edited

Loading