Update Clippy #88615
Merged
Update Clippy #88615
Conversation
This file contains 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
Link to edition guide instead of issues for 2021 lints. This changes the 2021 lints to not link to github issues, but to the edition guide instead. Fixes rust-lang#86996
Addressed PR reviews regarding code style
…type-lint, r=camsteffen Use `avoid-breaking-exported-api` configuration in types module This PR empowers our lovely `avoid-breaking-exported-api` configuration value to also influence the emission of lints inside the `types` module. (That's pretty much it, not really a change worthy of writing a fairy tale about. Don't get me wrong, I would love to write a short one, but I sadly need to study now). --- Closes: rust-lang/rust-clippy#7489 changelog: The `avoid-breaking-exported-api` configuration now also works for [`box_vec`], [`redundant_allocation`], [`rc_buffer`], [`vec_box`], [`option_option`], [`linkedlist`], [`rc_mutex`] changelog: [`rc_mutex`]: update the lint message to comply with the normal format --- r? `@camsteffen,` as you implemented the configuration value cc: `@flip1995,` as we've discussed this change in rust-lang/rust-clippy#7308
Update Clippy r? `@Manishearth`
…rednet Fix false positive on `filter_next` fixes rust-lang#7561 changelog: Fix false positive on [`filter_next`] when a method does not implement `Iterator`
Locals which can be partially moved created within the to-be-created closure shouldn't block the use of a closure
In some cases check if a borrow made in the scrutinee expression would prevent creating the closure used by `map`
* Captures by sub closures are now considered * Copy types are correctly borrowed by reference when their value is used * Fields are no longer automatically borrowed by value * Bindings in `match` and `let` patterns are now checked to determine how a local is captured
Downgrade option_if_let_else to nursery I believe that this lint's loose understanding of ownership (rust-lang#5822, rust-lang#6737) makes it unsuitable to be enabled by default in its current state, even as a pedantic lint. Additionally the lint has known problems with type inference (rust-lang#6137), though I may be willing to consider this a non-blocker in isolation if it weren't for the ownership false positives. A fourth false positive involving const fn: rust-lang#7567. But on top of these, for me the biggest issue is I basically fully agree with rust-lang/rust-clippy#6137 (comment). In my experience this lint universally makes code worse even when the resulting code does compile. --- changelog: remove [`option_if_let_else`] from default set of enabled lints
but skips directories containing CACHEDIR.TAG e.g. the target/ dir
…g, r=camsteffen lintcheck always copies in a fresh crate when provided with a crate path changelog: none When lintcheck is run on local crates it copies the crate to `target/lintcheck/sources/crate_name` on the first run only. Then in subsequent runs of lintcheck it reuses this same copy. This caching behaviour makes sense when dealing with immutable crates.io releases and git commits. However it is quite surprising that the changes to my local crate are not used when I run lintcheck. To fix this I removed the copy, instead clippy runs directly off the provided crate folder. I have tested this and have not observed any negative effects from doing this. But maybe i'm missing something as im not familiar with clippy! Alternatively we could make it copy the entire crate every run, but that seems problematic to me as multi-gigabyte target folders will take a long time to copy and wear down SSDs for developers who frequently run lintcheck.
Manual map 7413 fixes: rust-lang#7413 This only fixes the specific problem from rust-lang#7413, not the general case. The full fix requires interacting with the borrow checker to determine the lifetime of all the borrows made in the function. I'll open an issue about it later. changelog: Don't suggest using `map` when the option is borrowed in the match, and also consumed in the arm. changelog: Locals declared within the would-be closure will not prevent the closure from being suggested in `manual_map` and `map_entry`
- Deprecate clippy::invalid_atomic_ordering - Use rustc_diagnostic_item for the orderings in the invalid_atomic_ordering lint - Reduce code duplication - Give up on making enum variants diagnostic items and just look for `Ordering` instead I ran into tons of trouble with this because apparently the change to store HIR attrs in a side table also gave the DefIds of the constructor instead of the variant itself. So I had to change `matches_ordering` to also check the grandparent of the defid as well. - Rename `atomic_ordering_x` symbols to just the name of the variant - Fix typos in checks - there were a few places that said "may not be Release" in the diagnostic but actually checked for SeqCst in the lint. - Make constant items const - Use fewer diagnostic items - Only look at arguments after making sure the method matches This prevents an ICE when there aren't enough arguments. - Ignore trait methods - Only check Ctors instead of going through `qpath_res` The functions take values, so this couldn't ever be anything else. - Add if_chain to allowed dependencies - Fix grammar - Remove unnecessary allow
…eywiser Uplift the invalid_atomic_ordering lint from clippy to rustc This is mostly just a rebase of rust-lang#79654; I've copy/pasted the text from that PR below. r? `@lcnr` since you reviewed the last one, but feel free to reassign. --- This is an implementation of rust-lang/compiler-team#390. As mentioned, in general this turns an unconditional runtime panic into a (compile time) lint failure. It has no false positives, and the only false negatives I'm aware of are if `Ordering` isn't specified directly and is comes from an argument/constant/whatever. As a result of it having no false positives, and the alternative always being strictly wrong, it's on as deny by default. This seems right. In the [zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Uplift.20the.20.60invalid_atomic_ordering.60.20lint.20from.20clippy/near/218483957) `@joshtriplett` suggested that lang team should FCP this before landing it. Perhaps libs team cares too? --- Some notes on the code for reviewers / others below ## Changes from clippy The code is changed from [the implementation in clippy](https://github.com/rust-lang/rust-clippy/blob/68cf94f6a66e47234e3adefc6dfbe806cd6ad164/clippy_lints/src/atomic_ordering.rs) in the following ways: 1. Uses `Symbols` and `rustc_diagnostic_item`s instead of string literals. - It's possible I should have just invoked Symbol::intern for some of these instead? Seems better to use symbol, but it did require adding several. 2. The functions are moved to static methods inside the lint struct, as a way to namespace them. - There's a lot of other code in that file — which I picked as the location for this lint because `@jyn514` told me that seemed reasonable. 3. Supports unstable AtomicU128/AtomicI128. - I did this because it was almost easier to support them than not — not supporting them would have (ideally) required finding a way not to give them a `rustc_diagnostic_item`, which would have complicated an already big macro. - These don't have tests since I wasn't sure if/how I should make tests conditional on whether or not the target has the atomic... This is to a certain extent an issue of 64bit atomics too, but 128-bit atomics are much less common. Regardless, the existing tests should be *more* than thorough enough here. 4. Minor changes like: - grammar tweaks ("loads cannot have `Release` **and** `AcqRel` ordering" => "loads cannot have `Release` **or** `AcqRel` ordering") - function renames (`match_ordering_def_path` => `matches_ordering_def_path`), - avoiding clippy-specific helper methods that don't exist in rustc_lint and didn't seem worth adding for this case (for example `cx.struct_span_lint` vs clippy's `span_lint_and_help` helper). ## Potential issues (This is just about the code in this PR, not conceptual issues with the lint or anything) 1. I'm not sure if I should have used a diagnostic item for `Ordering` and its variants (I couldn't figure out how really, so if I should do this some pointers would be appreciated). - It seems possible that failing to do this might possibly mean there are more cases this lint would miss, but I don't really know how `match_def_path` works and if it has any pitfalls like that, so maybe not. 2. I *think* I deprecated the lint in clippy (CC `@flip1995` who asked to be notified about clippy changes in the future in [this comment](rust-lang#75671 (comment))) but I'm not sure if I need to do anything else there. - I'm kind of hoping CI will catch if I missed anything, since `x.py test src/tools/clippy` fails with a lot of errors with and without my changes (and is probably a nonsense command regardless). Running `cargo test` from src/tools/clippy also fails with unrelated errors that seem like refactorings that didnt update clippy? So, honestly no clue. 3. I wasn't sure if the description/example I gave good. Hopefully it is. The example is less thorough than the one from clippy here: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_atomic_ordering. Let me know if/how I should change it if it needs changing. 4. It pulls in the `if_chain` crate. This crate was already used in clippy, and seems like it's used elsewhere in rustc, but I'm willing to rewrite it to not use this if needed (I'd prefer not to, all things being equal).
Add a scenario where `manual_flatten` is triggered when match expression will still be used after the match in `if let`.
`manual_flatten` should not trigger when match expression in `if let` is going to be used.
Check expr usage for `manual_flatten` Fixes rust-lang#6784 Fixes rust-lang#7538 `manual_flatten` should not trigger when `if let` match expression will be used. changelog: [`manual_flatten`] checks for expr usage after `if let`
Rustup r? `@ghost` changelog: none
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Use binary-dep-depinfo to resolve UI test dependencies Closes rust-lang#7343 Closes rust-lang#6809 Closes rust-lang#3643 changelog: none r? `@flip1995` cc `@Jarcho`
|
@Manishearth This is now ready. Regular sync day is tomorrow, but I would just go ahead and sync it now. |
|
@bors r+ |
|
📌 Commit fe247b4 has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=1 |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (626649f): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
Sep 28, 2021
Update Clippy r? `@Manishearth`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? @Manishearth