Uplift the invalid_atomic_ordering lint from clippy to rustc by jyn514 · Pull Request #84039 · rust-lang/rust

jyn514 · 2021-04-09T16:10:44Z

This is mostly just a rebase of #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 @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 in the following ways:

Uses Symbols and rustc_diagnostic_items 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.
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.
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.
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)

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.
I think I deprecated the lint in clippy (CC @flip1995 who asked to be notified about clippy changes in the future in this 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.
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.
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).

rust-highfive · 2021-04-09T16:10:47Z

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

jyn514 · 2021-04-09T16:11:18Z

cc @thomcc

jyn514 · 2021-04-09T16:13:34Z

Oh also the clippy deprecation will conflict with rust-lang/rust-clippy#7056; I expect there will be a subtree sync before fcp finishes though so it shouldn't be a big deal.

src/tools/clippy/clippy_lints/src/lib.rs

jyn514 · 2021-04-09T17:46:18Z

I am not sure why CI is failing :( I think it has something to do with Acquire being a Ctor instead of the variant itself, but I'm not sure what's going wrong. Some debugging in https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/How.20do.20I.20get.20the.20ctor.20of.20an.20enum.20variant.3F.

thomcc · 2021-04-10T02:50:12Z

Hey, thanks for taking this over and carrying it over the finish line. I had struggled a few times with the rebase, since I'm mostly unfamiliar with rustc and clippy, and generally didn't have the time to dedicate to it at the moment.

Still, will be super thrilled to see it land, it will make atomic stuff way easier to teach, if nothing else!

camsteffen · 2021-04-10T22:14:51Z

compiler/rustc_lint/src/types.rs

This is a growing pattern in Clippy that is now moving to rustc. Should we have a tcx.get_diagnostic_name(def_id)?

Or, alternatively...

if cx.tcx.is_diagnostic_item(sym::atomic_mod, cx.tcx.parent(did)) { if matches!(cx.tcx.item_name(did), sym::AtomicU8 | sym::AtomicBool | ..) {

Yes, we should IMO

It already exists: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.TyCtxt.html#method.get_diagnostic_item
It has a warning that it's slower than is_diagnostic_item (presumably because it invalids more of the incremental cache). Should I use it anyway?

Also, I'm not sure what you're suggesting the new code should look like. Don't I need to check all the types in the array no matter what?

It already exists

That gets a DefId from a name. I'm suggesting the opposite.

My second suggestion is to make the parent module a diagnostic item instead of the types and then just check the name of the type.

library/core/src/sync/atomic.rs

compiler/rustc_lint/src/types.rs

camsteffen · 2021-04-10T22:52:59Z

compiler/rustc_lint/src/types.rs

It would be better short-circuiting to check the method name before the type.

Unfortunately that panics if the method name is the same but the type doesn't match, because it assumes the call has at least 2/3 args.

Err actually it panics with or without that change; let me look into it.

@lcnr why was the UI test you suggested valid? Shouldn't inherent impls always take precedence over trait impls? nightly rustc accepts it, but I don't know why :/

camsteffen · 2021-04-10T22:56:17Z

compiler/rustc_lint/Cargo.toml

Ooh bold move!

I would prefer that we try to keep our external dependencies as small as possible and this doesn't seem like a necessary addition to me but we have no official policy saying that, so I don't think it should hold up this PR.

@wesleywiser if_chain is already in the source tree through clippy, this just adds it to rustc itself. I could rewrite it without if_chain, but it would be annoying and IMO a lot uglier.

Yes, that's true but clippy isn't a dependency of rustc so any dependencies they pull in don't actually affect rustc. That's why you had to add it to the tidy allowed set of dependencies.

It's my personal preference and we already have quite a lot of external dependencies as that list showed so don't worry about it 🙂

compiler/rustc_lint/src/types.rs

camsteffen

A couple more ideas!

compiler/rustc_lint/src/types.rs

lcnr · 2021-04-15T15:48:37Z

don't have a lot of capacity rn and this is out of cache for me

r? @estebank in case you have the time, otherwise I can probably get to this next weekend

flip1995 · 2021-04-22T09:46:57Z

Sync PR is up: #84427

jyn514 · 2021-04-22T13:54:27Z

I think this is mostly done from an implementation standpoint. @rust-lang/lang does this need an FCP? If so, could someone start one?

bors · 2021-07-24T01:58:35Z

⌛ Testing commit 768e85bbc5f8001b70bb13c37f0777b4ed232500 with merge f530f4975433fbc8a6bc2318fd508a50c0852372...

bors · 2021-07-24T02:30:31Z

💔 Test failed - checks-actions

bors · 2021-07-29T18:21:10Z

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

jyn514 · 2021-07-29T18:23:48Z

@wesleywiser do you know why this is still running on android? I have // only-x86_64 at the top of the file: https://github.com/rust-lang/rust/blob/768e85bbc5f8001b70bb13c37f0777b4ed232500/src/test/ui/lint/lint-invalid-atomic-ordering-int.rs#L2

wesleywiser · 2021-07-29T18:40:57Z

I'm not sure. It seems like that should work as compiletest is getting invoked with the correct target flag. Maybe try adding // ignore-android?

jyn514 · 2021-07-31T14:13:14Z

@bors r=wesleywiser rollup=never

bors · 2021-07-31T14:13:15Z

📌 Commit f1ee65e609789518247d9c8b35050f7941a2e9d4 has been approved by wesleywiser

bors · 2021-07-31T16:28:22Z

⌛ Testing commit f1ee65e609789518247d9c8b35050f7941a2e9d4 with merge 677762189fe82d44347708c21a7eef0e65e5fad4...

bors · 2021-07-31T16:47:34Z

💔 Test failed - checks-actions

camelid · 2021-08-15T22:16:25Z

triage: @jyn514 it looks like some tests need to be blessed to update line numbers and then this can be re-approved.

@rustbot label: -S-waiting-on-review +S-waiting-on-author

jyn514 · 2021-08-16T01:07:45Z

@bors r=wesleywiser rollup=never

bors · 2021-08-16T01:07:47Z

📌 Commit 95f4920a398d3900974a49259b8ee7fc4ec7190e has been approved by wesleywiser

bors · 2021-08-16T03:12:44Z

⌛ Testing commit 95f4920a398d3900974a49259b8ee7fc4ec7190e with merge 41d8c0eb647dd7026d752da6a7dcf871c589dffe...

bors · 2021-08-16T03:45:20Z

💔 Test failed - checks-actions

jyn514 · 2021-08-16T03:46:54Z

I give up. I have no idea what's wrong and I am not willing to put more time into it.

jyn514 · 2021-08-16T03:50:48Z

Oh. For some reason there are like 12 files that all test the same thing, and I only added // ignore-android to one of them.

- 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

jyn514 · 2021-08-16T04:00:13Z

@bors r=wesleywiser

bors · 2021-08-16T04:00:15Z

📌 Commit 5522177 has been approved by wesleywiser

bors · 2021-08-16T06:36:17Z

⌛ Testing commit 5522177 with merge 92f3753...

bors · 2021-08-16T09:38:13Z

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 92f3753 to master...

wesleywiser · 2021-08-16T16:04:59Z

Woohoo! Congrats @jyn514 🎉

jyn514 added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-lang Relevant to the language team labels Apr 9, 2021

rust-highfive assigned lcnr Apr 9, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2021

flip1995 reviewed Apr 9, 2021

View reviewed changes

src/tools/clippy/clippy_lints/src/lib.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

camsteffen reviewed Apr 10, 2021

View reviewed changes

jyn514 force-pushed the uplift-atomic-ordering branch 3 times, most recently from 40b4b00 to caa95ff Compare April 11, 2021 04:40

This comment has been minimized.

Sign in to view

camsteffen reviewed Apr 12, 2021

View reviewed changes

compiler/rustc_lint/src/types.rs Outdated Show resolved Hide resolved

compiler/rustc_lint/src/types.rs Outdated Show resolved Hide resolved

jyn514 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 14, 2021

camsteffen reviewed Apr 14, 2021

View reviewed changes

compiler/rustc_lint/src/types.rs Outdated Show resolved Hide resolved

jyn514 force-pushed the uplift-atomic-ordering branch from daf3ddf to 116d14b Compare April 14, 2021 20:03

jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 14, 2021

rust-highfive assigned estebank and unassigned lcnr Apr 15, 2021

This comment has been minimized.

Sign in to view

thomcc and others added 2 commits August 16, 2021 03:55

Only run expensive calculations if the method name is recognized

5522177

bors mentioned this pull request Aug 16, 2021

Add enum_intrinsics_non_enums lint #83908

Merged

Takashiidobe mentioned this pull request May 31, 2022

remove panic docs for atomic load and stores #97590

Closed

jieyouxu mentioned this pull request Nov 20, 2024

Add test directive needs-target-has-atomic #133095

Closed

Uh oh!

Conversation

jyn514 commented Apr 9, 2021

Changes from clippy

Potential issues

Uh oh!

rust-highfive commented Apr 9, 2021

Uh oh!

jyn514 commented Apr 9, 2021

Uh oh!

jyn514 commented Apr 9, 2021

Uh oh!

Uh oh!

This comment has been minimized.

jyn514 commented Apr 9, 2021

Uh oh!

thomcc commented Apr 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

camsteffen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lcnr commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flip1995 commented Apr 22, 2021

Uh oh!

jyn514 commented Apr 22, 2021

Uh oh!

This comment has been minimized.

bors commented Jul 24, 2021

Uh oh!

This comment has been minimized.

bors commented Jul 24, 2021

Uh oh!

bors commented Jul 29, 2021

Uh oh!

jyn514 commented Jul 29, 2021

Uh oh!

wesleywiser commented Jul 29, 2021

Uh oh!

jyn514 commented Jul 31, 2021

Uh oh!

bors commented Jul 31, 2021

Uh oh!

This comment has been minimized.

bors commented Jul 31, 2021

lcnr commented Apr 15, 2021 •

edited

Loading