Stream: t-compiler/help

Topic: lazy norm + object safety


lcnr (May 20 2020 at 21:40, on Zulip):

Working on #72219 part 1: Object safety interactions with constants

I managed to find a way to use the currently missing object safety check to achieve greatness (aka segfaults) :sparkles:

#![feature(const_generics)]
#![feature(const_type_id, core_intrinsics)]

struct Const<const N: usize>([&'static u32; N]);

trait A: 'static {
    fn foo(&self) -> Const<{ std::intrinsics::type_id::<Self>() as u8 as usize }>;
}

impl<T: 'static> A for T {
    fn foo(&self) -> Const<{ std::intrinsics::type_id::<Self>() as u8 as usize }> {
        Const([&1; std::intrinsics::type_id::<Self>() as u8 as usize])
    }
}

fn main() {
    let x: &dyn A = &7i32;
    let foo = A::foo(x);
    // A is not object safe, as the return type of `foo` depends on `Self`.
    // This is unsound.
    println!("{:?}", &foo.0[..]);
}
lcnr (May 20 2020 at 21:40, on Zulip):

cc @nikomatsakis @varkor on how to actually check unevaluated consts for object safety violations.

lcnr (May 20 2020 at 21:42, on Zulip):

Do we want to walk the MIR of the const argument to check if it uses self?

lcnr (May 20 2020 at 22:01, on Zulip):

Thinking about this a bit more, I am not completely sure if this is well formed :thinking:
So this test may stop relying on object safety checking once https://github.com/rust-lang/rust/pull/70107 is merged.

nikomatsakis (May 21 2020 at 09:38, on Zulip):

Hmm.

nikomatsakis (May 21 2020 at 09:39, on Zulip):

So my assumption, which I realize now was flawed or at least non-obvious, is that we would want to limit references to Self by constants apart from something like <Self as Trait<..>>::Constant

nikomatsakis (May 21 2020 at 09:39, on Zulip):

But I realize that -- at least with our current setup -- we don't have visibility into that, or at least we'd have to inspect the MIR or something

nikomatsakis (May 21 2020 at 09:39, on Zulip):

Which I guess is your point

nikomatsakis (May 21 2020 at 09:39, on Zulip):

Welp that's going to be annoying

lcnr (May 21 2020 at 16:51, on Zulip):

Would it be possible to only mention needed subst of ConstKind::Unevaluated during construction,
so instead of identity substs in https://github.com/rust-lang/rust/blob/06c9fef822b890054fcefa9a567b57eb6edfe638/src/librustc_middle/ty/sty.rs#L2273 we try to only add actually used params?

lcnr (May 21 2020 at 16:52, on Zulip):

This would also allow me to fix https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/substs.20of.20Unevaluated.20consts/near/198163517 by simply checking for cycles in generalize/equate.

lcnr (May 21 2020 at 16:53, on Zulip):

This would mean that the object safety check is slightly conservative, but tbh I feel fine with that :sweat_smile:

nikomatsakis (May 21 2020 at 18:09, on Zulip):

that would certainly solve various problems

nikomatsakis (May 21 2020 at 18:09, on Zulip):

it seems like it should be possible

nikomatsakis (May 21 2020 at 18:09, on Zulip):

at minimum we could special case "simple" constants like 22

nikomatsakis (May 21 2020 at 18:09, on Zulip):

and/or path-based constants like X or T::Foo

nikomatsakis (May 21 2020 at 18:09, on Zulip):

(Do those require {...} syntax?)

lcnr (May 21 2020 at 18:18, on Zulip):

Paths do require braces unless they are only 1 segment and not in the type namespace.

lcnr (May 21 2020 at 18:18, on Zulip):

at minimum we could special case "simple" constants like 22

We already do :laughing:, which is why 7 sometimes works where { 7 } doesn't.

lcnr (May 21 2020 at 18:19, on Zulip):

https://github.com/rust-lang/rust/blob/06c9fef822b890054fcefa9a567b57eb6edfe638/src/librustc_middle/ty/sty.rs#L2241

lcnr (May 21 2020 at 18:20, on Zulip):

I don't think braces are problematic, as we can just unwrap them in Const::from_anon_const.

lcnr (May 21 2020 at 18:22, on Zulip):

Afaict we have to minimize substs using Hir, as optimized_mir probably causes cycle errors here

lcnr (May 21 2020 at 18:24, on Zulip):

A good thing is that afaik consts can only depend on generic params if they explicitly mention them, so this may be implementable using a rather simple hir Visitor

lcnr (May 21 2020 at 18:29, on Zulip):

^ I would like some confirmation for the last point so I don't miss some strange edge case which breaks this approach after it's already in use

lcnr (May 22 2020 at 18:05, on Zulip):

Simply discarding irrelevant substs does not work, as we currently use indexing in a bunch of places, not sure how much we rely on this though,
as ICE tend to stop any further execution :laughing:

lcnr (May 22 2020 at 18:08, on Zulip):

Two possible ways I can think of are to either update the generics_of query for AnonConst.

lcnr (May 22 2020 at 18:12, on Zulip):

I am not sure how well this would work,. We rely on Generics.parent for relevant parameters which we have to stop doing as consts may only depend on some of the defined params.

lcnr (May 22 2020 at 18:14, on Zulip):

The other option would be to use a bunch of duct tape everywhere substs are used. This seems like a horrible idea though

lcnr (May 22 2020 at 18:18, on Zulip):

Note that while object safety can be fixed without touching substs, I don't think there is a different solution which allows us to fix https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/substs.20of.20Unevaluated.20consts/near/198163517

nikomatsakis (May 22 2020 at 20:51, on Zulip):

@lcnr I'll try to schedule some time to get back to you on this, I feel like I need to sit down and think a bit about it

nikomatsakis (May 22 2020 at 20:51, on Zulip):

might be useful to find some time to talk about it together

lcnr (May 22 2020 at 22:12, on Zulip):

I only have a few weekly lectures and can spend the rest of my time fairly liberally atm. It should be fairly
easy to find a date which works for me.

lcnr (May 28 2020 at 21:03, on Zulip):

ping @nikomatsakis. Is this still on your radar?

mark-i-m (May 28 2020 at 22:19, on Zulip):

/me checks Niko's radar radar.jpeg

mark-i-m (May 28 2020 at 22:20, on Zulip):

:heart:

Last update: Sep 27 2020 at 14:15UTC