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

Add a "diagnostic item" scheme for lints referring to libstd items #60966

Merged
merged 2 commits into from Aug 30, 2019

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 19, 2019

fixes #39131

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2019
src/liballoc/vec.rs Outdated Show resolved Hide resolved
@petrochenkov petrochenkov self-assigned this May 19, 2019
@oli-obk 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 items May 21, 2019
@oli-obk oli-obk marked this pull request as ready for review May 21, 2019 15:35
@rust-highfive

This comment has been minimized.

@petrochenkov petrochenkov removed their assignment May 21, 2019
@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 23, 2019

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

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

oli-obk commented May 25, 2019

@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
Copy link
Member

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

@oli-obk
Copy link
Contributor Author

oli-obk commented May 27, 2019

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

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
Copy link
Contributor

bors commented May 27, 2019

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

@eddyb
Copy link
Member

eddyb commented May 28, 2019

@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
Copy link
Member

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
Copy link
Member

eddyb commented May 28, 2019

@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
Copy link
Contributor Author

oli-obk commented May 29, 2019

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
Copy link
Contributor

bors commented Jun 2, 2019

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

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 6, 2019

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
Copy link

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

@bors
Copy link
Contributor

bors commented Aug 29, 2019

📌 Commit 26e9990 has been approved by eddyb

@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, 2019
@rust-highfive
Copy link
Collaborator

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
Copy link
Contributor

Centril commented Aug 30, 2019

@bors r- PR builder is unhappy ^--

@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 30, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 30, 2019

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Aug 30, 2019

📌 Commit 6978b94 has been approved by eddyb

@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 30, 2019
@bors
Copy link
Contributor

bors commented Aug 30, 2019

⌛ Testing commit 6978b94 with merge c7d4df0...

bors added a commit that referenced this pull request Aug 30, 2019
Add a "diagnostic item" scheme for lints referring to libstd items

fixes #39131

r? @Manishearth @rust-lang/wg-diagnostics
@bors
Copy link
Contributor

bors commented Aug 30, 2019

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 30, 2019
@bors bors merged commit 6978b94 into rust-lang:master Aug 30, 2019
@rust-highfive
Copy link
Collaborator

📣 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).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Aug 30, 2019
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).
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Aug 30, 2019
bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 30, 2019
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Aug 30, 2019
bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 30, 2019
@oli-obk oli-obk deleted the diagnostic_items branch June 9, 2020 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "diagnostic items" which behave like lang items