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

Uplift the invalid_atomic_ordering lint from clippy to rustc #79654

Closed

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Dec 3, 2020

This is an implementation of rust-lang/compiler-team#390.

Nobody complained beyond noting that it doesn't seem like there's a good process here (I strongly agree!), and I had some time today, so I went and did it. I have no clue who to ask for review, so I'm hoping either reviewbot picks well or it sorts itself out (by way of someone telling me who to request or someone passing it off or whatever).

Maybe I should have waited to get a second or find a reviewer? If so, sorry. The process is pretty unclear, and I don't really see any downsides to this lint.

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? I don't know how to ping either.


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:

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

@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2020
@thomcc
Copy link
Member Author

thomcc commented Dec 3, 2020

Hrm, tidy is mad about the comments that tell what the error message should be. I'm not sure I should do about that, unfortunately. I just truncated the lines it was mad about.

@flip1995
Copy link
Member

flip1995 commented Dec 3, 2020

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) but I'm not sure if I need to do anything else there.

Thanks for notifying me! The lint is not yet properly deprecated. If this PR is approved from the Rust side, I can do this for you (either by providing you a patch file for it or by pushing a commit myself). Explaining how to do it is harder than doing it myself. Maybe we should write a tool for deprecating lints. 🤔

Currently every test in Clippy fails, because registering a lint as removed, registers the lint, which is a problem, because it is already registered.

  • x.py test src/tools/clippy fails with a lot of errors with and without my changes (and is probably a nonsense command regardless).

Well this is exaclly the command to use to test Clippy. It should always pass without changes to Clippy.

@thomcc
Copy link
Member Author

thomcc commented Dec 9, 2020

@flip1995 I undid it, which fixed the issue. Not sure how you want to do the rest of it though, I'm open to whatever, but sending a patch might be the easiest.

@flip1995
Copy link
Member

flip1995 commented Dec 9, 2020

@thomcc You can find the Clippy patch here: https://gist.github.com/flip1995/a9758736c0a71b33c3d579134af4dc17

(This patch is based on the branch of this PR rebased on top of 8080f54. I had to rebase to get it to compile locally. However, the patch probably also works without a rebase)

@thomcc thomcc force-pushed the uplift-invalid-atomic-ordering-lint branch from f278453 to ee38c89 Compare December 9, 2020 12:04
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems mostly ready to me.

Can you use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.is_diagnostic_item instead of comparing paths? You may have to add some #[rustc_diagnostic_item = "variant_name"] to the variants for this

}

fn matches_ordering_def_path(cx: &LateContext<'_>, did: DefId, orderings: &[Symbol]) -> bool {
orderings.iter().any(|ordering| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use diagnostic items instead here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get

error: unused attribute
   --> library/core/src/sync/atomic.rs:223:5
    |
223 |     #[rustc_diagnostic_item = "atomic_ordering_relaxed"]

on each of these if I try to put it on a variant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think (code's still compiling...) can fix this by adding

        if let hir::ItemKind::Enum(e, _) = &item.kind {
            for variant in e.variants {
                self.observe_item(variant.attrs, variant.id);
            }
        }

after this

self.observe_item(&item.attrs, item.hir_id);
, and making it #[cfg_attr(not(bootstrap), rustc_diagnostic_item = "atomic_ordering_florb")]

It's small enough for me to think that's okay (if it were a more invasive change I'd punt on it), but it's your call.

(I'll update this based on if the tests manage to pass after the rebuild)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to run to work before the build finished, so I pushed it up so CI could sort it out for me. That is: the last two commits are just tentative.

let method = method_path.ident.name;
if Self::type_is_atomic(cx, &args[0]);
if method == sym::load || method == sym::store;
let ordering_arg = if method == sym::load { &args[1] } else { &args[2] };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diagnostic items instead of Symbol equality

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems tricky because there's not just one load method, there's one for AtomicBool::load, one for AtomicU8::load, one for AtomicU16::load, etc.

I had thought that diagnostic items wouldn't work here (it didn't seem to, but maybe I'm mistaken)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, they do have to be unique afaik 🤔

I guess using names is fine in that case. Thanks for looking into this.

Copy link
Member Author

@thomcc thomcc Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure how to solve this but it also seems like less of an issue (since we use rustc_diagnostic_item for the actual atomic type). I don't think it's worth having atomic_store_u8, atomic_store_u16, etc methods all have unique diagnostic items.

It feels like you kind of want a rustc_diagnostic_tag which would be allowed to have multiple things that it applies to.

As is maybe this is fine though. What's the downside of not using rustc_diagnostic_item here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, someting like rustc_doagnostic_tag is what I would want here 👍

The main issue is when moving stuff in std which breaks lints like this. I think this may also cause issues if a user impls a trait like the following, or at least it's less obvious for people reading the code what's the expected behavior here

trait Foo {
    fn load(self);
}

In this PR you can just keep using sym::load for now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add the following as an ui test?

// check-pass
use std::sync::atomic::{AtomicUsize, Ordering};

trait Foo {
    fn store(self, ordering: Ordering);
}

impl Foo for AtomicUsize {
    fn store(self, _ordering: Ordering) {
        AtomicUsize::store(&self, 4, Ordering::SeqCst);
    }
}

fn main() {
    let x = AtomicUsize::new(3);
    x.store(Ordering::Acquire);
}

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me regarding the Clippy changes.

@bors
Copy link
Contributor

bors commented Dec 11, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the implementation looks good to me, even if i personally dislike if chains because they break rustfmt but that isn't something worth blocking this PR on 🤷

@rust-lang/lang is now required for an fcp if i understand that correctly?

@@ -211,13 +213,15 @@ unsafe impl<T> Sync for AtomicPtr<T> {}
#[stable(feature = "rust1", since = "1.0.0")]
#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
#[non_exhaustive]
#[allow(unused_attributes)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[allow(unused_attributes)]
#[cfg_attr(bootstrap, allow(unused_attributes))]

let method = method_path.ident.name;
if Self::type_is_atomic(cx, &args[0]);
if method == sym::load || method == sym::store;
let ordering_arg = if method == sym::load { &args[1] } else { &args[2] };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add the following as an ui test?

// check-pass
use std::sync::atomic::{AtomicUsize, Ordering};

trait Foo {
    fn store(self, ordering: Ordering);
}

impl Foo for AtomicUsize {
    fn store(self, _ordering: Ordering) {
        AtomicUsize::store(&self, 4, Ordering::SeqCst);
    }
}

fn main() {
    let x = AtomicUsize::new(3);
    x.store(Ordering::Acquire);
}

@thomcc
Copy link
Member Author

thomcc commented Dec 31, 2020

Uhhhh sorry about the delay here. Have been away. Looking into what's needed of me here now.

@JohnCSimon
Copy link
Member

JohnCSimon commented Jan 19, 2021

Ping from triage
@thomcc - can you please resolve the merge conflict - and mark this s-waiting-on-reviewer? Thank you!

@JohnCSimon
Copy link
Member

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

@rustbot rustbot 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 Jan 19, 2021
@thomcc
Copy link
Member Author

thomcc commented Jan 19, 2021

Sorry, I ended up getting the flu and having no time to do what I said #79654 (comment).

I'll try to get to this this week sometime.

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2021
@jyn514 jyn514 added the A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. label Feb 16, 2021
@crlf0710
Copy link
Member

crlf0710 commented Mar 5, 2021

@thomcc Any updates on this?

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2021
@crlf0710
Copy link
Member

crlf0710 commented Apr 9, 2021

Triage: I'm gonna close this due to inactivity. Feel free to reopen or create a new PR when you've got time to work on this again. Thanks!

@crlf0710 crlf0710 closed this Apr 9, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 16, 2021
…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).
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Aug 26, 2021
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants