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

Fix issue4503 #4543

Merged
merged 13 commits into from
Jan 13, 2020
Merged

Fix issue4503 #4543

merged 13 commits into from
Jan 13, 2020

Conversation

xiongmao86
Copy link
Contributor

@xiongmao86 xiongmao86 commented Sep 14, 2019

Fixes #4503.

changelog: Add a lint checking user are using FileType.is_file() method and suggest using !FileType.is_dir().

  • Followed [lint naming conventions][lint_naming]
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed ./util/dev update_lints
  • Added lint documentation
  • Run ./util/dev fmt

@xiongmao86 xiongmao86 changed the title Issue4503 [WIP] Fix issue4503 Sep 14, 2019
@xiongmao86
Copy link
Contributor Author

I had lint logic ready. But when I cargo +nightly-2019-09-09 test, The dogfood test failed.

This is the log from running cargo +nightly-2019-09-09 test --test dogfood:

rui@Lenovo:~/rust-clippy$ cargo +nightly-2019-09-09 test --test dogfood
    Finished dev [unoptimized + debuginfo] target(s) in 0.11s
     Running target/debug/deps/dogfood-da4d84be69e33eac

running 2 tests
test dogfood ... FAILED
test dogfood_tests ... ok

failures:

---- dogfood stdout ----
status: exit code: 101
stdout: 
stderr:     Blocking waiting for file lock on package cache
   Compiling clippy v0.0.212 (/home/rui/rust-clippy)
error: linking with `cc` failed: exit code: 1
  |
  = note: "cc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-m64" "-L" "/home/rui/.rustup/toolchains/nightly-2019-09-09-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.116t1ydy9ideddxg.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.163er32nmi7i36pe.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.1ggtkyrc3oenmcow.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.20xr7xreua862t0y.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.28z0stdj5wo2lkee.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.2kr03yngyvde3sd3.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.2pkqb6drnwr2zy7j.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.2uv4sjofn9kgntiv.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.2zwnvc543cg2456c.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.323gbj0k1ti6llmh.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.37bp07cn2d9m9cy4.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.3b5kb8rod2da6nlp.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.3nxwsh6mbz8819ob.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.3z2ur4iakbv5rf1l.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.4b9h8zo2ui6hqsso.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.4seg0arvr30n67wd.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.4zza8vk65w7e5qzb.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.50fo9vdt11t5e3f7.rcgu.o" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.v3rurly6dbtaens.rcgu.o" "-o" "/home/rui/rust-clippy/target/dogfood/debug/deps/libclippy.so" "-Wl,--version-script=/tmp/rustc0PiOEV/list" "/home/rui/rust-clippy/target/dogfood/debug/deps/clippy.468osoaaakpbp2ax.rcgu.o" "-Wl,-zrelro" "-Wl,-znow" "-nodefaultlibs" "-L" "/home/rui/rust-clippy/target/dogfood/debug/deps" "-L" "/home/rui/.rustup/toolchains/nightly-2019-09-09-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-Wl,-Bstatic" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libclippy_lints-34e56af20e3b7735.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libregex_syntax-84d19ad960b7da43.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/liburl-a69906a5d0e3aac3.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libpercent_encoding-d21f1bd1e24cdd4b.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libidna-6e9a113b5168a2e3.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libunicode_normalization-0c489a3cf5589085.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libunicode_bidi-a3b77945ab3bd48f.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libpulldown_cmark-a18b014193a39e2c.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libmemchr-b331e9160fa8f7f2.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libunicase-4ece29bee923cb36.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libbitflags-78fdf4f4a9a4744e.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libitertools-ce73c6a363071bf7.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libeither-72736a1b4824b363.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libcargo_metadata-b9866a601bdca73d.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libserde_json-cfcd9bd5c841e46e.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libryu-1b56339d1bed781e.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libitoa-419d61021aca3f4b.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libquine_mc_cluskey-64386ead9a25021c.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libsemver-4006e56a5bfe5e4f.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libsemver_parser-1a06cb80a18ec96a.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libsmallvec-99e107c914a6b5d2.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libmatches-970bad7e3b4dc43a.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/liblazy_static-ec0a077c25ca9a34.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libif_chain-30615dbbdbaf241b.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libtoml-ba77f8fb49059904.rlib" "-Wl,--no-whole-archive" "-Wl,--whole-archive" "/tmp/rustc0PiOEV/libserde-5ada223979f81eba.rlib" "-Wl,--no-whole-archive" "-L" "/home/rui/.rustup/toolchains/nightly-2019-09-09-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-Wl,-Bdynamic" "-lrustc_driver-7f59143e8d87d50e" "-L" "/home/rui/.rustup/toolchains/nightly-2019-09-09-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-ltest-be322d795464d73e" "-Wl,--start-group" "-L" "/home/rui/.rustup/toolchains/nightly-2019-09-09-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-lstd-8b07ab0b0fc43a5b" "-Wl,--end-group" "-Wl,-Bstatic" "/tmp/rustc0PiOEV/libcompiler_builtins-7e9599474821408b.rlib" "-Wl,-Bdynamic" "-lutil" "-lutil" "-ldl" "-lrt" "-lpthread" "-lgcc_s" "-lc" "-lm" "-lrt" "-lpthread" "-lutil" "-lutil" "-shared"
  = note: /usr/bin/ld: /tmp/rustc0PiOEV/libclippy_lints-34e56af20e3b7735.rlib(clippy_lints-34e56af20e3b7735.4kqlnbpflzwa5vjh.rcgu.o): bad reloc symbol index (0x6000000d >= 0x33) for offset 0x21a047 in section `.debug_info'
          /usr/bin/ld: /tmp/rustc0PiOEV/libclippy_lints-34e56af20e3b7735.rlib(clippy_lints-34e56af20e3b7735.4kqlnbpflzwa5vjh.rcgu.o): error adding symbols: bad value
          collect2: error: ld returned 1 exit status
          

error: aborting due to previous error

error: Could not compile `clippy`.

To learn more, run the command again with --verbose.

thread 'dogfood' panicked at 'assertion failed: output.status.success()', tests/dogfood.rs:28:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.


failures:
    dogfood

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--test dogfood'

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

I don't know why the dogfood test fails. But since it's a linker error, you can ignore this.

The important part of this lint is to support all platforms and not only linux.

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Sep 17, 2019
clippy_lints/src/lib.rs Outdated Show resolved Hide resolved
@xiongmao86
Copy link
Contributor Author

xiongmao86 commented Sep 17, 2019

@flip1995 CI build failed. How can I find out which version of toolchain to download for fixing the build? I have commit all your suggestions, but the status still show 1 change requested, I am not familiar with github, is this normal?

@llogiq
Copy link
Contributor

llogiq commented Sep 17, 2019

We currently have a problem with the build we don't quite know how to solve, and it will likely include a fix in rustc. I think the best plan is to wait until the issue is solved, which will hopefully happen within the next 24 hours.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

The "1 change requested" thing won't go away until I (or another maintainer) do another review.

I'm still not sure if this lint behaves correctly on Windows(/macOS). In the issue different behavior of these functions were mentioned. If the lint suggestion is correct on all platforms, please document this.


Yeah CI failure is unrelated. Once #4545 is merged this should go away with another CI run. So as long as it passes locally for you, this should be fine.

tests/compile-test.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Sep 20, 2019

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

Copy link
Contributor Author

@xiongmao86 xiongmao86 left a comment

Choose a reason for hiding this comment

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

@flip1995 I have add two new case to be implemented, but didn't touch the origin lint logic, but why doesn't this line trigger lints?

@xiongmao86
Copy link
Contributor Author

@flip1995, I figured out the bug with dbg!. Now I have update the lint logic, and some minor stuff. And it pass cargo test locally. Can you take a look?

@flip1995 flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 24, 2019
@flip1995
Copy link
Member

Thanks for updating! I try to take a look in the next few days. But I can't promise anything, since I'm pretty swamped with my master thesis.

If I should forget about it, don't hesitate to ping me again :)

@xiongmao86
Copy link
Contributor Author

Thank you.

@xiongmao86 xiongmao86 force-pushed the issue4503 branch 2 times, most recently from dd3e3e2 to e83dc7e Compare January 4, 2020 03:49
@xiongmao86
Copy link
Contributor Author

@flip1995 do you have time to review my code now?

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Yes, sorry for the long wait time and thanks for the ping! I totally forgot about this.

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jan 4, 2020

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

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

I like the solution with "denies"/"covers" 👍

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
@xiongmao86
Copy link
Contributor Author

Ready to merge. @flip1995 thank you very much for helping me with this pr.

@flip1995
Copy link
Member

flip1995 commented Jan 9, 2020

Can you squash some commits? Especially the "Update clippy_lints/..." commits.

@xiongmao86
Copy link
Contributor Author

@flip1995 is this ok? or should I also change the rest update commits?

@flip1995
Copy link
Member

Another update_lints is required. When you're done, please remove the "[WIP]" status from the PR title

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 11, 2020
@bors
Copy link
Contributor

bors commented Jan 11, 2020

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

@xiongmao86
Copy link
Contributor Author

@flip1995, How can I find out the progress of rustup in advance, are there any keyword to search the pull request? or any other means? How can I find out which version of rust to use. I revert the commit, but some other problem show up when I cargo test.

@flip1995
Copy link
Member

flip1995 commented Jan 13, 2020

You can just execute sh setup-toolchain.sh in the clippy root, which installs the latest rustc master, always use this version. If you can't build this version, just wait for a Rustup PR or do it yourself. After that rebase your PR on the Rustup and it should build again.

Rustups are required, when the compiler changes some internals, which Clippy depends on and therefore Clippy breaks. Most of the time, we don't get notified beforehand. But when something breaks, an issue like rust-lang/rust#68107 gets opened in the rust compiler. After that Clippy needs a fix, called a rustup, like #5025 #5040 #4218 .... This should be done in a separate PR though.

@flip1995 flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 13, 2020
@flip1995
Copy link
Member

Now this should be good to go! 🎉 Thanks for all the work!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 13, 2020

📌 Commit bba4688 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jan 13, 2020

⌛ Testing commit bba4688 with merge c24a422...

bors added a commit that referenced this pull request Jan 13, 2020
Fix issue4503

Fixes #4503.

changelog: Add a lint checking user are using FileType.is_file() method and suggest using !FileType.is_dir().

- [x] Followed [lint naming conventions][lint_naming]
- [x] Added passing UI tests (including committed `.stderr` file)
- [x] `cargo test` passes locally
- [x] Executed `./util/dev update_lints`
- [x] Added lint documentation
- [x] Run `./util/dev fmt`
@bors
Copy link
Contributor

bors commented Jan 13, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing c24a422 to master...

@bors bors merged commit bba4688 into rust-lang:master Jan 13, 2020
@xiongmao86
Copy link
Contributor Author

@flip1995, accomplished something finally. Thank you very much for helping me with this pr.

@xiongmao86 xiongmao86 deleted the issue4503 branch January 14, 2020 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: The marked PR was approved and is only waiting bors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggested lint: use "!is_dir()" instead of "is_file()"
5 participants