`// ORDERING: ...` comment for atomic operations

[ojeda]

What it does

Similar to // SAFETY: ..., // PANIC: ... and // CAST: ... comments, but for atomic operations that use one or more Orderings.

Note that in the Linux kernel we use a custom implementation of atomics, so we should probably pass a list of functions/methods/... to warn about, similar to // PANIC: ....

More generally, we could consider an implementation that warns on functions/methods that take Ordering parameters unless opt-ed out. But just the list as for // PANIC: ... sounds good enough, and perhaps it makes sense to instead generalize that lint to allow the user to customize a map of TAG-> list of methods that require a comment.

That is, we would like code to look like:

// ORDERING: Relaxed is fine because we don't expect synchronization here.
let old = self.init.xchg(1, Relaxed);

In other words, like undocumented_unsafe_blocks, but for atomic operations.

In addition, there would be a dual lint to detect unnecessary comments: #16074.

Cc: @blyxyas @hcbarker @fbq

Advantage

The advantages are very similar to the ones we have seen from applying the // SAFETY: ... convention:

• Atomic operations get documented better, i.e. the reason a particular Ordering is used must be explained.

• It gives some pause to developers when picking an Ordering.

These advantages also mean review time (and thus maintainers' workload) gets reduced, since reviewers will have an easier time following the rationale behind the Orderings picked.

Drawbacks

No response

Example

let old = self.init.xchg(1, Relaxed);

Should be written as:

// ORDERING: Relaxed is fine because we don't expect synchronization here.
let old = self.init.xchg(1, Relaxed);

Comparison with existing lints

No response

Additional Context

Please see the "Additional context" for // PANIC: ... on #15895.

[y21]

I'm personally not too much of a fan of these tag comments, given that #[expect] achieves basically the same thing. I'm aware that there's some arguments for them in #15895 but they don't seem convincing enough for me for the complexity this would introduce, the dual lint #16074 for example is also simply not needed if this were just a normal restriction lint that people #[expect], since that already warns if the lint doesn't emit there.

Or, seen from another perspective: these // PANIC: comments are not so much about allowing a false positive of a lint, but rather about writing a required comment.

I'd argue most restriction (or even some pedantic lints, like the various cast ones) are intended to be used exactly like this; in many instances the pattern to lint against is not a false positive and you want to #[expect] them. Combined with allow_attributes_without_reason it's also possible to require a comment.

[blyxyas]

I talked with Miguel about this concern in Kangrejos this year, and he denoted the aspect that #[expect] is much more technical, people see it more of an ad-hoc justification of why the "bad pattern is permitted". In these cases, the // PANIC, "// CAST", etc... comments move from a "ignore this warning, I know it's bad" mentality into a "I know what I'm doing, and this is the reason" mentality.

That's the reason I don't mind having two concepts that technically do the same, but have vastly different ways of thinking.

(Also, I think that the Rust-skeptic crowd doesn't like to see allows for our lints)

[ojeda]

@y21 Attributes are a quite bad fit for this, in my opinion.

They are harder to read and write, they seem worse for multiline, they require providing a lint level which is not relevant (expect), they require a lint name that may be longer than needed (e.g. a clippy:: prefix and possibly more, e.g. a suffix) and that name may be an implementation detail after all.

Really, what we want is an annotation that explains something about the code that follows. Which is what a code comment is, after all.

In my view, it is quite clear just by looking at it:

#[expect(clippy::ordering_tag, reason = "Relaxed is fine because we don't expect synchronization here.")]
let old = self.init.xchg(1, Relaxed);

// ORDERING: Relaxed is fine because we don't expect synchronization here.
let old = self.init.xchg(1, Relaxed);

It is not just about the length, but about the fact that, with the attribute approach, the most important information is mixed with other stuff in the middle of the line (in the case of the tag kind) and at the end of the line (in the case of the reason).

Moreover, using a lint attribute for this is, in my view, abusing the lint system. For instance, since we support several compiler versions, we need to support ignoring only those unknown expects without wrapping with cfgs. Maybe there is a way for that already, but my point is that such a problem may appear because it was not meant for that. Similarly, we would need a way to customize allow_attributes_without_reason for certain lints only.

There are other annoying bits when considering the wider context, e.g. it will be inconsistent with existing // SAFETY: ... comments and, when one sees an attribute, one will need to scan it to see the lint name just to understand if it is an explanation (i.e. like a tag) or about a false positive (i.e. like a lint attribute).

I'd argue most restriction (or even some pedantic lints, like the various cast ones) are intended to be used exactly like this;

I don't know if most (quite a few look to me like about replacing/avoiding the pattern completely and I don't see why one would use #[expect]), but it is true that some may be used as you say. For those, I think a tag would have been best for the reasons mentioned above.

for the complexity this would introduce

I would imagine most of these tags would work similarly to other lints, especially since nowadays the infrastructure for // SAFETY: ... is there.