Never consider raw pointer casts to be trival

[Noratrieb]

HIR typeck tries to figure out which casts are trivial by doing them as
coercions and seeing whether this works. Since HIR typeck is oblivious
of lifetimes, this doesn't work for pointer casts that only change the
lifetime of the pointee, which are, as borrowck will tell you, not
trivial.

This change makes it so that raw pointer casts are never considered
trivial.

This also incidentally fixes the "trivial cast" lint false positive on
the same code. Unfortunately, "trivial cast" lints are now never emitted
on raw pointer casts, even if they truly are trivial. This could be
fixed by also doing the lint in borrowck for raw pointers specifically.

fixes #113257

[rustbot]

r? @compiler-errors

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

[saethlin]

We have a MIR opt that deletes redundant casts. Would it simplify things to always emit casts and lean on the opt?

[compiler-errors]

We have a MIR opt that deletes redundant casts.

Yeah, perhaps we could do a perf run on that and see what it syas.

[compiler-errors]

@rustbot author (pls return to me after experimentation, or if you literally just don't want to test stuff, since this seems fine either way)

[bors]

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

[Noratrieb]

I looked a bit more into this. Saethlin's suggestion is not really possible. In MIR, all casts are "fully resolved", it's exactly known what kind of cast it is: https://doc.rust-lang.org/1.71.0/nightly-rustc/rustc_middle/mir/enum.CastKind.html. So we can't just create completely redundant casts like that (there is a MIR opt to remove redundant casts and it will nicely remove the pointer cast from #113257, but we can't just generate "a cast" because MIR has no notion of "a cast".

When exploring more about how casts work, I wrote down some stuff about them: https://hackmd.io/@Nilstrieb/S1eYpMXF3

So the question that remains for me is whether to do this pointer special casing (I am convinced that we need pointer special casing, because pointer casts are special) should live in THIR building like my PR here or whether it should live in typeck.

Unless you have strong preferences, I would need to take a look at the code again to determine where it would best be placed.

[lcnr]

hm, I think it would be better to change this in CastCheck::check/try_coercion_cast to not set_coercion_cast for pointer casts. Right now this PR would fix this for mir borrowck but it would still incorrectly emit the trivial cast lint.

Unsure how to handle this 🤔 never succeed in try_coercion_cast if we're casting raw pointers feels suboptimal but might be the best solution for now? I think the lint for raw pointers only makes sense after borrowck 🤷

[Noratrieb]

I don't have time to actually implement this right now but right now the solution I like the most is to add a new special if case to this branch here to always call do_check if we're doing a raw pointer cast. It feels wrong to have such a special case but I think we need a strong special case here.

rust/compiler/rustc_hir_typeck/src/cast.rs

Line 729 in 4a0402c

} else {

[Noratrieb]

I changed it a bit to now do the check in HIR typeck. I believe this is the best solution without rewriting how casting works everywhere (I haven't thought about what such a solution would look like), but I am not very confident.
@rustbot ready

[bors]

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

[lcnr]

will take a look soon

r? @lcnr

[lcnr]

@Nilstrieb can you open an issue that we stop emitting the trivial_casts lint for pointers with an example?

---

going to nominate for @rust-lang/lang. I consider this to be a pure bugfix.

We currently do not generate any MIR for pointer casts which only transmute lifetimes. This happens as HIR typeck does not consider the source and target to have distinct types. This causes the following snippet to error:

struct Foo<'a>(&'a ());

fn foo<'a>(v: *const Foo<'a>) -> *const Foo<'static> {
    v as *const Foo<'static>
    //~^ ERROR lifetime may not live long enough
}

This can already be avoided by first casting to an unrelated type:

struct Foo<'a>(&'a ());

fn foo<'a>(v: *const Foo<'a>) -> *const Foo<'static> {
    v as *const () as *const Foo<'static>
}

With this PR both snippets now compile. Unfortunately this means that we now don't emit the trivial_casts lint when casting raw pointers as we don't know whether the cast is trivial during HIR typeck:

#![warn(trivial_casts)]
struct Foo<'a>(&'a ());
fn foo<'a>(v: *const Foo<'a>) -> *const Foo<'a> {
    v as *const Foo<'a>
    //~^ WARN trivial cast: `*const Foo<'a>` as `*const **Foo<'a>`
}

We could check raw pointer casts during MIR borrowck and lint there, but that may not be worth it 🤷 I think it's fine to open an issue for it and merge this as is, as it does not seem too important to me.

[traviscross]

@lcnr: This was discussed in the T-lang meeting today. T-lang agrees that this is a bug fix.

@rustbot labels -I-lang-nominated

[Noratrieb]

I created an issue: #117187.

[lcnr]

@bors r+

[bors]

📌 Commit e8a4814 has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit e8a4814 with merge 6f65201...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 6f65201 to master...

[rust-timer]

Finished benchmarking commit (6f65201): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	5.8%	[5.8%, 5.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.1%	[-3.1%, -3.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.3%	[-3.1%, 5.8%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.8%	[1.2%, 2.8%]	5
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[1.2%, 2.8%]	5

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.0%, 0.1%]	2

Bootstrap: 633.297s -> 633.438s (0.02%)
Artifact size: 304.48 MiB -> 304.44 MiB (-0.01%)

[steffahn]

You might be interested in

• Pointer casts allow switching trait parameters for trait objects, which can be unsound with raw pointers as receiver types under feature(arbitrary_self_types) #120217

which was caused or uncovered (or at least simplified to exploit, in case there was a different way before) by this PR.