Consider using pointers and &raw operator in object_downcast implementation about anyhow

[dtolnay]

😐 I am becoming inclined to just leave it. As you wrote, the public API is sound and I definitely don't see this getting miscompiled. It's just going to be a thing to keep an eye on as the unsafe code guidelines get pinned down further, so I'll keep the issue open.

We can switch to &raw when it stabilizes.

I am hesitant to separate the vtable to two methods because it would be adding many more lines of unsafe code to the crate, and a bloatier vtable, for questionable benefit.

[Michael-F-Bryan]

I tried to derive a minimal example on the playground. @dtolnay, would you say the linked code is a faithful representation of Error::downcast_mut() and the object_downcast vtable method?

Running it under miri fails with a "trying to reborrow for Unique" error message. I'm guessing that means we're trying to get a unique borrow to something when we're not allowed?

   Compiling playground v0.0.1 (/playground)
error: Miri evaluation error: trying to reborrow for Unique, but parent tag <untagged> does not have an appropriate item in the borrow stack
  --> src/main.rs:15:13
   |
15 |             &mut *ptr.cast().as_ptr()
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^ trying to reborrow for Unique, but parent tag <untagged> does not have an appropriate item in the borrow stack
   |
note: inside call to `TopLevel::get_field` at src/main.rs:32:20
  --> src/main.rs:32:20
   |
32 |     println!("{}", top_level.get_field());
   |                    ^^^^^^^^^^^^^^^^^^^^^
   = note: inside call to `main` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:67:34
   = note: inside call to closure at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:73
   = note: inside call to closure at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/sys_common/backtrace.rs:129:5
   = note: inside call to `std::sys_common::backtrace::__rust_begin_short_backtrace::<[closure@DefId(1:6019 ~ std[8be3]::rt[0]::lang_start_internal[0]::{{closure}}[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:13
   = note: inside call to closure at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:305:40
   = note: inside call to `std::panicking::r#try::do_call::<[closure@DefId(1:6018 ~ std[8be3]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:281:13
   = note: inside call to `std::panicking::r#try::<i32, [closure@DefId(1:6018 ~ std[8be3]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe]>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:394:14
   = note: inside call to `std::panic::catch_unwind::<[closure@DefId(1:6018 ~ std[8be3]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:51:25
   = note: inside call to `std::rt::lang_start_internal` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:67:5
   = note: inside call to `std::rt::lang_start::<()>`

error: aborting due to previous error

error: could not compile `playground`.

To learn more, run the command again with --verbose.

[dtolnay]

Yeah this is worth fixing, good catch.

Ideally vtable's object_downcast would be defined only in terms of pointers and never touch a reference, but that may need &raw?

[Michael-F-Bryan]

I was thinking you could pass in raw pointer instead of a &ErrorImpl<()>. Maybe NonNull<ErrorImpl<()>> or *const ErrorImpl<()>?

Miri doesn't complain after switching the playground example to NonNull.

[dtolnay]

In your new playground link &inner.field still makes a shared reference that we convert to NonNull then &mut, which is no better than the current code in anyhow. It is strange that Miri doesn't catch this but maybe neither of us is clear on the exact rule. The new raw-reference operator would allow &raw inner.field but it's not stable yet.

[Michael-F-Bryan]

I think that's because NonNull::from(&self.inner) immediately creates a raw pointer to self.inner, so miri sees we create a shared reference and immediately release it, and only then is object_downcast called. But you're totally correct that this is no better than the current code, it's just miri may not complain about our new formulation.

Looking at the RFC, am I right in saying &raw lets you create a *mut pointer directly to a field/variable instead of the current &mut thing as *mut Thing which requires us to create a valid &mut reference?

[dtolnay]

Yes the operator is for raw-pointer-to-struct ⟶ raw-pointer-to-field, which isn't currently exposed without passing through a reference.

I would prefer not to accept the change from the playground link unless we better understand why it makes a difference to Miri. My guess is that neither playground is strictly conforming to the stacked borrow rules (both do effectively the exact same sequence of casts) but Miri is suppressing warnings on the latter playground as a concession to practicality because it is such a widespread pattern for now, in which case I feel fine sticking with the current code which does the same widespread thing.

It could be that the best workaround is to split the vtable to have distinct object_downcast_ref and object_downcast_mut function pointers with the same behavior but & vs &mut in the right place. I don't love it because it feels bloaty, but it would probably pass Miri.

[Michael-F-Bryan]

Splitting object_downcast into two vtable methods sounds reasonable, but considering it would just be a bunch of copy/paste replacing & with &mut I agree that it's not overly satisfying.

I guess to answer the question posed in this issue's title, our best answer is "I dunno", isn't it?

In practice this code is okay, as in the &mut self argument to Error::downcast_mut() ensures it's not possible for the caller to violate Rust's borrowing rules and the code runs fine today. I'm not confident enough with the theory to say whether either the current code or the proposed solution is sound, but the unsafe code still provides a safe interface to users and there's no way I can see for someone to abuse it.

Are we able to get a second opinion?