`// CAST: ...` comment on `as` casts

[ojeda]

What it does

Similar to // PANIC: ... and // SAFETY: ... comments, but for as casts.

In the Linux kernel, we try to avoid type cast expressions, i.e. the as operator, which are quite powerful, and instead aim to use restricted variants that are less powerful to avoid mistakes and make intent more clear. We already enable other Clippy lints to spot such cases, e.g. we enabled for 6.17 a set of those: ptr_as_ptr, ptr_cast_constness, as_ptr_cast_mut, as_underscore, cast_lossless and ref_as_ptr.

Thus it would be nice to require a developer to write an explanation for the remaining cases, just like they need to do so for unsafe blocks with // SAFETY: ..., or for certain panicking operations with // PANIC: ... (#15895).

That is, we would like code to look like:

// CAST: ...
value as usize

In other words, like undocumented_unsafe_blocks, but for as casts. And like as_conversions, but only triggering if the comment is not provided.

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

Note on pointer casts: currently in the kernel we sometimes use // CAST: ... for pointer casts via .cast() too, since those are dangerous too (even if already more restricted than as). We may want to extend this lint (or ideally have a separate one) to cover those too. This is particularly important for the dual lint.

Cc: @blyxyas @hcbarker

Advantage

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

• Type cast expressions get documented better, i.e. the reason they are needed must be explained.

• It gives some pause to developers when introducing a type cast expression -- in many cases, there are better alternatives they should be using instead.

These advantages also mean review time (and thus maintainers' workload) gets reduced, since developers will more often realize an as cast should not have been used before submitting the code (or, if it was truly needed, then the reason will help others understand why without having to ask about it).

Drawbacks

No response

Example

value as usize

Should be written as:

// CAST: ...
value as usize

Comparison with existing lints

This is similar to as_conversions, but it would only lint if there is no // CAST: ... comment justifying the conversion.

One could require allow or expect for this instead -- please see the rationale for using a comment instead on #15895:

There is of course #[allow(..., reason = "...")] nowadays as well, but ideally we would want it as a comment since we already have other similar "tags", e.g. SAFETY, INVARIANT, CAST..., which would make it more consistent, and it would not require the "burden" of mentioning the lint name, deciding whether to allow or expect, writing the usual attribute syntax, and so on.

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. That also allows to keep allows for this lint reserved for the case where the lint somehow fires but it wasn't supposed to fire.

Additional Context

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

[charlieHsiuC]

Hello!

I've had a quick look at the relevant code and would love to give this a try as my first contribution.
I'm still familiarizing myself with the details, so I'd appreciate some mentorship.

@rustbot claim

[charlieHsiuC]

Hi @ojeda,

I've started a PR for this issue with an intuitive implementation.
I'm not entirely sure if I'm on the right track, so early feedback would be great!

A couple of specific questions:

• Should this lint handle macros, similar to how undocumented_unsafe_blocks does?
• For the complex test case I added: where would be the best place to put the comment?

Thanks!

[ojeda]

Thanks a lot for working on this! The tests you wrote are nice.

Should this lint handle macros, similar to how undocumented_unsafe_blocks does?

Do you mean casts inside macros? I think we can ignore macros for now, if that makes it easier.

For the complex test case I added: where would be the best place to put the comment?

Do you mean the match ... { ... } as ... case? Hmm... That is a good question.

We should try to be consistent with // SAFETY: and other "tags", but I guess this is the first such case. Hmm... I don't think we have that code in Linux, so hopefully it is not common (we do have unsafe { ... } as ... one liners, though).

My first instinct would be to put it at the top of the match, because putting it in the previous line to } looks like it is "outside". I guess one could argue for having it between the } and the as too, but would rustfmt work well if we did that? Even if it does, perhaps at the top is simpler.

---

By the way, have you considered splitting the lint by different kinds of casts somehow (or making it configurable)?

I am asking in case it may be possible to make it easier to gradually enable those lints, since covering every as cast at once may make it hard for projects to adopt (without introducing many allows or expects or // CAST: TODOs).

For instance, some ideas:

• Perhaps chained casts like ... as ... as ... could be one.
• Perhaps as _ could be another.
• Perhaps pointer casts could be another (we should consider the interactions with the other existing lints about pointer casts too).
• Perhaps another could be casts that are used to match the type of a static or const definition, e.g. const ID: bindings::clockid_t = bindings::CLOCK_MONOTONIC as bindings::clockid_t;.

It would be good to try to apply the lint to some small projects that have casts, to see how hard it would be to enable, e.g. I got some of those ideas from a quick grep in our Linux code.

[charlieHsiuC]

Thank you for the detailed feedback and the encouraging words!

Since I’m still familiarizing myself with the project, I agree that ignoring macros for now is a good way to keep the initial scope manageable.

Your suggestion to analyze 'as' cast patterns in the Linux kernel is very helpful. I'll go through the codebase to see how these casts are typically used (such as chained casts, pointer casts, or those in const definitions) to better understand the real-world impact.

I also agree that making the lint configurable or splitting it into categories is essential for gradual adoption. I’ll focus on the analysis first and then refine the implementation based on those findings.

Regarding the match case, placing the comment at the top of the match block makes sense to me as well, especially considering rustfmt and overall readability.

I'll update you once I have more insights from the analysis!

[charlieHsiuC]

I spent some time figuring out how to run my custom lint on the Rust code within the Linux kernel.
After running it (ignoring macro expansion), I found over 200 instances of as casts.

Following your suggestions, I’ve categorized these instances as follows:

• Chained casts (as ... as ...): Only 1 occurrence.
• Underscore casts (as _): These seem to be covered by clippy::as_underscore. I didn't see this in the core kernel code, though I found some instances in driver code.
• Pointer casts (as mut* and as const*): Found 15 and 9 instances, respectively.
• Casts matching const or static definitions (70 total):
  • as vm_flags_t (26) and as bindings:: (4).
  • as c_ and as ffi::c_ (6).
  • as usize (27).
• Casts during variable initialization/assignment (~22 instances):
  • let pos = buf as usize;
  • let io = match MmioRaw::new(ioptr, len as usize) {
  • let ioptr: usize = unsafe { bindings::pci_iomap(pdev.as_raw(), num, 0) } as usize;
  • let index = unsafe { TableIndex::new(index as usize) };
  • let out = unsafe { &mut *(core::ptr::from_mut(out) as *mut [MaybeUninit<u8>]) };
• Casts matching function input/output:
  • bindings::fsleep(delta.as_micros_ceil() as c_ulong)
  • return Err(Error::from_errno(res as i32));
  • Ok(res as usize)
  • This category could potentially be further subdivided by types like as c_, as bindings::, as usize... .
• Casts within assert macros: 7 instances found in build_assert! and debug_assert!.

These are my initial thoughts on the categorization based on the analysis. There might be some cases I've missed, but I hope this provides a solid foundation as we continue our discussion.

[ojeda]

Thanks for the analysis!

I think the next step would be to see which ones would be the worthiest/easiest, e.g. those that are likely to be wrong or subtle, and thus a reader would appreciate a // CAST: comment in almost all cases, and so enforcing such a comment everywhere would be easy to justify.

I would also suggest checking our existing // CAST: comments -- there may be some ideas/inspiration there.

[charlieHsiuC]

I found most existing // CAST: comments are about pointer casting. So, I'll start with checking as mut* and as const*.

[charlieHsiuC]

I added a configuration to restrict checks to as casts for *const T and *mut T only, while retaining an option to enable checks for all types of as casts.
Regarding the FCP discussion, I agree that the logic for // CAST: comments should be generic and shared across different undocumented lints. Would it be better to first define a list of accepted scenarios and then implement them incrementally?"