improve compile errors for invalid ptr-to-ptr casts with trait objects #130234

lukas-code · 2024-09-11T17:08:47Z

This is a follow-up to #120248 to improve some of its error messages.

Make the borrowcheck error for "type annotation requires that x must outlive y" actually point at the type annotation, i.e. the type T in a x as T cast. This makes the error more consistent with other errors caused by type annotation in other places, such as

error: lifetime may not live long enough
 --> src/lib.rs:4:12
  |
3 | fn bar(a: &i32) {
  |           - let's call the lifetime of this reference `'1`
4 |     let b: &'static i32 = a;
  |            ^^^^^^^^^^^^ type annotation requires that `'1` must outlive `'static`

Don't say "cast" when we actually mean "coercion" and give borrowcheck errors from actual casts (which is currently just the check added in Make casts of pointers to trait objects stricter #120248) a higher priority than ones from coercions. This can improve the errors for ptr-to-ptr cast between trait objects because they are are lowered as an upcast "unsizing" coercion if possible (which may be the identity upcast) followed by the actual cast.
Bring back the old "casting X as Y is invalid" message for type mismatch in the principals and reword the "vtable kinds may not match" message to more accurately describe the pointer metadata and not refer to "vtables" if the metadata is unknown.

fixes #130030

r? @WaffleLapkin but feel free to reassign

compiler/rustc_hir_typeck/src/cast.rs

WaffleLapkin · 2024-09-12T10:22:48Z

compiler/rustc_middle/src/mir/query.rs

@@ -233,8 +233,9 @@ pub enum ConstraintCategory<'tcx> {
    UseAsConst,
    UseAsStatic,
    TypeAnnotation,
-    Cast {
-        /// Whether this is an unsizing cast and if yes, this contains the target type.
+    Cast,


Why does a cast not have unsize_to? Casts can be unsizing, for example: &() as &dyn std::any::Any.

The intent was to have Cast represent only the non-coercion casts from the table here¹ and use Coercion for any coercion whether they are caused by a cast or not. But seeing that MIR also just represents coercions as special kind of cast and not the other way around, maybe we should just add a field is_coercion to the Cast variant and not add a new variant.

Footnotes

side note: function item to function pointer and closure to function pointer should not be listed there as they are coercions. ↩

maybe we should just add a field is_coercion to the Cast variant and not add a new variant.

This seems appropriate. I think it's nice to mention what the user actually written in syntax. But trying to explain that as casts are a super set of coercions is probably confusing.

compiler/rustc_middle/src/thir/visit.rs

compiler/rustc_mir_build/src/build/expr/as_place.rs

compiler/rustc_mir_build/src/thir/print.rs

WaffleLapkin · 2024-09-12T10:33:51Z

tests/ui/lifetimes/issue-90600-expected-return-static-indirect.stderr

 LL |     let read = &refcell as &RefCell<dyn Read>;
-   |                -------- cast requires that `foo` is borrowed for `'static`
+   |                -------- coercion requires that `foo` is borrowed for `'static`


this and some others bellow sure look like a cast? what happened here?

This is a side effect from how explicit coercion casts and implicit coercions are represented in the same way in MIR. I will try to recover the wording here.

So this turned out to be non-trivial, because there is currently no way to distinguish

let x: T = y;

from

let x = y as T

by looking at the MIR alone (without relying on the exact order of MIR statements).

So our options here are

Leave the diagnostic as is, calling some coercion casts just "coercions" (technically correct).

Revert the wording and call all coercions "casts" (technically wrong).

Add some additional diagnostic-only information into the MIR to allow us to distinguish them and output the optimal diagnostic (which is really just one word). This is currently unprecedented as far as I'm aware.

I'm unsure what the best option here is and would appreciate your input :)

I would prefer the third option, i.e. to add some diagnostic only info. That being said I think you should check with people who work on MIR more (miri or miropt zulip streams maybe?).

The first option (using "coercion" when the user wrote a cast) could be OK. If you want we can leave the third option as a follow up (please try to do it though 😄).

After checking again, I found that we do have one instance of diagnostic information in MIR here, so this feels less unprecedented now:

rust/compiler/rustc_middle/src/mir/syntax.rs

Lines 735 to 736 in 8c2c9a9

/// Where this call came from in HIR/THIR.

call_source: CallSource,

tests/ui/traits/trait-object-lifetime-default-note.rs

compiler/rustc_hir_typeck/src/cast.rs

rustbot · 2024-09-15T18:45:38Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

Some changes occurred in match lowering

cc @Nadrieril

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

lukas-code · 2024-09-15T18:51:22Z

Ah shit. Well I guess that's one way to ask for feedback about diagnostic info in MIR. 🥲

The relevant commit is 59696de.

lukas-code · 2024-09-15T19:06:34Z

tests/ui/dyn-star/dyn-to-rigid.stderr

  --> $DIR/dyn-to-rigid.rs:7:5
   |
 LL |     x as usize
-   |     ^^^^^^^^^^
+   |     ^^^^^^^^^^ an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object


This diagnostic is terrible, but that also applies to all other places that it is emitted and I'd rather postpone cleaning it up, because this PR is already getting way too large.

I support that.

rustbot · 2024-09-15T19:41:18Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2024-09-15T20:56:29Z

compiler/rustc_middle/src/mir/syntax.rs

@@ -1421,6 +1419,15 @@ pub enum CastKind {
    Transmute,
 }

+/// Represents how a [`CastKind::PointerCoercion`] was constructed, used for diagnostics.


Suggested change

/// Represents how a [`CastKind::PointerCoercion`] was constructed, used for diagnostics.

/// Represents how a [`CastKind::PointerCoercion`] was constructed.

/// Used only for diagnostics.

WaffleLapkin

I like this a lot!

r=me with or without nits.

WaffleLapkin · 2024-09-20T09:35:03Z

compiler/rustc_borrowck/src/type_check/mod.rs

-                    CastKind::PointerCoercion(PointerCoercion::ReifyFnPointer) => {
+                match *cast_kind {
+                    CastKind::PointerCoercion(PointerCoercion::ReifyFnPointer, coercion_source) => {
+                        let is_implicit_coercion = coercion_source == CoercionSource::Implicit;


nitpick: Maybe just have a CoercionSource field in ConstraintCategory::Cast too, instead of a bool and this conversion?

I considered this, but decided against it, because the Ord of ConstraintCategory is relevant for the diagnostic output and I didn't want this property to spread all over the compiler.

WaffleLapkin · 2024-09-20T09:43:22Z

compiler/rustc_middle/src/mir/syntax.rs

-    PointerCoercion(PointerCoercion),
-    /// Cast into a dyn* object.
-    DynStar,
+    PointerCoercion(PointerCoercion, CoercionSource),


I'm not well versed in dyn* object, does grouping them with pointers actually make sense? cc @compiler-errors

WaffleLapkin · 2024-09-20T09:57:47Z

tests/ui/kindck/kindck-impl-type-params.stderr

   |
 LL | fn foo<'a>() {
   |        -- lifetime `'a` defined here
 LL |     let t: S<&'a isize> = S(marker::PhantomData);
 LL |     let a = &t as &dyn Gettable<&'a isize>;
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type annotation requires that `'a` must outlive `'static`
+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^ type annotation requires that `'a` must outlive `'static`


question: why does this say "type annotation" instead of "cast" like most others?

Because borrow errors from type annotations are given special priority over errors from other sources (including casts) here:

rust/compiler/rustc_borrowck/src/region_infer/mod.rs

Line 2105 in 1a5a224

ConstraintCategory::TypeAnnotation

Changing this code at all makes many other diagnostics worse, so I'd prefer to leave it alone for now.

Also, this special casing can cause some diagnostic output that is just plainly wrong, so it should definitely be changed in the future.

trait Trait<'a> {} fn require_static(_: *const dyn Trait<'static>) {} fn not_static<'a>(a: *const dyn Trait<'a>) { require_static(a as *const dyn Trait<'a>); }

error: lifetime may not live long enough --> src/lib.rs:6:20 | 5 | fn not_static<'a>(a: *const dyn Trait<'a>) { | -- lifetime `'a` defined here 6 | require_static(a as *const dyn Trait<'a>); | ^^^^^^^^^^^^^^^^^^^^^^^^^ type annotation requires that `'a` must outlive `'static`

bors · 2024-09-20T23:31:25Z

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

This changes the remaining span for the cast, because the new `Cast` category has a higher priority (lower `Ord`) than the old `Coercion` category, so we no longer report the region error for the "unsizing" coercion from `*const Trait` to itself.

lukas-code · 2024-09-24T22:36:15Z

Rebased with no further changes. Per #130234 (review), I'll just go ahead and

@bors r=WaffleLapkin

Any further improvements can be landed separately.

bors · 2024-09-24T22:36:17Z

📌 Commit b62e72c has been approved by WaffleLapkin

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - rust-lang#130234 (improve compile errors for invalid ptr-to-ptr casts with trait objects) - rust-lang#130752 (Improve assembly test for CMSE ABIs) - rust-lang#130764 (Separate collection of crate-local inherent impls from error tracking) - rust-lang#130788 (Pin memchr to 2.5.0 in the library rather than rustc_ast) - rust-lang#130789 (add InProgress ErrorKind gated behind io_error_inprogress feature) - rust-lang#130793 (Mention `COMPILETEST_VERBOSE_CRASHES` on crash test failure) - rust-lang#130798 (rustdoc: inherit parent's stability where applicable) Failed merges: - rust-lang#130735 (Simple validation for unsize coercion in MIR validation) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-09-25T04:54:53Z

⌛ Testing commit b62e72c with merge 1b5aa96...

Rollup merge of rust-lang#130234 - lukas-code:ptr-cast-errors, r=WaffleLapkin improve compile errors for invalid ptr-to-ptr casts with trait objects This is a follow-up to rust-lang#120248 to improve some of its error messages. 1. Make the borrowcheck error for "type annotation requires that x must outlive y" actually point at the type annotation, i.e. the type `T` in a `x as T` cast. This makes the error more consistent with other errors caused by type annotation in other places, such as ```text error: lifetime may not live long enough --> src/lib.rs:4:12 | 3 | fn bar(a: &i32) { | - let's call the lifetime of this reference `'1` 4 | let b: &'static i32 = a; | ^^^^^^^^^^^^ type annotation requires that `'1` must outlive `'static` ``` 2. Don't say "cast" when we actually mean "coercion" and give borrowcheck errors from actual casts (which is currently just the check added in rust-lang#120248) a higher priority than ones from coercions. This can improve the errors for ptr-to-ptr cast between trait objects because they are are lowered as an upcast "unsizing" coercion if possible (which may be the identity upcast) followed by the actual cast. 3. Bring back the old "casting X as Y is invalid" message for type mismatch in the principals and reword the "vtable kinds may not match" message to more accurately describe the pointer metadata and not refer to "vtables" if the metadata is unknown. fixes rust-lang#130030 r? `@WaffleLapkin` but feel free to reassign

Changes required due to: - rust-lang/rust#130234 improve compile errors for invalid ptr-to-ptr casts with trait objects - rust-lang/rust@cfb8419900 Separate collection of crate-local inherent impls from error reporting - rust-lang/rust@40fca8f7a8 Bump Clippy version -> 0.1.83 Resolves: model-checking#3548

Changes required due to: - rust-lang/rust#130234 improve compile errors for invalid ptr-to-ptr casts with trait objects - rust-lang/rust@cfb8419900 Separate collection of crate-local inherent impls from error reporting - rust-lang/rust@40fca8f7a8 Bump Clippy version -> 0.1.83 Resolves: #3548 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

…piler-errors minor `*dyn` cast cleanup Small follow-up to rust-lang#130234 to remove a redundant check and clean up comments. No functional changes. Also, explain why casts cannot drop the principal even though coercions can, and add a test because apparently we didn't have one already. r? `@WaffleLapkin` or `@compiler-errors`

Rollup merge of rust-lang#131898 - lukas-code:ptr-cast-cleanup, r=compiler-errors minor `*dyn` cast cleanup Small follow-up to rust-lang#130234 to remove a redundant check and clean up comments. No functional changes. Also, explain why casts cannot drop the principal even though coercions can, and add a test because apparently we didn't have one already. r? `@WaffleLapkin` or `@compiler-errors`

rustbot assigned WaffleLapkin Sep 11, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 11, 2024

This comment has been minimized.

Sign in to view

WaffleLapkin reviewed Sep 12, 2024

View reviewed changes

compiler-errors reviewed Sep 12, 2024

View reviewed changes

compiler/rustc_hir_typeck/src/cast.rs Show resolved Hide resolved

lukas-code force-pushed the ptr-cast-errors branch from 0762b2c to f9b3109 Compare September 15, 2024 18:45

lukas-code commented Sep 15, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

lukas-code force-pushed the ptr-cast-errors branch from f9b3109 to 65b2679 Compare September 15, 2024 19:41

RalfJung reviewed Sep 15, 2024

View reviewed changes

WaffleLapkin approved these changes Sep 20, 2024

View reviewed changes

Lukas Markeffsky added 7 commits September 24, 2024 22:17

unify dyn* coercions with other pointer coercions

46ecb23

add another test

b52dea8

use more accurate spans for user type ascriptions

d1e82d4

be even more precise about "cast" vs "coercion"

bd31e3e

improve errors for invalid pointer casts

39f66ba

update doc comment

b62e72c

lukas-code force-pushed the ptr-cast-errors branch from 3a6f518 to b62e72c Compare September 24, 2024 21:12

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 24, 2024

tgross35 mentioned this pull request Sep 24, 2024

Rollup of 7 pull requests #130807

Merged

bors merged commit 75296fc into rust-lang:master Sep 25, 2024
6 checks passed

rustbot added this to the 1.83.0 milestone Sep 25, 2024

lukas-code deleted the ptr-cast-errors branch September 25, 2024 11:43

tautschnig mentioned this pull request Sep 26, 2024

Update toolchain to 2024-09-26 model-checking/kani#3549

Merged

lukas-code mentioned this pull request Oct 18, 2024

minor *dyn cast cleanup #131898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve compile errors for invalid ptr-to-ptr casts with trait objects #130234

improve compile errors for invalid ptr-to-ptr casts with trait objects #130234

lukas-code commented Sep 11, 2024

This comment has been minimized.

WaffleLapkin Sep 12, 2024

lukas-code Sep 12, 2024

WaffleLapkin Sep 12, 2024

WaffleLapkin Sep 12, 2024 •

edited

Loading

lukas-code Sep 12, 2024

lukas-code Sep 12, 2024

WaffleLapkin Sep 13, 2024

lukas-code Sep 15, 2024

rustbot commented Sep 15, 2024

lukas-code commented Sep 15, 2024

lukas-code Sep 15, 2024

saethlin Sep 15, 2024

This comment has been minimized.

rustbot commented Sep 15, 2024

RalfJung Sep 15, 2024

WaffleLapkin left a comment

WaffleLapkin Sep 20, 2024

lukas-code Sep 20, 2024

WaffleLapkin Sep 20, 2024

WaffleLapkin Sep 20, 2024

lukas-code Sep 20, 2024

lukas-code Sep 20, 2024

bors commented Sep 20, 2024

lukas-code commented Sep 24, 2024

bors commented Sep 24, 2024

bors commented Sep 25, 2024

	/// Where this call came from in HIR/THIR.
	call_source: CallSource,

	/// Represents how a [`CastKind::PointerCoercion`] was constructed, used for diagnostics.
	/// Represents how a [`CastKind::PointerCoercion`] was constructed.
	/// Used only for diagnostics.

improve compile errors for invalid ptr-to-ptr casts with trait objects #130234

improve compile errors for invalid ptr-to-ptr casts with trait objects #130234

Conversation

lukas-code commented Sep 11, 2024

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

WaffleLapkin Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Sep 15, 2024

lukas-code commented Sep 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Sep 15, 2024

Choose a reason for hiding this comment

WaffleLapkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 20, 2024

lukas-code commented Sep 24, 2024

bors commented Sep 24, 2024

bors commented Sep 25, 2024

WaffleLapkin Sep 12, 2024 •

edited

Loading