Trait upcasting coercion (part4) #88010

crlf0710 · 2021-08-13T14:02:41Z

This is a incomplete version of trait upcasting unsafety checking. I think it's working in principle, however i think i meet a problem here. How can i know Box<T> or Arc<T>'s internal pointer can be seen as "always-valid"?

cc @RalfJung

rust-highfive · 2021-08-13T14:02:44Z

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

rust-log-analyzer · 2021-08-13T14:13:39Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/compiler/rustc_mir/src/transform/mod.rs at line 5:
 use rustc_hir as hir;
 use rustc_hir::def_id::{DefId, LocalDefId};
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
+use rustc_hir::lang_items::LangItem;
 use rustc_index::vec::IndexVec;
 use rustc_middle::mir::visit::Visitor as _;
 use rustc_middle::mir::{traversal, Body, ConstQualifs, MirPhase, Promoted};
Diff in /checkout/compiler/rustc_mir/src/transform/mod.rs at line 12:
 use rustc_middle::ty::query::Providers;
 use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable};
 use rustc_span::{Span, Symbol};
-use rustc_hir::lang_items::LangItem;
 use std::borrow::Cow;
 
 
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_mir/src/transform/simplify.rs" "/checkout/compiler/rustc_mir/src/transform/deduplicate_blocks.rs" "/checkout/compiler/rustc_mir/src/transform/mod.rs" "/checkout/compiler/rustc_mir/src/transform/multiple_return_terminators.rs" "/checkout/compiler/rustc_mir/src/transform/separate_const_switch.rs" "/checkout/compiler/rustc_mir/src/transform/check_packed_ref.rs" "/checkout/compiler/rustc_mir/src/transform/lower_intrinsics.rs" "/checkout/compiler/rustc_mir/src/transform/abort_unwinding_calls.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.

RalfJung · 2021-08-13T15:59:41Z

compiler/rustc_mir/src/transform/mod.rs

@@ -625,3 +628,27 @@ fn promoted_mir<'tcx>(

    tcx.arena.alloc(promoted)
 }
+
+pub(crate) fn custom_coerce_unsize_info<'tcx>(


Maybe this function would be better put somewhere in rustc_mir/src/util?

should i create a new module in util for this?

If none of the others seem to fit, yeah.

compiler/rustc_mir/src/transform/check_unsafety.rs

RalfJung · 2021-08-13T16:01:01Z

compiler/rustc_mir/src/transform/check_unsafety.rs

@@ -132,6 +132,24 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
                    self.register_violations(&violations, &unsafe_blocks);
                }
            },
+            Rvalue::Cast(kind, source, mir_cast_ty) => {
+                if let CastKind::Pointer(ty::adjustment::PointerCast::Unsize) = kind {
+                    let mir_source_ty = match *source {


You should be able to use Operand::ty here, right?

I'm not totally sure, since this is earlier than codegen phase, if i'm not misunderstanding it. I'll take a look.

This has nothing to do with codegen. I think you basically inlined Operand::ty here.

RalfJung · 2021-08-13T16:02:25Z

compiler/rustc_mir/src/transform/check_unsafety.rs

+    fn is_trait_upcasting_coercion_pointee(&self, source: Ty<'tcx>, target: Ty<'tcx>) -> bool {
+        match (source.kind(), target.kind()) {
+            (&ty::Dynamic(data_a, ..), &ty::Dynamic(data_b, ..)) => {
+                data_a.principal_def_id() != data_b.principal_def_id()


Why is it okay to allow casts where data_a.principal_def_id() == data_b.principal_def_id()? Please add a comment.

RalfJung · 2021-08-13T16:04:11Z

src/test/ui/traits/trait-upcasting/unsafe-trait-upcasting-coercion2.rs

+    //~^ ERROR mismatched types
+}
+
+fn main() {}


Why are these two separate files?

Please also add some casts that are okay.

Does this hit the last match arm in is_trait_upcasting_coercion_from_raw_pointer?

This is a separate file because the "mismatched types" error will prevent unsafety checker from running at all, it seems. I don't really know whether error recovery is possible at all in this case. This test case is just the status quo. I'll leave comments, when the unsafety checking implementation is ready.

Please also add some casts that are okay.

I will, thanks!

Does this hit the last match arm in is_trait_upcasting_coercion_from_raw_pointer?

No it doesn't, the unsafety checker is not yet running in this case.

No it doesn't, the unsafety checker is not yet running in this case.

Okay, then the other test should definitely do something to hit that arm.

RalfJung · 2021-08-13T16:05:14Z

however i think i meet a problem here. How can i know Box or Arc's internal pointer can be seen as "always-valid"?

Good question. Currently you seem to be doing some kind of recursion here, but I am not sure what this means in terms of the resulting behavior.

Is it correct that that Adt arm in is_trait_upcasting_coercion_from_raw_pointer can only be reached for types that implement CoerceUnsized?

Cc @rust-lang/lang

crlf0710 · 2021-08-14T05:31:40Z

@RalfJung i think the answer is yes, according to CoerceUnsized documentation. It even covers all the Non-ADT cases, like the reference and raw pointer types.

On the other hand the reference says something unintuitive: It says TyCtor(T) to TyCtor(U). Not sure if whether this clause is intended to cover the CoerceUnsized cases.

I think my current problem is that, the CoerceUnsized trait doesn't say anything semantically about what's happening underhood. It is defined in a very structured typing approach.

RalfJung · 2021-08-14T13:13:40Z

Yeah, it sounds like for full expressivity we'd need CoerceUnsizedUnsafe -- for smart pointers that wrap a raw pointer and don't guarantee via a custom invariant that it is valid.

For now, I think assuming they are safe to cast (if the recursive check succeeds) makes sense. But this should be added to the list of unresolved questions for trait coercions, to be brought up in the stabilization report.

Also there should be tests for things like *const Cell<dyn T> to *const Cell<dyn U>, to ensure that this requires unsafe (if it works at all).

crlf0710 · 2021-08-18T11:07:14Z

I'll let this PR wait on #88135 a little, and discuss more about CoerceUnsized during the review of that pr.

crlf0710 · 2021-08-22T16:41:59Z

#88135 has landed, and i created #88239 for a less ad-hoc approach to gather feedback and discussions.

RalfJung · 2021-08-23T14:02:38Z

I'll be pretty busy for some time since I am moving, so I am probably not the best reviewer here.

crlf0710 · 2021-08-27T05:01:26Z

That's ok, after some discussions i think this is not the prefered approach here. Closing for now, but might get back to this if necessary.

crlf0710 added 2 commits August 13, 2021 21:58

Implement unsafety checking for trait upcasting coercion.

1862e98

Take care of the ADT unsizing case.

cff23a0

rust-highfive assigned davidtwco Aug 13, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2021

crlf0710 mentioned this pull request Aug 13, 2021

Tracking issue for dyn upcasting coercion #65991

Open

21 tasks

crlf0710 changed the title ~~Trait upcasting part3~~ Trait upcasting coercion (part3) Aug 13, 2021

RalfJung reviewed Aug 13, 2021

View reviewed changes

compiler/rustc_mir/src/transform/check_unsafety.rs Show resolved Hide resolved

RalfJung reviewed Aug 13, 2021

View reviewed changes

crlf0710 changed the title ~~Trait upcasting coercion (part3)~~ Trait upcasting coercion (part4) Aug 18, 2021

crlf0710 mentioned this pull request Aug 22, 2021

[Experimental] Split out an UnsafeCoerceUnsized trait from CoerceUnsized #88239

Closed

davidtwco assigned RalfJung and unassigned davidtwco Aug 23, 2021

crlf0710 closed this Aug 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trait upcasting coercion (part4) #88010

Trait upcasting coercion (part4) #88010

crlf0710 commented Aug 13, 2021

rust-highfive commented Aug 13, 2021

rust-log-analyzer commented Aug 13, 2021

RalfJung Aug 13, 2021

crlf0710 Aug 13, 2021

RalfJung Aug 14, 2021

RalfJung Aug 13, 2021

crlf0710 Aug 13, 2021 •

edited

Loading

RalfJung Aug 14, 2021

RalfJung Aug 13, 2021

crlf0710 Aug 13, 2021

RalfJung Aug 13, 2021

crlf0710 Aug 13, 2021 •

edited

Loading

RalfJung Aug 14, 2021

RalfJung commented Aug 13, 2021

crlf0710 commented Aug 14, 2021 •

edited

Loading

RalfJung commented Aug 14, 2021

crlf0710 commented Aug 18, 2021

crlf0710 commented Aug 22, 2021

RalfJung commented Aug 23, 2021

crlf0710 commented Aug 27, 2021

Trait upcasting coercion (part4) #88010

Trait upcasting coercion (part4) #88010

Conversation

crlf0710 commented Aug 13, 2021

rust-highfive commented Aug 13, 2021

rust-log-analyzer commented Aug 13, 2021

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

crlf0710 Aug 13, 2021 • 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

crlf0710 Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Aug 13, 2021

crlf0710 commented Aug 14, 2021 • edited Loading

RalfJung commented Aug 14, 2021

crlf0710 commented Aug 18, 2021

crlf0710 commented Aug 22, 2021

RalfJung commented Aug 23, 2021

crlf0710 commented Aug 27, 2021

crlf0710 Aug 13, 2021 •

edited

Loading

crlf0710 Aug 13, 2021 •

edited

Loading

crlf0710 commented Aug 14, 2021 •

edited

Loading