Add a "diagnostic item" scheme for lints referring to libstd items by oli-obk · Pull Request #60966 · rust-lang/rust

oli-obk · 2019-05-19T18:17:26Z

r? @Manishearth @rust-lang/wg-diagnostics

bors · 2019-05-23T02:01:50Z

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

oli-obk · 2019-05-25T16:19:33Z

@rust-lang/compiler Are you alright with adding this scheme (similar to lang items) for diagnostic items, allowing rustc lints and clippy lints to ask the compiler whether a specific DefId is a specific diagnostic item. It will place these into metadata (should have no effect beyond the single u32 offset value for crates without diagnostic items) and clippy will be able to overwrite the query to add its own clippy::diagnostic_item entries to it (so e.g. serde can use it, since we also lint serde specific things).

michaelwoerister · 2019-05-27T09:43:24Z

Is there an overview somewhere of what "diagnostic items" are and why the concept is needed?

oli-obk · 2019-05-27T10:25:14Z

The list of things that we want to make diagnostic items include (but are not limited to) https://github.com/rust-lang/rust-clippy/blob/abe139c7ac7d5a33a6edb60ba0d4eb4dfaf11103/clippy_lints/src/utils/paths.rs#L4-L115

Now our scheme (of using &[&str] to declare paths to items that we want to lint about) does work, but it has several flaws:

If an item is a core reexport, we need to check both the std path and the core path to ensure we can lint on no_core platforms, too.
If we want to fetch the DefId of a path (e.g. of a trait in order to check whether something implements said trait), we need to use this fun piece of code which walks the module tree to find the item and then get the DefId.
If libstd/libcore move a type into a private module and reexport it, the paths break and we need to fix them

Furthermore, some rustc diagnostics have to go through pretty printing types and matching on the result like in

rust/src/librustc_mir/borrow_check/move_errors.rs

Lines 317 to 322 in ed2a511

let orig_path_ty = format!(
"{:?}",
 original_path.ty(self.mir,self.infcx.tcx).ty,
);
let snippet = self.infcx.tcx.sess.source_map().span_to_snippet(span).unwrap();
let is_option = orig_path_ty.starts_with("std::option::Option");

in order to be able to report targeted diagnostics.

This PR will allow making all of these libcore/libstd specific errors and lints much simpler and reduce the complexity needed around them.

bors · 2019-05-27T18:35:49Z

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

eddyb · 2019-05-28T17:16:15Z

@oli-obk You can always look up the core/alloc path, it will have the DefId as its std reexport.
Also, the other flaws can be solved by just exposing a bit more of the name resolution machinery.
IMO, that's a better direction to go into, for non-language-mandated definitions.

But I am not strongly in favor of either, sometimes I'd rather clippy pattern-matched the definition, so if I made my own weird Option-like, I'd get similar warnings, but that's a bit far-fetched.

Manishearth · 2019-05-28T17:40:21Z

Note that lookups are pretty slow compared to this scheme.

I would like to expose name resolution machinery, though, we need that anyway for rustdoc.

But I am not strongly in favor of either, sometimes I'd rather clippy pattern-matched the definition, so if I made my own weird Option-like, I'd get similar warnings, but that's a bit far-fetched.

It would need to behave exactly the same way Option does in the contexts any given lint cares about, so pattern matching that ("does it have a function named X?" etc) gets much more expensive

eddyb · 2019-05-28T18:26:33Z

@Manishearth I agree about pattern-matching requiring custom logic, which has a time/effort cost to write in the first place - however, the cost to run a lookup/pattern-match is easily amortized by caching.

oli-obk · 2019-05-29T07:55:12Z

I'd rather clippy pattern-matched the definition, so if I made my own weird Option-like, I'd get similar warnings, but that's a bit far-fetched.

I want to add a scheme like #[clippy::diagnostic_item = "foo], and I'm very open to allowing multiple definitions of foo. I want opt-in for such lints for new types, because our lints are super handcrafted to the specific types they target and I don't see pattern-matching definitions coming any time soon or ever (as I foresee super many false positives that can't be eliminated in general).

Note that this change is done out of three reasons:

performance (ok caching could help, but any machinery will end up looking like this machinery at some point, or actually end up less comprehensible)
stability (less clippy breakage due to paths changing)
usability (we now have canonical names for language defined things and potentially even user defined things via clippy::diagnostic_item)
it just seems cleaner to have a clear, unambiguous way to refer to a specific item

tbh, the last three are why I did this

bors · 2019-06-02T14:51:11Z

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

oli-obk · 2019-06-06T11:03:13Z

Heh, one idea I just had: We can pattern match definitions and suggest that the user add #[clippy::diagnostic_item = "foo"] if the type is similar enough to foo. This way we don't forget anything and at the same time have opt-in

Dylan-DPC-zz · 2019-07-01T14:21:49Z

ping from triage @Manishearth @oli-obk any updates?

bors · 2019-08-29T23:02:41Z

📌 Commit 26e9990 has been approved by eddyb

rust-highfive · 2019-08-30T00:18:28Z

The job x86_64-gnu-llvm-6.0 of your PR failed (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.

2019-08-29T23:01:54.1845514Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-29T23:01:54.2091692Z ##[command]git config gc.auto 0
2019-08-29T23:01:54.2190774Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-29T23:01:54.2250919Z ##[command]git config --get-all http.proxy
2019-08-29T23:01:54.2403119Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/60966/merge:refs/remotes/pull/60966/merge
---
2019-08-30T00:07:01.7314116Z .................................................................................................... 1500/8977
2019-08-30T00:07:07.7847516Z .................................................................................................... 1600/8977
2019-08-30T00:07:21.2805495Z ................................................i...............i................................... 1700/8977
2019-08-30T00:07:30.0908712Z .................................................................................................... 1800/8977
2019-08-30T00:07:45.4216813Z .......................................iiiii........................................................ 1900/8977
2019-08-30T00:07:57.1008317Z .................................................................................................... 2100/8977
2019-08-30T00:07:59.9396081Z .................................................................................................... 2200/8977
2019-08-30T00:08:04.2679778Z .................................................................................................... 2300/8977
2019-08-30T00:08:12.3290601Z .................................................................................................... 2400/8977
---
2019-08-30T00:11:23.4917936Z ..........................i...............i......................................................... 4700/8977
2019-08-30T00:11:36.0649631Z .................................................................................................... 4800/8977
2019-08-30T00:11:42.6308005Z .................................................................................................... 4900/8977
2019-08-30T00:11:53.9955882Z .................................................................................................... 5000/8977
2019-08-30T00:11:59.9109538Z .......ii.ii........................................................................................ 5100/8977
2019-08-30T00:12:13.7625463Z .................................................................................................... 5300/8977
2019-08-30T00:12:22.5880589Z ......................................................................i............................. 5400/8977
2019-08-30T00:12:30.3669112Z .................................................................................................... 5500/8977
2019-08-30T00:12:37.6512494Z .................................................................................................... 5600/8977
2019-08-30T00:12:37.6512494Z .................................................................................................... 5600/8977
2019-08-30T00:12:48.5427306Z ................................................................ii...i..ii...........i.............. 5700/8977
2019-08-30T00:13:15.8298791Z .................................................................................................... 5900/8977
2019-08-30T00:13:23.6036720Z .................................................................................................... 6000/8977
2019-08-30T00:13:23.6036720Z .................................................................................................... 6000/8977
2019-08-30T00:13:30.1509285Z .................................................................i..ii.............................. 6100/8977
2019-08-30T00:14:00.6833375Z .................................................................................................... 6300/8977
2019-08-30T00:14:02.9808332Z ....................i............................................................................... 6400/8977
2019-08-30T00:14:05.3454175Z ............................................................................................i....... 6500/8977
2019-08-30T00:14:08.2976977Z .................................................................................................... 6600/8977
---
2019-08-30T00:18:23.0018331Z 2019-08-30T00:18:23.0019024Z ---- [ui] ui/tool-attributes/diagnostic_item.rs stdout ----
2019-08-30T00:18:23.0019099Z diff of stderr:
2019-08-30T00:18:23.0019158Z 2019-08-30T00:18:23.0019506Z - error[E0658]: diagnostic items compiler are internal support for linting and will never be stabilized
2019-08-30T00:18:23.0019711Z + error[E0658]: diagnostic items compiler internal support for linting
2019-08-30T00:18:23.0020048Z 3 |
2019-08-30T00:18:23.0020048Z 3 |
2019-08-30T00:18:23.0020093Z 4 LL | #[rustc_diagnostic_item = "foomp"]
2019-08-30T00:18:23.0020166Z 2019-08-30T00:18:23.0020212Z The actual stderr differed from the expected stderr.
2019-08-30T00:18:23.0020539Z Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/tool-attributes/diagnostic_item/diagnostic_item.stderr
2019-08-30T00:18:23.0020539Z Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/tool-attributes/diagnostic_item/diagnostic_item.stderr
2019-08-30T00:18:23.0020822Z To update references, rerun the tests and pass the `--bless` flag
2019-08-30T00:18:23.0021119Z To only update this specific test, also pass `--test-args tool-attributes/diagnostic_item.rs`
2019-08-30T00:18:23.0021201Z error: 1 errors occurred comparing output.
2019-08-30T00:18:23.0021263Z status: exit code: 1
2019-08-30T00:18:23.0021263Z status: exit code: 1
2019-08-30T00:18:23.0022033Z command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/tool-attributes/diagnostic_item.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/tool-attributes/diagnostic_item" "-Crpath" "-O" "-Cdebuginfo=0" "-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/tool-attributes/diagnostic_item/auxiliary" "-A" "unused"
2019-08-30T00:18:23.0022391Z ------------------------------------------
2019-08-30T00:18:23.0022426Z 2019-08-30T00:18:23.0022679Z ------------------------------------------
2019-08-30T00:18:23.0022731Z stderr:
2019-08-30T00:18:23.0022731Z stderr:
2019-08-30T00:18:23.0022959Z ------------------------------------------
2019-08-30T00:18:23.0023029Z error[E0658]: diagnostic items compiler internal support for linting
2019-08-30T00:18:23.0023357Z |
2019-08-30T00:18:23.0023357Z |
2019-08-30T00:18:23.0023423Z LL | #[rustc_diagnostic_item = "foomp"] //~ ERROR will never be stabilized
2019-08-30T00:18:23.0023513Z |
2019-08-30T00:18:23.0023513Z |
2019-08-30T00:18:23.0023917Z = note: for more information, see ***/issues/29642
2019-08-30T00:18:23.0023978Z = help: add `#![feature(rustc_attrs)]` to the crate attributes to enable
2019-08-30T00:18:23.0024010Z 2019-08-30T00:18:23.0024055Z error[E0601]: `main` function not found in crate `diagnostic_item`
2019-08-30T00:18:23.0024117Z |
2019-08-30T00:18:23.0024427Z = note: consider adding a `main` function to `/checkout/src/test/ui/tool-attributes/diagnostic_item.rs`
2019-08-30T00:18:23.0024531Z error: aborting due to 2 previous errors
2019-08-30T00:18:23.0024559Z 2019-08-30T00:18:23.0024605Z Some errors have detailed explanations: E0601, E0658.
2019-08-30T00:18:23.0025383Z For more information about an error, try `rustc --explain E0601`.
---
2019-08-30T00:18:23.0054033Z thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:536:22
2019-08-30T00:18:23.0054285Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
2019-08-30T00:18:23.0070503Z 2019-08-30T00:18:23.0070576Z 2019-08-30T00:18:23.0072252Z 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" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--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-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -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" "6.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"
2019-08-30T00:18:23.0072667Z 2019-08-30T00:18:23.0072700Z 2019-08-30T00:18:23.0079061Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-08-30T00:18:23.0079135Z Build completed unsuccessfully in 1:09:12
2019-08-30T00:18:23.0079135Z Build completed unsuccessfully in 1:09:12
2019-08-30T00:18:23.0133008Z == clock drift check ==
2019-08-30T00:18:23.0143618Z local time: Fri Aug 30 00:18:23 UTC 2019
2019-08-30T00:18:23.1004544Z network time: Fri, 30 Aug 2019 00:18:23 GMT
2019-08-30T00:18:23.1009822Z == end clock drift check ==
2019-08-30T00:18:23.9162448Z ##[error]Bash exited with code '1'.
2019-08-30T00:18:23.9205572Z ##[section]Starting: Checkout
2019-08-30T00:18:23.9207349Z ==============================================================================
2019-08-30T00:18:23.9207423Z Task : Get sources
2019-08-30T00:18:23.9207489Z Description : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

Centril · 2019-08-30T00:22:13Z

@bors r- PR builder is unhappy ^--

oli-obk · 2019-08-30T01:00:03Z

@bors r=eddyb

bors · 2019-08-30T01:00:04Z

📌 Commit 6978b94 has been approved by eddyb

bors · 2019-08-30T03:09:46Z

⌛ Testing commit 6978b94 with merge c7d4df0...

@Manishearth

Add a "diagnostic item" scheme for lints referring to libstd items fixes#39131 r? @Manishearth @rust-lang/wg-diagnostics

bors · 2019-08-30T06:49:08Z

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing c7d4df0 to master...

rust-highfive · 2019-08-30T06:50:21Z

📣 Toolstate changed by #60966!

Tested on commit c7d4df0.
Direct link to PR: #60966

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch @flip1995, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch @flip1995, @rust-lang/infra).
💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).

@Manishearth

Tested on commit rust-lang/rust@c7d4df0. Direct link to PR: <rust-lang/rust#60966> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch @flip1995, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch @flip1995, @rust-lang/infra). 💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).

Rustup to rust-lang/rust#60966 changelog: none

rust-highfive assigned Manishearth May 19, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2019

oli-obk commented May 19, 2019
View reviewed changes

Comment threadsrc/liballoc/vec.rs Outdated

petrochenkov self-assigned this May 19, 2019

oli-obk changed the title ~~WIP: Add a "diagnostic item" scheme for lints referring to libstd items~~Add a "diagnostic item" scheme for lints referring to libstd itemsMay 21, 2019

oli-obk marked this pull request as ready for review May 21, 2019 15:35

This comment has been minimized.
Sign in to view

petrochenkov removed their assignment May 21, 2019

This comment has been minimized.
Sign in to view

oli-obk force-pushed the diagnostic_items branch from ffea401 to 10d161fCompare May 22, 2019 15:03

This comment has been minimized.
Sign in to view

oli-obk force-pushed the diagnostic_items branch from 10d161f to e735e21Compare May 22, 2019 16:14

oli-obk force-pushed the diagnostic_items branch from e735e21 to 66df46fCompare May 25, 2019 15:53

oli-obk mentioned this pull request May 25, 2019
When encountering move error on an Option, suggest using as_ref#61147
Merged

oli-obk added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 25, 2019

Manishearth approved these changes May 25, 2019
View reviewed changes

estebank mentioned this pull request May 25, 2019
When suggesting borrow, remove useless clones #61143
Merged

oli-obk force-pushed the diagnostic_items branch from 66df46f to 90f9684Compare May 27, 2019 20:37

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, 2019

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 30, 2019

Update tests
6978b94

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 30, 2019

bors added a commit that referenced this pull request Aug 30, 2019
Auto merge of#60966- oli-obk:diagnostic_items, r=eddyb
c7d4df0
Add a "diagnostic item" scheme for lints referring to libstd items fixes#39131 r? @Manishearth @rust-lang/wg-diagnostics

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 30, 2019

bors merged commit 6978b94 into rust-lang:masterAug 30, 2019

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Aug 30, 2019
Rustup torust-lang/rust#60966
b0e8e05

bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 30, 2019
Auto merge of#4475- flip1995:rustup, r=phansch
ae72d31
Rustup to rust-lang/rust#60966 changelog: none

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Aug 30, 2019
Rustup torust-lang/rust#60966
3c11406

bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 30, 2019
Auto merge of#4475- flip1995:rustup, r=phansch
70e7d07
Rustup to rust-lang/rust#60966 changelog: none

phansch mentioned this pull request Sep 6, 2019
Simplify utils::match_def_path, removing a FIXME rust-lang/rust-clippy#4508
Merged

Centril mentioned this pull request Nov 4, 2019
Improve uninit/zeroed lint #66044
Merged

RalfJung mentioned this pull request Nov 4, 2019
Complete move to "diagnostic items" #66075
Closed
3 tasks

This was referenced Nov 8, 2019
Add IntoIterator impl for arrays by value (for [T; N]) #65819
Closed
invalid_value lint: use diagnostic items #66217
Merged

Manishearth mentioned this pull request Mar 30, 2020
Move our paths over to diagnostic items rust-lang/rust-clippy#5393
Closed

oli-obk deleted the diagnostic_items branch June 9, 2020 19:45

Uh oh!

Uh oh!

Conversation

oli-obk commented May 19, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented May 23, 2019

Uh oh!

oli-obk commented May 25, 2019

Uh oh!

michaelwoerister commented May 27, 2019

Uh oh!

oli-obk commented May 27, 2019

Uh oh!

bors commented May 27, 2019

Uh oh!

eddyb commented May 28, 2019

Uh oh!

Manishearth commented May 28, 2019

Uh oh!

eddyb commented May 28, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk commented May 29, 2019

Uh oh!

bors commented Jun 2, 2019

Uh oh!

oli-obk commented Jun 6, 2019

Uh oh!

Dylan-DPC-zz commented Jul 1, 2019

Uh oh!

bors commented Aug 29, 2019

Uh oh!

rust-highfive commented Aug 30, 2019

Uh oh!

Centril commented Aug 30, 2019

Uh oh!

oli-obk commented Aug 30, 2019

Uh oh!

bors commented Aug 30, 2019

Uh oh!

bors commented Aug 30, 2019

Uh oh!

bors commented Aug 30, 2019

Uh oh!

rust-highfive commented Aug 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

oli-obk commented May 19, 2019•
edited
Loading

eddyb commented May 28, 2019•
edited
Loading