Add a "diagnostic item" scheme for lints referring to libstd items by oli-obk · Pull Request #60966 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation52 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
WIP: Add a "diagnostic item" scheme for lints referring to libstd items Add a "diagnostic item" scheme for lints referring to libstd items
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #61044) made this pull request unmergeable. Please resolve the merge conflicts.
Relevant to the compiler team, which will review and decide on the PR/issue.
label
@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).
Is there an overview somewhere of what "diagnostic items" are and why the concept is needed?
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
corereexport, we need to check both thestdpath and thecorepath to ensure we can lint onno_coreplatforms, too. - If we want to fetch the
DefIdof 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 theDefId. - 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.
☔ The latest upstream changes (presumably #60827) made this pull request unmergeable. Please resolve the merge conflicts.
@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.
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
@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.
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
☔ The latest upstream changes (presumably #61276) made this pull request unmergeable. Please resolve the merge conflicts.
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
📌 Commit 26e9990 has been approved by eddyb
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
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🔞23.0018331Z
2019-08-30T00🔞23.0019024Z ---- [ui] ui/tool-attributes/diagnostic_item.rs stdout ----
2019-08-30T00🔞23.0019099Z diff of stderr:
2019-08-30T00🔞23.0019158Z
2019-08-30T00🔞23.0019506Z - error[E0658]: diagnostic items compiler are internal support for linting and will never be stabilized
2019-08-30T00🔞23.0019711Z + error[E0658]: diagnostic items compiler internal support for linting
2019-08-30T00🔞23.0020048Z 3 |
2019-08-30T00🔞23.0020048Z 3 |
2019-08-30T00🔞23.0020093Z 4 LL | #[rustc_diagnostic_item = "foomp"]
2019-08-30T00🔞23.0020166Z
2019-08-30T00🔞23.0020212Z The actual stderr differed from the expected stderr.
2019-08-30T00🔞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🔞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🔞23.0020822Z To update references, rerun the tests and pass the `--bless` flag
2019-08-30T00🔞23.0021119Z To only update this specific test, also pass `--test-args tool-attributes/diagnostic_item.rs`
2019-08-30T00🔞23.0021201Z error: 1 errors occurred comparing output.
2019-08-30T00🔞23.0021263Z status: exit code: 1
2019-08-30T00🔞23.0021263Z status: exit code: 1
2019-08-30T00🔞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🔞23.0022391Z ------------------------------------------
2019-08-30T00🔞23.0022426Z
2019-08-30T00🔞23.0022679Z ------------------------------------------
2019-08-30T00🔞23.0022731Z stderr:
2019-08-30T00🔞23.0022731Z stderr:
2019-08-30T00🔞23.0022959Z ------------------------------------------
2019-08-30T00🔞23.0023029Z error[E0658]: diagnostic items compiler internal support for linting
2019-08-30T00🔞23.0023357Z |
2019-08-30T00🔞23.0023357Z |
2019-08-30T00🔞23.0023423Z LL | #[rustc_diagnostic_item = "foomp"] //~ ERROR will never be stabilized
2019-08-30T00🔞23.0023513Z |
2019-08-30T00🔞23.0023513Z |
2019-08-30T00🔞23.0023917Z = note: for more information, see ***/issues/29642
2019-08-30T00🔞23.0023978Z = help: add `#![feature(rustc_attrs)]` to the crate attributes to enable
2019-08-30T00🔞23.0024010Z
2019-08-30T00🔞23.0024055Z error[E0601]: `main` function not found in crate `diagnostic_item`
2019-08-30T00🔞23.0024117Z |
2019-08-30T00🔞23.0024427Z = note: consider adding a `main` function to `/checkout/src/test/ui/tool-attributes/diagnostic_item.rs`
2019-08-30T00🔞23.0024531Z error: aborting due to 2 previous errors
2019-08-30T00🔞23.0024559Z
2019-08-30T00🔞23.0024605Z Some errors have detailed explanations: E0601, E0658.
2019-08-30T00🔞23.0025383Z For more information about an error, try `rustc --explain E0601`.
---
2019-08-30T00🔞23.0054033Z thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:536:22
2019-08-30T00🔞23.0054285Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
2019-08-30T00🔞23.0070503Z
2019-08-30T00🔞23.0070576Z
2019-08-30T00🔞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🔞23.0072667Z
2019-08-30T00🔞23.0072700Z
2019-08-30T00🔞23.0079061Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-08-30T00🔞23.0079135Z Build completed unsuccessfully in 1:09:12
2019-08-30T00🔞23.0079135Z Build completed unsuccessfully in 1:09:12
2019-08-30T00🔞23.0133008Z == clock drift check ==
2019-08-30T00🔞23.0143618Z local time: Fri Aug 30 00🔞23 UTC 2019
2019-08-30T00🔞23.1004544Z network time: Fri, 30 Aug 2019 00🔞23 GMT
2019-08-30T00🔞23.1009822Z == end clock drift check ==
2019-08-30T00🔞23.9162448Z ##[error]Bash exited with code '1'.
2019-08-30T00🔞23.9205572Z ##[section]Starting: Checkout
2019-08-30T00🔞23.9207349Z ==============================================================================
2019-08-30T00🔞23.9207423Z Task : Get sources
2019-08-30T00🔞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)
@bors r- PR builder is unhappy ^--
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
📌 Commit 6978b94 has been approved by eddyb
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
bors added a commit that referenced this pull request
rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request
bors added a commit to rust-lang/rust-clippy that referenced this pull request
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request
bors added a commit to rust-lang/rust-clippy that referenced this pull request
This was referenced
Nov 8, 2019
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.