add new lint as_underscore_ptr to check for as *{const,mut} _

[asquared31415]

This probably could go in suspicious, but I was unsure where exactly to put it. It's not necessarily wrong code on its own, but can be problematic in certain situations.

changelog: [as_underscore_ptr]: a new lint to detect use of as *const _ or as *mut _ which can be the cause of subtle raw pointer bugs.

[rustbot]

r? @Alexendoo

(rustbot has picked a reviewer for you, use r? to override)

[Niki4tap]

Does this also lint on mutability casts?

Like:

let ptr: *mut SomeVeryComplexType = ...;
return ptr as *const _;

I feel like this lint can be very noisy in some cases, maybe pedantic is a better category for this?

[asquared31415]

It currently is pedantic because I was aware that lots of code uses these casts (not just in the mutability case). But yes it lints every time the target type of a cast is any pointer to _.

[Alexendoo]

but can be problematic in certain situations.

as *const _ is a fairly common pattern, do we have a good idea about what those situations are and if we could target them specifically?

e.g. like @Niki4tap brings up we could ignore when the T in &/*const/*mut T is unchanged, would there still be potential issues if we ignore those cases?

fn f(x: &u8) {
    // only the pointer/reference kind is changed, does not lint
    let a: *const u8 = x as *const _;
    let b: *mut u8 = a as *mut _;

    // changes the type of the referent, would lint
    let c: *const i8 = a as *const _;
}

[asquared31415]

Linting on "a pointer cast that changes the type of the referent" for ptr->ptr is just the current lint that suggests ptr.cast::<T>(). ref->ptr casts cannot change the type of the referent, as enforced by the compiler.
There probably should be a lint for *const T as *mut T (or the other way) without changing the type that suggests some unstable ptr methods as well, though idk what the clippy policy on that is.

Does clippy have a crater-like tool that I could use to better understand the cases where this fires and the lint's suggestion would be a downgrade? I expected the lint to be moderately common, hence an allow-by-default group, but also from my understanding, a significant number of people try to avoid this pattern as much as reasonable, because of how tricky the casts can be.

The goal of this new lint is to find cases where the use of as * _ produces a type that the user might not expect, or is liable to change without warning. Some of the specific cases that I had in mind are:

struct UwU;
impl UwU {
    // intent: turn a `&UwU` into a `*const u8` that points to the same data
    fn as_ptr(&self) -> *const u8 {
        // ⚠️ `&self` is a `&&UwU`, so this turns a double pointer into a single pointer
        // ⚠️ This pointer is a dangling pointer to a local
        &self as *const _ as *const u8
    }
}

⬆️ (turns a double pointer into a single pointer because the type of self is less obvious)

fn uwu(data: &[u64]) -> *const u64 { // changing the return type is dangerous
    data.as_ptr() as *const _
}

⬆️ (the pointer type could be easily changed by inference)

unsafe fn read_data(data: *const u32) { // 1. if this signature changes
    let _ = *data;
}

let arr = [1_u32, 2, 3, 4];
unsafe {
    let ptr = arr.as_ptr().add(2) as *const _; // 2. this inference can change
    /*
    some more code
    */
    read_data(ptr); // 3. because of this call
}

⬆️ (inference for a local is determined by how it gets used later)

Cases where the type is not obvious and cases where the type is obvious look really similar, and I'm not sure if clippy wants the complexity of picking out specific patterns for this lint, if the information such as "what controls this inference" is even available at all.

I would like to improve the lint so that people can use it via pedantic or possibly even suspicious if it can be very accurate, but if that's not reasonable/possible, would putting this lint in restriction as-is be acceptable, under the same reasoning as as_underscore?

[asquared31415]

On your point of trying to exclude "the referent is unchanged", in the example I provide in the lint (and the first example in my previous comment) the problematic cast is "reference to pointer that does not change the pointed to type", &self as *const _ (does &&UwU -> *const &UwU, and that's all the compiler allows). The next cast is "cast a pointer to another pointer with an explicit type". Under that suggested rule as-is, we miss the first cast, which is the suspicious cast, and the second cast always has to be fine, ptr -> ptr is a well defined and common pattern, the best we can do is suggest .cast::<u8>(), but that doesn't help this specific casting error.

[Alexendoo]

Thanks for the examples!

Yeah at the very least this is a useful restriction lint, but higher than that would be good if we can get it not too loud

I saw the first example in the test file that didn't contain the ⚠️ comments explaining the issue. But just from the lint message in .stderr I think I glossed over the extra & in the type, it could be worth adding a note if the type is behind a reference to drive the point home. Maybe that could also be a heuristic itself

For the other two, if the lint didn't fire because the type behind the pointer is unchanged it would at least serve as a guard ensuring it stays that way

[Alexendoo]

A route we could go is to land this as-is in restriction, then have any heuristic based ones as their own new lints. Because it would still be useful to be able to deny all inferred type casts

[blyxyas]

The use of UwU instead of the boring foo and bar is epic.

[asquared31415]

I think that "is more than indirection" could be a useful heuristic for showing an additional note for this, for things like *mut &T, *mut *mut T, *mut Rc<T>, and *mut Box<T>. I will do that, and lower this lint to restriction.

I will also update the UI test file to contain exactly the same code and comments as the example, since that's more clear.

[KisaragiEffective]

IMHO - if you divide case - casting to nested-pointer (like &&UwU) by as *{const,mut} _ and everything else for the cast, former could be suspicious (or, at least pedantic).

[asquared31415]

I implemented the changes to this lint to show an extra note for some especially suspicious cases, and moved the lint down to restriction. Are there any other concerns with this PR?

[Alexendoo]

Thanks for that, just a couple things then it'd good to merge. Sorry for the delay!

[asquared31415]

What do I do about that CI failure? It says that the as_underscore_ptr UI test has a fixed file (true) but is missing a run-rustfix annotation (false). However when I look at the "changed files" there's 100% a // run-rustfix as the very start of the file?

[Alexendoo]

Ahh that's an unfortunate message, it should be //@run-rustfix since #10669, the message didn't get updated in compiletest but would be resolved by #10426

[bors]

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

[Alexendoo]

I brought this up in the last meeting re: it being restriction and a good way to catch the motivating example was suggested, if we lint casts where the level of indirection decreases but is hidden behind a _ that would be able to be warn by default