Rustup #7604
Merged
Rustup #7604
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 #86996
Update Clippy r? `@Manishearth`
- 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
Uplift the invalid_atomic_ordering lint from clippy to rustc This is mostly just a rebase of rust-lang/rust#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/rust#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).
Fix clippy::collapsible_match with let expressions This fixes rust-lang#7575 which is a regression from #80357. I am fixing the bug here instead of in the clippy repo (if that's okay) because a) the regression has not been synced yet and b) I would like to land the fix on nightly asap. The fix is basically to re-generalize `match` and `if let` for the lint implementation (they were split because `if let` no longer desugars to `match` in the HIR). Also fixes rust-lang#7586 and fixes rust-lang#7591 cc `@rust-lang/clippy` `@xFrednet` do you want to review this?
cleanup: `Span::new` -> `Span::with_lo` Extracted from rust-lang/rust#84373 as suggested in rust-lang/rust#84373 (comment). It turned out less useful then I expected, but anyway. r? `@cjgillot` `@bors` rollup
Fix typos “a”→“an” Fix typos in comments; found using a regex to find some easy instance of incorrect usage of a vs. an. While automation was used to find these, every change was checked manually. Changes in submodules get separate PRs: * rust-lang/stdarch#1201 * rust-lang/cargo#9821 * rust-lang/miri#1874 * rust-lang/rls#1746 * rust-lang/rust-analyzer#9984 _folks @ rust-analyzer are fast at merging…_ * rust-lang/rust-analyzer#9985 * rust-lang/rust-analyzer#9987 * rust-lang/rust-analyzer#9989 _For `clippy`, I don’t know if the changes should better better be moved to a PR to the original repo._ <hr> This has some overlap with #88226, but neither is a strict superset of the other. If you want multiple commits, I can split it up; in that case, make sure to suggest a criterion for splitting.
Get piece unchecked in `write` We already use specialized `zip`, but it seems like we can do a little better by not checking `pieces` length at all. `Arguments` constructors are now unsafe. So the `format_args!` expansion now includes an `unsafe` block. <details> <summary>Local Bench Diff</summary> ```text name before ns/iter after ns/iter diff ns/iter diff % speedup fmt::write_str_macro1 22,967 19,718 -3,249 -14.15% x 1.16 fmt::write_str_macro2 35,527 32,654 -2,873 -8.09% x 1.09 fmt::write_str_macro_debug 571,953 575,973 4,020 0.70% x 0.99 fmt::write_str_ref 9,579 9,459 -120 -1.25% x 1.01 fmt::write_str_value 9,573 9,572 -1 -0.01% x 1.00 fmt::write_u128_max 176 173 -3 -1.70% x 1.02 fmt::write_u128_min 138 134 -4 -2.90% x 1.03 fmt::write_u64_max 139 136 -3 -2.16% x 1.02 fmt::write_u64_min 129 135 6 4.65% x 0.96 fmt::write_vec_macro1 24,401 22,273 -2,128 -8.72% x 1.10 fmt::write_vec_macro2 37,096 35,602 -1,494 -4.03% x 1.04 fmt::write_vec_macro_debug 588,291 589,575 1,284 0.22% x 1.00 fmt::write_vec_ref 9,568 9,732 164 1.71% x 0.98 fmt::write_vec_value 9,516 9,625 109 1.15% x 0.99 ``` </details>
xFrednet
reviewed
Aug 27, 2021
There was a problem hiding this comment.
30/103 files reviewed up until: clippy_lints/src/manual_map.rs
I really like the fact that we replaced the box syntax with Box::new(). I still have to check if the documentation was also adjusted with this change.
Some comments are journey entries or todos for me, I'll resolve all unimportant ones, when I'm done 🙃
|
Yay it's green! |
xFrednet
reviewed
Aug 27, 2021
|
I've started the review and mostly skipped changes related to the new if let expression, as they have already been reviewed. I'll finish my review tomorrow. Is there still something blocking the merged? 🙃 |
|
☔ The latest upstream changes (presumably #7573) made this pull request unmergeable. Please resolve the merge conflicts. |
|
It seems like rust-lang/rust#88572 fixes the issue and a revert won't be necessary, so I'm going through with this rustup now. |
Except for the missing docs ones
|
@bors r+ |
|
📌 Commit 01b17af has been approved by |
|
@bors p=1 |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
This was referenced Sep 2, 2021
Merged
|
Thanks for merging. I just managed to push it up and then had to jump on a plane. I'll do the Rust side of the sync first thing tomorrow. |
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.
I'll write some TODOs later. This time I'll need a review for this Rustup
r? @ghost (for now)
changelog: none