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

Backwards compatibility for tool/clippy lints #53762

Merged
merged 6 commits into from
Sep 1, 2018

Conversation

flip1995
Copy link
Member

cc #44690
cc rust-lang/rust-clippy#2977 (comment)

This is the next step towards tool_lints.

This makes Clippy lints still work without scoping, but will warn and suggest the new scoped name. This warning will only appear if the code is checked with Clippy itself.

There is still an issue with using the old lint name in inner attributes. For inner attributes the warning gets emitted twice. I'm currently not really sure why this happens, but will try to fix this ASAP.

r? @Manishearth

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 28, 2018
|
= note: #[warn(unknown_lints)] on by default

warning: lint name `clippy_group` is deprecated and may not have an effect in the future
Copy link
Member Author

Choose a reason for hiding this comment

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

This warning is emitted twice

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Aug 28, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:48:05] ....................................................................................................
[00:48:09] ....................................................................................................
[00:48:11] ..........i.........................................................................................
[00:48:14] ....................................................................................................
[00:48:16] ...........................................................iiiiiiiii................................
[00:48:22] ....................................................................................................
[00:48:25] ....................................................................................................
[00:48:28] .......................................i............................................................
[00:48:31] ..........................................................................................ii..ii....
---
travis_time:start:test_ui-fulldeps
Check compiletest suite=ui-fulldeps mode=ui (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:55:47] 
[00:55:47] running 29 tests
[00:56:09] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:497:22
[00:56:09] .............F...............
[00:56:09] 
[00:56:09] ---- [ui] ui-fulldeps/lint_tool_test.rs stdout ----
[00:56:09] 
[00:56:09] 
[00:56:09] error: /checkout/src/test/ui-fulldeps/lint_tool_test.rs:17: unexpected warning: '17:9: 17:21: lint name `clippy_group` is deprecated and may not have an effect in the future [renamed_and_removed_lints]'
[00:56:09] error: 1 unexpected errors found, 0 expected errors not found
[00:56:09] status: exit code: 1
[00:56:09] status: exit code: 1
[00:56:09] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui-fulldeps/lint_tool_test.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/lint_tool_test/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/lint_tool_test/auxiliary" "-A" "unused"
[00:56:09]     Error {
[00:56:09]         line_num: 17,
[00:56:09]         kind: Some(
[00:56:09]             Warning
[00:56:09]             Warning
[00:56:09]         ),
[00:56:09]         msg: "17:9: 17:21: lint name `clippy_group` is deprecated and may not have an effect in the future [renamed_and_removed_lints]"
[00:56:09] ]
[00:56:09] 
[00:56:09] thread '[ui] ui-fulldeps/lint_tool_test.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:1283:13
[00:56:09] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:56:09] test result: FAILED. 28 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
[00:56:09] 
[00:56:09] 
[00:56:09] 
[00:56:09] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui-fulldeps" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-5.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "5.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:56:09] 
[00:56:09] 
[00:56:09] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:56:09] Build completed unsuccessfully in 0:11:56
[00:56:09] Build completed unsuccessfully in 0:11:56
[00:56:09] Makefile:58: recipe for target 'check' failed
[00:56:09] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:00f74fad
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:03728b9f:start=1535463833680044718,finish=1535463833688457486,duration=8412768
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1b92b6d2
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_f

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

design LGTM. This is somewhat temporary code so I'm not looking too closely.

I can r+, but perhaps we should r? @nrc

@Manishearth
Copy link
Member

Did a closer pass through, looks good.

Might be good to also lint on cfg_attr(clippy) asking people to remove it in favor of these but that's trickier to do. Perhaps just mention in the lint warning that cfg(clippy) is also not necessary anymore.

r=manishearth it if you end up making those changes and/or feel it's otherwise ready

@bors delegate+

@bors
Copy link
Contributor

bors commented Aug 28, 2018

✌️ @flip1995 can now approve this pull request

@Manishearth
Copy link
Member

btw, feel free to submodule your clippy PR here so we don't have to do the submodule dance, just be careful not to push to that PR once this merges (I'll merge it on that side)

@Manishearth
Copy link
Member

We can also merge into a staging branch if you'd like

@flip1995
Copy link
Member Author

Might be good to also lint on cfg_attr(clippy) asking people to remove it in favor of these but that's trickier to do.

I tried to write a Clippy lint for that, but I couldn't get it to lint on inner attributes. (unfinished: https://github.com/flip1995/rust-clippy/commits/cfg_attr_lint) I think the problem was, that they never appear in the AST. Either they were already removed or converted to just allow/warn/.... I would add a hint, that also cfg_attr are deprecated.

Before merging this I'd like to fix #53762 (comment)

btw, feel free to submodule your clippy PR here

How can I do that? My git-fu isn't good enough for that. 😄

@Manishearth
Copy link
Member

Is the clippy side PR ready? Give me push access (tick the box on this page that lets rust collab push to the PR) and I'll do the rest.

@flip1995
Copy link
Member Author

The Clippy PR is ready. It builds without errors with this PR. The box is ticked, thanks for doing this!

@Manishearth
Copy link
Member

Gonna wait for #53758 to land first, but will do soon!

new_name.to_string(),
)));
}
CheckLintNameResult::Tool(Ok(&ids.0))
Copy link
Member Author

Choose a reason for hiding this comment

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

FIXME: This should be Err((Some(ids), complete_name)

@Manishearth
Copy link
Member

Rebased, updated submodule, and pushed. If this lands (with or without breaking clippy), please press the merge button on rust-lang/rust-clippy#2977 without adding any commits to the PR. Do a manual merge if there are conflicts (there shouldn't be). If clippy is still broken, use rustup-toolchain-install-master to get the latest rustc master build and fix it, and open a PR.

(I'd do the latter steps myself but I'm getting on a flight soon)

@Manishearth
Copy link
Member

@bors r+ delegate+

@bors
Copy link
Contributor

bors commented Aug 29, 2018

✌️ @flip1995 can now approve this pull request

@bors
Copy link
Contributor

bors commented Aug 29, 2018

📌 Commit 0beab9472373c9bfcf145d3ee2ff9d384728c9fd has been approved by Manishearth

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:49:01] ....................................................................................................
[00:49:05] ....................................................................................................
[00:49:07] ..........i.........................................................................................
[00:49:10] ....................................................................................................
[00:49:13] ...........................................................iiiiiiiii................................
[00:49:18] ....................................................................................................
[00:49:22] ....................................................................................................
[00:49:24] .......................................i............................................................
[00:49:27] ..........................................................................................ii...ii...
---
travis_time:start:test_ui-fulldeps
Check compiletest suite=ui-fulldeps mode=ui (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:56:49] 
[00:56:49] running 29 tests
[00:57:11] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:497:22
[00:57:11] ............F................
[00:57:11] 
[00:57:11] ---- [ui] ui-fulldeps/lint_tool_test.rs stdout ----
[00:57:11] 
[00:57:11] 
[00:57:11] error: /checkout/src/test/ui-fulldeps/lint_tool_test.rs:17: unexpected warning: '17:9: 17:21: lint name `clippy_group` is deprecated and may not have an effect in the future [renamed_and_removed_lints]'
[00:57:11] error: 1 unexpected errors found, 0 expected errors not found
[00:57:11] status: exit code: 1
[00:57:11] status: exit code: 1
[00:57:11] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui-fulldeps/lint_tool_test.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/lint_tool_test/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/lint_tool_test/auxiliary" "-A" "unused"
[00:57:11]     Error {
[00:57:11]         line_num: 17,
[00:57:11]         kind: Some(
[00:57:11]             Warning
[00:57:11]             Warning
[00:57:11]         ),
[00:57:11]         msg: "17:9: 17:21: lint name `clippy_group` is deprecated and may not have an effect in the future [renamed_and_removed_lints]"
[00:57:11] ]
[00:57:11] 
[00:57:11] thread '[ui] ui-fulldeps/lint_tool_test.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:1283:13
[00:57:11] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:57:11] test result: FAILED. 28 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
[00:57:11] 
[00:57:11] 
[00:57:11] 
[00:57:11] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui-fulldeps" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-5.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "5.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:57:11] 
[00:57:11] 
[00:57:11] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:57:11] Build completed unsuccessfully in 0:12:12
[00:57:11] Build completed unsuccessfully in 0:12:12
[00:57:11] make: *** [check] Error 1
[00:57:11] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1b27faee
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:026da824:start=1535570453026000231,finish=1535570453033880253,duration=7880022
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:02bec9a8
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:02009620
travis_time:start:02009620
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:11faae0c
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Manishearth
Copy link
Member

Unsure why that's unexpected, it's included in the error.

@flip1995
Copy link
Member Author

flip1995 commented Aug 29, 2018

Unsure why that's unexpected, it's included in the error.

Yes but the warning is emitted twice.

I couldn't find the reason for the double emitted warning on inner attributes today... I'm not sure what I'm doing different than in the UNKNOWN_LINTS case, which does not get linted twice.

@Manishearth
Copy link
Member

Manishearth commented Aug 29, 2018 via email

@flip1995
Copy link
Member Author

@bors r=Manishearth

@bors
Copy link
Contributor

bors commented Aug 29, 2018

📌 Commit 37b75435ef73bab70750ca0a4ba89a6445cafdb7 has been approved by Manishearth

@Manishearth
Copy link
Member

Done

@bors r+

@bors
Copy link
Contributor

bors commented Aug 31, 2018

📌 Commit 9cbe518 has been approved by Manishearth

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 31, 2018
@flip1995
Copy link
Member Author

Thanks!

@bors
Copy link
Contributor

bors commented Sep 1, 2018

⌛ Testing commit 9cbe518 with merge b7e4402...

bors added a commit that referenced this pull request Sep 1, 2018
Backwards compatibility for tool/clippy lints

cc #44690
cc rust-lang/rust-clippy#2977 (comment)

This is the next step towards `tool_lints`.

This makes Clippy lints still work without scoping, but will warn and suggest the new scoped name. This warning will only appear if the code is checked with Clippy itself.

There is still an issue with using the old lint name in inner attributes. For inner attributes the warning gets emitted twice. I'm currently not really sure why this happens, but will try to fix this ASAP.

r? @Manishearth
@bors
Copy link
Contributor

bors commented Sep 1, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Manishearth
Pushing b7e4402 to master...

@bors bors merged commit 9cbe518 into rust-lang:master Sep 1, 2018
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #53762!

Tested on commit b7e4402.
Direct link to PR: #53762

💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Sep 1, 2018
Tested on commit rust-lang/rust@b7e4402.
Direct link to PR: <rust-lang/rust#53762>

💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).
@bors bors mentioned this pull request Sep 1, 2018
@flip1995 flip1995 deleted the tool_lints branch September 1, 2018 13:53
flip1995 added a commit to flip1995/rust that referenced this pull request Sep 1, 2018
bors added a commit that referenced this pull request Sep 2, 2018
Fix of bug introduced by #53762 (tool_lints)

Before implementing backwards compat for tool lints, the `Tool` case when parsing cmdline lints was unreachable. This changed with #53762.

This fix is needed for rls test-pass. (@nrc)

r? @Manishearth
@flip1995 flip1995 mentioned this pull request Sep 3, 2018
@Arnavion
Copy link

What does "lint name {} is deprecated and may not have an effect in the future. Also cfg_attr(cargo-clippy) won't be necessary anymore" mean by the second sentence? The way it's worded makes it sound like I still need to keep the cfg_attr, but then why is it warning me about it now? If I'm supposed to remove it now, why does it say "won't be necessary" ?

@flip1995
Copy link
Member Author

It isn't necessary on nightly, with tool_lints feature turned on. But it is still necessary on stable, until tool_lints are stable (see rust-lang/rust-clippy#3159). Maybe the cfg_attr note was a bit premature.

@Arnavion
Copy link

Thanks for the link to that issue.

LL | #![cfg_attr(foo, warn(test_lint))]
| ^^^^^^^^^ help: change it to: `clippy::test_lint`
|
= note: #[warn(renamed_and_removed_lints)] on by default
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason that this warning is emitted twice is the note #[warn(renamed_and_removed_lints)] on by default, which only appears on the first occurrence of lint warnings. That doesn't effect the UNKNOWN_LINTS lint though.. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants