Stream: t-compiler/major changes

Topic: Uplift the `invalid_atomic_ordering` lint from clippy


Thom Chiovoloni (Dec 01 2020 at 03:12, on Zulip):

A new proposal has been announced: Uplift the invalid_atomic_ordering lint from clippy to rustc. I have no clue if this will be discussed in a meeting or not but you probably can find better things to do with that time in any case.

Something seems to have happened to the MCP stream robot, so I am fulfilling its role manually. Beep boop.

BlackHoleFox (Dec 01 2020 at 04:35, on Zulip):

I'm in favor /o/. A. It seems harmless and provides better IDE checking vs a runtime panic, and B. This bit me the other day and would have been nice to have :)

simulacrum (Dec 01 2020 at 13:45, on Zulip):

I believe lint additions should be lang MCP?

Thom Chiovoloni (Dec 01 2020 at 18:32, on Zulip):

In https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Process.20for.20moving.20lints.20from.20Clippy.20to.20Rustc/near/214891748 I was told to make it in compiler-team. https://github.com/rust-lang/compiler-team/issues/346 is also precedent of this being a compiler-team thing in the past.

scottmcm (Dec 01 2020 at 18:35, on Zulip):

(This would be a good thing to have a meta discussion about, I think -- personally I don't think lang needs to approve lints about things that don't work. Maybe if there are proposals to lint on things that _do_ work it makes some sense to loop in lang because it's a weak language feature deprecation. But especially for "that's the wrong way to use a library function" I don't think lang needs to be involved, though maybe libs would want to know.)

simulacrum (Dec 01 2020 at 18:37, on Zulip):

Yeah, I think there's definitely some meta discussion -- my impression was that lang wanted to approve all lints. It seems fine for some of that to be ceded to T-compiler/libs.

nikomatsakis (Dec 01 2020 at 21:00, on Zulip):

We have typically said we want to approve lints, yes.

nikomatsakis (Dec 01 2020 at 21:00, on Zulip):

But I don't know that we need an MCP

nikomatsakis (Dec 01 2020 at 21:00, on Zulip):

I might be inclined to just have the PR

nikomatsakis (Dec 01 2020 at 21:00, on Zulip):

we should document it, regardless

Josh Triplett (Dec 01 2020 at 21:26, on Zulip):

:+1: for just doing an FCP in the PR for lang team approval.

scottmcm (Dec 02 2020 at 20:41, on Zulip):

For lack of an obvious place to put it, I filed it as a meeting proposal in https://github.com/rust-lang/lang-team/issues/72

Thom Chiovoloni (Dec 03 2020 at 02:27, on Zulip):

I just went and did it https://github.com/rust-lang/rust/pull/79654.

Thom Chiovoloni (Dec 03 2020 at 02:41, on Zulip):

One super nitpickey issue I thought of is:

When given _.store(_, AcqRel) the lint's help message tells you which orderings would be valid for a store, for example:

error: atomic stores cannot have `Acquire` or `AcqRel` ordering
  --> $DIR/lint-invalid-atomic-ordering-bool.rs:25:20
   |
LL |     x.store(false, Ordering::AcqRel);
   |                    ^^^^^^^^^^^^^^^^
   |
   = help: consider using ordering modes `Release`, `SeqCst` or `Relaxed`

If the code really did want AcqRel semantics on the store, the correct answer would be _.swap(_, AcqRel).

That said, I feel like anybody who knows that they really want AcqRel for that store is likely to know this already, and we suggest SeqCst which is strictly stronger on the store than the AcqRel swap would be anyway.

A similar issue also exists for a _.load(Release) since in the unlikely event where they really want that, the right answer would be to do both _.load(Relaxed) and a fence(Release) (and in practice, IME _.load(Release) is usually _.load(<uh, was it Acquire or Release that I use for loads?>))

Anyway, I think this is fine, since the help message mentions sufficiently strong options for both those cases, and code that wants those things is pretty weird anyway and would probably only be written by people who know this already (hopefully). I figured I'd mention it in case someone strongly disagrees, though.

Mario Carneiro (Dec 03 2020 at 02:44, on Zulip):

Doesn't this ordering literally not make sense though? Like it makes no sense to give this to LLVM as it doesn't mean anything

Mario Carneiro (Dec 03 2020 at 02:46, on Zulip):

If the code really did want AcqRel semantics on the store,

That is, I don't think there is such a thing as AcqRel semantics on a store

Thom Chiovoloni (Dec 03 2020 at 03:03, on Zulip):

@Mario Carneiro right, it would need to be an RMW.

Thom Chiovoloni (Dec 03 2020 at 03:09, on Zulip):

you see this in implementation of some concurrent algorithms (petersons algorithm for example requires it when locking, i believe)

Mario Carneiro (Dec 03 2020 at 03:54, on Zulip):

If it's a RMW or something where the ordering makes sense, I would expect the error/lint warning against it not to exist

Thom Chiovoloni (Dec 03 2020 at 03:56, on Zulip):

Yeah, the lint doesn't fire on anything that wouldn't cause an unconditional panic.

Thom Chiovoloni (Dec 03 2020 at 03:56, on Zulip):

Anyway, it seems what I said was just very confusing, sorry.

Last update: May 07 2021 at 07:45UTC