Make borrow_as_ptr flag implicit casts as well

[samueltardieu]

While I like replacing &x by &raw const x and &mut x by &raw mut x, I'm less sure I like the suggested reborrows such as &raw const *p. There was one in Clippy sources, see the PR diff.

@RalfJung, any opinion on this?

Fix #14406

changelog: [borrow_as_ptr]: lint implicit casts as well

[rustbot]

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[samueltardieu]

Note: I really dislike the reborrow suggestions, I'll remove them, and I'm not even sure this should be covered by this lint according to the description. Edit: done, especially given the hundreds of hits in lintcheck

[RalfJung]

Can you give a self-contained example of the reborrow case you asked about?

[samueltardieu]

Can you give a self-contained example of the reborrow case you asked about?

fn main() {
    let p = &0i32;
    let r: *const i32 = p;
}

Initially I had this suggest let r: *const i32 = &raw const *p;, but this should not be covered by the lint. This was particulary ugly with a function call instead of p, as it was proposing using things like &raw const *f(x) if f(x) was returning a ref.

[samueltardieu]

Incidentally, the lintcheck output without regard to MSRV made me notice some suspicious code in tracing-core where it looks like *const &Self pointers are compared with ptr::eq() when comparing *const Self pointers might have been intended.

[samueltardieu]

The lintcheck seems reasonable.

[RalfJung]

Initially I had this suggest let r: *const i32 = &raw const *p;, but this should not be covered by the lint

Agreed. Ideally this code would be changed so p becomes a raw pointer (if it has no other uses), but that's probably beyond the scope of the lint.

[RalfJung]

Does this cover all coercion sites, including e.g. function calls? I guess the fact that this triggered on that tracing-core code means the answer is "yes".

[samueltardieu]

Does this cover all coercion sites, including e.g. function calls? I guess the fact that this triggered on that tracing-core code means the answer is "yes".

Yes. It looks for all explicit references which are auto-reborrowed by the compiler as raw pointers, except for references in explicit casts, as those are linted separately.

Interestingly, this discussion brought me to think about one problematic case: &0 (or any ref to a temporary value) can be auto-reborrowed as &raw const 0, but the latter cannot be written as-is ("cannot take address of a temporary"). I have to think more about how to lint let _: *const i32 = &0. Should it warn? Should it suggest putting the temporary into a variable, but to what effect?

Edit: I've changed not to lint references to temporaries, as was done in the &temp as *const T already.

[RalfJung]

Yes. It looks for all explicit references which are auto-reborrowed by the compiler as raw pointers, except for references in explicit casts, as those are linted separately.

Can't this be just one joint check? The diagnostics can then be different, but ideally the logic for "do we lint" is unified.
(I'm not familiar with clippy's codebase though, maybe there's a good reason to have these separate.)

I have to think more about how to lint let _: *const i32 = &0. Should it warn? Should it suggest putting the temporary into a variable, but to what effect?

Good point. This should not lint.

[samueltardieu]

Yes. It looks for all explicit references which are auto-reborrowed by the compiler as raw pointers, except for references in explicit casts, as those are linted separately.

Can't this be just one joint check? The diagnostics can then be different, but ideally the logic for "do we lint" is unified. (I'm not familiar with clippy's codebase though, maybe there's a good reason to have these separate.)

The only part to share is the temporary expression check, and this is done. Otherwise, entry points are different (cast expression vs. any pointer expression whose parent is not a cast).

I have to think more about how to lint let _: *const i32 = &0. Should it warn? Should it suggest putting the temporary into a variable, but to what effect?

Good point. This should not lint.

Yes, this is what I ended up doing.