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

dead-code lint: say "constructed" for structs #52332

Conversation

zackmdavis
Copy link
Member

@zackmdavis zackmdavis commented Jul 13, 2018

Respectively.

This is a sequel to November 2017's #46103 / 1a9dc2e. It had been
reported (more than once—at least #19140, #44083, and #44565) that the
"never used" language was confusing for enum variants that were "used"
as match patterns, so the wording was changed to say never "constructed"
specifically for enum variants. More recently, the same issue was raised
for structs (#52325). It seems consistent to say "constructed" here,
too, for the same reasons.

While we're here, we can also use more specific word "called" for unused
functions and methods. (We declined to do this in #46103, but the
rationale given in the commit message doesn't actually make sense.)

This resolves #52325.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(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 Jul 13, 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.
travis_time:start:test_ui-fulldeps
Check compiletest suite=ui-fulldeps mode=ui (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:51:42] 
[00:51:42] running 21 tests
[00:51:58] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:498:22
[00:51:58] ......F..............
[00:51:58] 
[00:51:58] ---- [ui] ui-fulldeps/lint-plugin-cmdline-allow.rs stdout ----
[00:51:58] diff of stderr:
[00:51:58] 
[00:51:58] 
[00:51:58] - warning: function is never used: `lintme`
[00:51:58] + warning: function is never called: `lintme`
[00:51:58] 3    |
[00:51:58] 3    |
[00:51:58] 4 LL | fn lintme() { }
[00:51:58] 
[00:51:58] The actual stderr differed from the expected stderr.
[00:51:58] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/lint-plugin-cmdline-allow/lint-plugin-cmdline-allow.stderr
[00:51:58] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/lint-plugin-cmdline-allow/lint-plugin-cmdline-allow.stderr
[00:51:58] To update references, rerun the tests and pass the `--bless` flag
[00:51:58] To only update this specific test, also pass `--test-args lint-plugin-cmdline-allow.rs`
[00:51:58] error: 1 errors occurred comparing output.
[00:51:58] status: exit code: 0
[00:51:58] status: exit code: 0
[00:51:58] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui-fulldeps/lint-plugin-cmdline-allow.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-plugin-cmdline-allow/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-A" "test-lint" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui-fulldeps/lint-plugin-cmdline-allow/auxiliary" "-A" "unused"
[00:51:58] ------------------------------------------
[00:51:58] 
[00:51:58] ------------------------------------------
[00:51:58] stderr:
[00:51:58] stderr:
[00:51:58] ------------------------------------------
[00:51:58] {"message":"function is never called: `lintme`","code":{"code":"dead_code","explanation":null},"level":"warning","spans":[{"file_name":"/checkout/src/test/ui-fulldeps/lint-plugin-cmdline-allow.rs","byte_start":628,"byte_end":639,"line_start":20,"line_end":20,"column_start":1,"column_end":12,"is_primary":true,"text":[{"text":"fn lintme() { }","highlight_start":1,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"lint level defined here","code":null,"level":"note","spans":[{"file_name":"/checkout/src/test/ui-fulldeps/lint-plugin-cmdline-allow.rs","byte_start":589,"byte_end":595,"line_start":17,"line_end":17,"column_start":9,"column_end":15,"is_primary":true,"text":[{"text":"#![warn(unused)]","highlight_start":9,"highlight_end":15}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":null},{"message":"#[warn(dead_code)] implied by #[warn(unused)]","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"warning: function is never called: `lintme`\n  --> /checkout/src/test/ui-fulldeps/lint-plugin-cmdline-allow.rs:20:1\n   |\nLL | fn lintme() { }\n   | ^^^^^^^^^^^\n   |\nnote: lint level defined here\n  --> /checkout/src/test/ui-fulldeps/lint-plugin-cmdline-allow.rs:17:9\n   |\nLL | #![warn(unused)]\n   |         ^^^^^^\n   = note: #[warn(dead_code)] implied by #[warn(unused)]\n\n"}
[00:51:58] ------------------------------------------
[00:51:58] 
[00:51:58] thread '[ui] ui-fulldeps/lint-plugin-cmdline-allow.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3139:9
[00:51:58] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:51:58] test result: FAILED. 20 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
[00:51:58] 
[00:51:58] 
[00:51:58] 
[00:51:58] 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:51:58] 
[00:51:58] 
[00:51:58] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:51:58] Build completed unsuccessfully in 0:10:07
[00:51:58] Build completed unsuccessfully in 0:10:07
[00:51:58] make: *** [check] Error 1
[00:51:58] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:28c9e81c
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

@zackmdavis zackmdavis force-pushed the dead_code_lint_should_say_2_electric_boogaloo branch from acccb8d to 8cc4fd6 Compare July 13, 2018 04:49
@pnkfelix
Copy link
Member

pnkfelix commented Jul 13, 2018

I agree with this PR's change of "constructed" instead of "used".

As for the broader change... I didn't mind the previous terminology of "use" instead of where it says "call" here, but I don't particularly care either way. If people like "call" then fine. I guess I'll just let this land and if someone complains we can change it back.

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 13, 2018

📌 Commit 8cc4fd6 has been approved by pnkfelix

@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 Jul 13, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 13, 2018
…y_2_electric_boogaloo, r=pnkfelix

dead-code lint: say "constructed", "called" for structs, functions

Respectively.

This is a sequel to November 2017's rust-lang#46103 / 1a9dc2e. It had been
reported (more than once—at least rust-lang#19140, rust-lang#44083, and rust-lang#44565) that the
"never used" language was confusing for enum variants that were "used"
as match patterns, so the wording was changed to say never "constructed"
specifically for enum variants. More recently, the same issue was raised
for structs (rust-lang#52325). It seems consistent to say "constructed" here,
too, for the same reasons.

While we're here, we can also use more specific word "called" for unused
functions and methods. (We declined to do this in rust-lang#46103, but the
rationale given in the commit message doesn't actually make sense.)

This resolves rust-lang#52325.
bors added a commit that referenced this pull request Jul 13, 2018
Rollup of 16 pull requests

Successful merges:

 - #51962 (Provide llvm-strip in llvm-tools component)
 - #52003 (Implement `Option::replace` in the core library)
 - #52156 (Update std::ascii::ASCIIExt deprecation notes)
 - #52242 (NLL: Suggest `ref mut` and `&mut self`)
 - #52244 (Don't display default generic parameters in diagnostics that compare types)
 - #52290 (Deny bare trait objects in src/librustc_save_analysis)
 - #52293 (Deny bare trait objects in librustc_typeck)
 - #52299 (Deny bare trait objects in src/libserialize)
 - #52300 (Deny bare trait objects in librustc_target and libtest)
 - #52302 (Deny bare trait objects in the rest of rust)
 - #52310 (Backport 1.27.1 release notes to master)
 - #52314 (Fix ICE when using a pointer cast as array size)
 - #52315 (Resolve FIXME(#27942))
 - #52316 (task: remove wrong comments about non-existent LocalWake trait)
 - #52322 (Update llvm-rebuild-trigger in light of LLVM 7 upgrade)
 - #52332 (dead-code lint: say "constructed", "called" for structs, functions)

Failed merges:

r? @ghost
@pietroalbini
Copy link
Member

@bors r-
This is breaking Cargo's test suite :(

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

@zackmdavis the simplest solution might to be continue using the word "used" instead of "called" for functions, since it seems like cargo's test suite has that phrasing built into their current test suite.

(Otherwise I don't know how easy it would be to update cargo's test suite in sync with making the change to the wording proposed here.)

@zackmdavis
Copy link
Member Author

the simplest solution might to be continue using the word "used" instead of "called" for functions

Yes, have to leave now, but I can revise this PR tonight. Updating Cargo's tests wouldn't be difficult, but I'm not actually super-passionate about "called"

@zackmdavis
Copy link
Member Author

tonight

(Okay, maybe not tonight tonight, but soon)

This is a sequel to November 2017's rust-lang#46103 / 1a9dc2e. It had been
reported (more than once—at least rust-lang#19140, rust-lang#44083, and rust-lang#44565) that the
"never used" language was confusing for enum variants that were "used"
as match patterns, so the wording was changed to say never
"constructed" specifically for enum variants. More recently, the same
issue was raised for structs (rust-lang#52325). It seems consistent to say
"constructed" here, too, for the same reasons.

We considered using more specific word "called" for unused functions
and methods (while we declined to do this in rust-lang#46103, the rationale
given in the commit message doesn't actually make sense), but it turns
out that Cargo's test suite expects the "never used" message, and
maybe we don't care enough even to make a Cargo PR over such a petty
and subjective wording change.

This resolves rust-lang#52325.
@zackmdavis zackmdavis force-pushed the dead_code_lint_should_say_2_electric_boogaloo branch from 8cc4fd6 to 6c50ee5 Compare July 22, 2018 01:38
@zackmdavis zackmdavis changed the title dead-code lint: say "constructed", "called" for structs, functions dead-code lint: say "constructed" for structs Jul 22, 2018
@zackmdavis
Copy link
Member Author

@pnkfelix updated 🏁

@pietroalbini pietroalbini 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 (such as code changes or more information) from the author. labels Jul 30, 2018
@pietroalbini
Copy link
Member

Ping from triage @pnkfelix! This PR needs your review.

@pnkfelix
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 31, 2018

📌 Commit 6c50ee5 has been approved by pnkfelix

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 1, 2018
…y_2_electric_boogaloo, r=pnkfelix

dead-code lint: say "constructed" for structs

Respectively.

This is a sequel to November 2017's rust-lang#46103 / 1a9dc2e. It had been
reported (more than once—at least rust-lang#19140, rust-lang#44083, and rust-lang#44565) that the
"never used" language was confusing for enum variants that were "used"
as match patterns, so the wording was changed to say never "constructed"
specifically for enum variants. More recently, the same issue was raised
for structs (rust-lang#52325). It seems consistent to say "constructed" here,
too, for the same reasons.

~~While we're here, we can also use more specific word "called" for unused
functions and methods. (We declined to do this in rust-lang#46103, but the
rationale given in the commit message doesn't actually make sense.)~~

This resolves rust-lang#52325.
bors added a commit that referenced this pull request Aug 1, 2018
Rollup of 31 pull requests

Successful merges:

 - #52332 (dead-code lint: say "constructed" for structs)
 - #52340 (Document From trait implementations for OsStr, OsString, CString, and CStr)
 - #52628 (Cleanup some rustdoc code)
 - #52732 (Remove unstable and deprecated APIs)
 - #52745 (Update clippy to latest master)
 - #52756 (rustc: Disallow machine applicability in foreign macros)
 - #52771 (Clarify thread::park semantics)
 - #52810 ([NLL] Don't make "fake" match variables mutable)
 - #52821 (pretty print for std::collections::vecdeque)
 - #52822 (Fix From<LocalWaker>)
 - #52824 (Fix -Wpessimizing-move warnings in rustllvm/PassWrapper)
 - #52831 (remove references to AUTHORS.txt file)
 - #52835 (Fix Alias intra doc ICE)
 - #52842 (update comment)
 - #52846 (Add timeout to use of `curl` in bootstrap.py.)
 - #52851 (Make the tool_lints actually usable)
 - #52853 (Improve bootstrap help on stages)
 - #52859 (Use Vec::extend in SmallVec::extend when applicable)
 - #52861 (Add targets for HermitCore (https://hermitcore.org) to the Rust compiler and port libstd to it.)
 - #52867 (releases.md: fix 2 typos)
 - #52870 (Implement Unpin for FutureObj and LocalFutureObj)
 - #52876 (run-pass/const-endianness: negate before to_le())
 - #52878 (Fix wrong issue number in the test name)
 - #52883 (Include lifetime in mutability suggestion in NLL messages)
 - #52904 (NLL: sort diagnostics by span)
 - #52905 (Fix a typo in unsize.rs)
 - #52907 (NLL: On "cannot move out of type" error, print original before rewrite)
 - #52908 (Use SetLenOnDrop in Vec::truncate())
 - #52914 (Only run the sparc-abi test on sparc)
 - #52918 (Backport 1.27.2 release notes)
 - #52929 (Update compatibility note for 1.28.0 to be correct)

Failed merges:

 - #52758 (Cleanup for librustc::session)
 - #52799 (Use BitVector for global sets of AttrId)

r? @ghost
@kennytm
Copy link
Member

kennytm commented Aug 1, 2018

@bors r- rollup-

RLS's test cases need to be updated as well. It contained 3 tests which still expects the old error messages. See #52931 (comment)

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

↑ (This was sufficiently demoralizing and #52325 is sufficiently low-priority that I'm just going to close this for now to get it out of the way.)

@zackmdavis zackmdavis closed this Aug 5, 2018
@Mark-Simulacrum
Copy link
Member

Hm, I don't quite follow -- why are we blocking this on RLS test cases? We should be able to land things in tree even if they break RLS, no?

@kennytm
Copy link
Member

kennytm commented Aug 5, 2018

This PR was r+'ed during the beta week unfortunately 🙁.

@Mark-Simulacrum
Copy link
Member

@bors r=pnkfelix

We should see if there's something we can do to provide better feedback for that case.

@bors
Copy link
Contributor

bors commented Aug 5, 2018

📌 Commit 6c50ee5 has been approved by pnkfelix

@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 5, 2018
@kennytm
Copy link
Member

kennytm commented Aug 5, 2018

@bors rollup since we're outside the beta week now.

@bors
Copy link
Contributor

bors commented Aug 6, 2018

⌛ Testing commit 6c50ee5 with merge 7c98d2e...

bors added a commit that referenced this pull request Aug 6, 2018
…c_boogaloo, r=pnkfelix

dead-code lint: say "constructed" for structs

Respectively.

This is a sequel to November 2017's #46103 / 1a9dc2e. It had been
reported (more than once—at least #19140, #44083, and #44565) that the
"never used" language was confusing for enum variants that were "used"
as match patterns, so the wording was changed to say never "constructed"
specifically for enum variants. More recently, the same issue was raised
for structs (#52325). It seems consistent to say "constructed" here,
too, for the same reasons.

~~While we're here, we can also use more specific word "called" for unused
functions and methods. (We declined to do this in #46103, but the
rationale given in the commit message doesn't actually make sense.)~~

This resolves #52325.
@bors
Copy link
Contributor

bors commented Aug 6, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing 7c98d2e to master...

@bors bors merged commit 6c50ee5 into rust-lang:master Aug 6, 2018
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #52332!

Tested on commit 7c98d2e.
Direct link to PR: #52332

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

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Aug 6, 2018
Tested on commit rust-lang/rust@7c98d2e.
Direct link to PR: <rust-lang/rust#52332>

💔 rls on windows: test-pass → test-fail (cc @nrc, @rust-lang/infra).
💔 rls on linux: test-pass → test-fail (cc @nrc, @rust-lang/infra).
nrc added a commit to rust-lang/rls that referenced this pull request Aug 7, 2018
bors added a commit that referenced this pull request Aug 8, 2018
Update RLS, Rustfmt, and Clippy

Fixes bustage due to #52332

r? @kennytm
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.

Incorrect 'never used' warning for struct when only use is in pattern match
7 participants