Stream: wg-traits

Topic: coherence bypasses for traits #57893


nikomatsakis (Sep 13 2019 at 19:27, on Zulip):

Hi @blitzerr -- sounds good. I wanted to point you to this branch on my repository:

nikomatsakis (Sep 13 2019 at 19:28, on Zulip):

https://github.com/nikomatsakis/rust/tree/degenerate-object-safe-issue-57893

nikomatsakis (Sep 13 2019 at 19:28, on Zulip):

I went up taking a slightly different path than what I described in those mentoring instructions, in that I rearranged the ordering. I think the code is mostly prepared now for the so-called "degenerate" cases, but the function that looks for them needs to be written still.

centril (Sep 13 2019 at 19:34, on Zulip):

@nikomatsakis nice points re. dyn trait A as sugar for trait A + where dyn A: A & re. dyn trait Iterator { ... }

centril (Sep 13 2019 at 19:35, on Zulip):

this fits nicely together with 2027

centril (Sep 13 2019 at 19:37, on Zulip):

(the other zulip thread is https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.2357893.20coherence.20and.20traits/near/169073560 which has other examples due to @Ariel Ben-Yehuda)

blitzerr (Sep 13 2019 at 20:03, on Zulip):

@nikomatsakis thanks for the link. Will look into it.

Ariel Ben-Yehuda (Sep 13 2019 at 20:04, on Zulip):

This approach looks right to me

Ariel Ben-Yehuda (Sep 13 2019 at 20:05, on Zulip):

And I don't think we need to change anything but selection - projection won't use an object candidate if selection doesn't return one.

blitzerr (Sep 23 2019 at 00:45, on Zulip):

@nikomatsakis I don't how can I check if user has already implemented a variant that can be used for Trait Object ?

blitzerr (Sep 23 2019 at 00:59, on Zulip):

How do I write the logic solver for impl_potentially_overlapping_dyn_trait ? Is there an example I can look at ?

blitzerr (Sep 23 2019 at 03:34, on Zulip):

@Ariel Ben-Yehuda ^

nikomatsakis (Sep 23 2019 at 20:17, on Zulip):

@blitzerr

nikomatsakis (Sep 23 2019 at 20:18, on Zulip):

What I had in mind was to start with a fairly dumb check

blitzerr (Sep 23 2019 at 20:18, on Zulip):

hi :wave:

nikomatsakis (Sep 23 2019 at 20:19, on Zulip):

In particular, I wrote the following in the issue

So clearly the (semi-)syntactic check I was first envisioning is too simplistic. What we want is to see whether there is some impl of SomeTrait whose self type could be dyn SomeTrait (or equivalent to it). This can happen in basically three ways:

- a type parameter that is ?Sized
- an associated type projection (with any parameters) that is ?Sized
- a dyn SomeTrait type (which we already check for)
- an opaque type, but those are disallowed and the semantics are unclear anyway
- a type alias that equates to one of those things

nikomatsakis (Sep 23 2019 at 20:19, on Zulip):

So, the idea would be this. We will iterate over the impls of the trait

nikomatsakis (Sep 23 2019 at 20:19, on Zulip):

and we will look at the "self-type" of each impl

nikomatsakis (Sep 23 2019 at 20:19, on Zulip):

we can basically do match self_ty.sty { ... }

nikomatsakis (Sep 23 2019 at 20:20, on Zulip):

and then try to look for those cases I listed

nikomatsakis (Sep 23 2019 at 20:20, on Zulip):

So, the idea would be this. We will iterate over the impls of the trait

so let's start with this

nikomatsakis (Sep 23 2019 at 20:20, on Zulip):

we have this function

nikomatsakis (Sep 23 2019 at 20:20, on Zulip):
fn impl_potentially_overlapping_dyn_trait(self, trait_def_id: DefId) -> bool
blitzerr (Sep 23 2019 at 20:21, on Zulip):

Sounds good.

nikomatsakis (Sep 23 2019 at 20:21, on Zulip):

we can get the TraitDef for the given id by invoking tcx.trait_def(trait_def_id)

nikomatsakis (Sep 23 2019 at 20:21, on Zulip):

this has a method already calls for_each_impl

nikomatsakis (Sep 23 2019 at 20:22, on Zulip):

this is .. probably not what we really want to use, because it's kind of inefficient, but it's ok to start

nikomatsakis (Sep 23 2019 at 20:22, on Zulip):

so you can do trait_def.for_each_impl(|impl_def_id| { ... })

nikomatsakis (Sep 23 2019 at 20:23, on Zulip):

this is .. probably not what we really want to use, because it's kind of inefficient, but it's ok to start

it's inefficient because it literally iterates over all impls. But most of them are clearly not relevant -- e.g., an impl like impl Foo for u32 doesn't apply. We have the impls sorted by their "approximate self-type" precisely to make this sort of check faster. But, as I said, we can worry about that a bit later.

nikomatsakis (Sep 23 2019 at 20:24, on Zulip):

Once you have the impl, you can get its precise self type by using the type_of(impl_def_id) query

nikomatsakis (Sep 23 2019 at 20:24, on Zulip):

then we can try matching that

nikomatsakis (Sep 23 2019 at 20:24, on Zulip):

so now we have something like

nikomatsakis (Sep 23 2019 at 20:26, on Zulip):
let mut found_match = false;
tcx.trait_def(trait_def_id).for_each_impl(|impl_def_id| {
    let impl_self_ty = tcx.type_of(impl_def_id);
    match impl_self_ty.sty {
        ty::Param(param_ty) => ..., // one line for each of the cases above
        ...,
    }
});
nikomatsakis (Sep 23 2019 at 20:27, on Zulip):

rustdoc for ty::Param

nikomatsakis (Sep 23 2019 at 20:27, on Zulip):

so that match above will match any type parameter, but we want only ?Sized type parameters

nikomatsakis (Sep 23 2019 at 20:28, on Zulip):

that's a bit annoying, I have to look, but one way to find that out would to ask for tcx.predicates_of(impl_def_id), which will give you where clauses from the impl

nikomatsakis (Sep 23 2019 at 20:28, on Zulip):

you could look for one that matches impl_self_ty: Sized

nikomatsakis (Sep 23 2019 at 20:28, on Zulip):

there might be another place where we record the relevant info tho

nikomatsakis (Sep 23 2019 at 20:28, on Zulip):

or some similar code

nikomatsakis (Sep 23 2019 at 20:28, on Zulip):

anyway, I'd say just start with that one case

nikomatsakis (Sep 23 2019 at 20:28, on Zulip):

we can come back to these:

- an associated type projection (with any parameters) that is ?Sized
- a dyn SomeTrait type (which we already check for)
- an opaque type, but those are disallowed and the semantics are unclear anyway

nikomatsakis (Sep 23 2019 at 20:29, on Zulip):

really, only the second one needs work anyway

nikomatsakis (Sep 23 2019 at 20:29, on Zulip):

with me so far?

nikomatsakis (Sep 23 2019 at 20:29, on Zulip):

ok wait

nikomatsakis (Sep 23 2019 at 20:29, on Zulip):

you could look for one that matches impl_self_ty: Sized

there is some code for this already in traits/object_safety.rs that could be refactored to suit our puposes

nikomatsakis (Sep 23 2019 at 20:30, on Zulip):

in particular this function, trait_has_sized_self checks whether the trait declares a predicate where Self: Sized

nikomatsakis (Sep 23 2019 at 20:31, on Zulip):

and in particular this line is the one that is checking for Self -- note that Self is a type parameter of the trait

blitzerr (Sep 23 2019 at 20:31, on Zulip):

Rookie question - why is Sized the only one in which case a conflict can occur ?

blitzerr (Sep 23 2019 at 20:31, on Zulip):

Can't it be any associative type ?

blitzerr (Sep 23 2019 at 20:31, on Zulip):

@nikomatsakis

nikomatsakis (Sep 23 2019 at 20:31, on Zulip):

we are looking for impls that could apply where the self type is dyn Foo -- that is an unsized type (that is, dyn Foo: Sized is false)

nikomatsakis (Sep 23 2019 at 20:32, on Zulip):

this isn't the most precise check, but it's an easy one

nikomatsakis (Sep 23 2019 at 20:32, on Zulip):

that is, if we know that T: Sized, then we rule out the possibility that T == dyn Foo

nikomatsakis (Sep 23 2019 at 20:32, on Zulip):

(there could be other ways to rule that out, too)

nikomatsakis (Sep 23 2019 at 20:32, on Zulip):

it's "sufficient but not necessary", I guess

blitzerr (Sep 23 2019 at 20:33, on Zulip):

Thank you so much for your time and explaining this to me @nikomatsakis :pray:

blitzerr (Sep 23 2019 at 20:34, on Zulip):

I will give it a shot (tonight, hopefully)

nikomatsakis (Sep 23 2019 at 20:37, on Zulip):

@blitzerr great! let me know how it goes of course.

nikomatsakis (Sep 23 2019 at 20:37, on Zulip):

in particular this function, trait_has_sized_self checks whether the trait declares a predicate where Self: Sized

I think what we want to do here, to finish the thought, is refactor this to something like generics_require_param_sized(self, def_id: DefId, param_ty: ParamTy) -> bool { ... }

blitzerr (Sep 23 2019 at 20:37, on Zulip):

Absolutely @nikomatsakis :grinning:

nikomatsakis (Sep 23 2019 at 20:38, on Zulip):

and then in place of is_param(0) we'd invoke is_param(param_ty.index)

nikomatsakis (Sep 23 2019 at 20:38, on Zulip):

the existing callers can use ParamTy::for_self()

nikomatsakis (Sep 23 2019 at 20:38, on Zulip):

to create a ParamTy

nikomatsakis (Sep 23 2019 at 20:38, on Zulip):

your new caller would be using the param_ty it extracted via the match

nikomatsakis (Sep 23 2019 at 20:39, on Zulip):
let mut found_match = false;
tcx.trait_def(trait_def_id).for_each_impl(|impl_def_id| {
    let impl_self_ty = tcx.type_of(impl_def_id);
    match impl_self_ty.sty {
        ty::Param(param_ty) if self.generics_require_param_sized(impl_def_id, param_ty) => found_match = true,
        _ => { }
    }
});
found_match

so something like that :point_up:

blitzerr (Sep 23 2019 at 20:39, on Zulip):

Some of this will only make sense to me after I spend some time looking at the code. :grinning:

nikomatsakis (Sep 23 2019 at 20:39, on Zulip):

yep. I'm done typing now, let me know how it goes :)

blitzerr (Sep 23 2019 at 20:39, on Zulip):

Sure :grinning:

blitzerr (Sep 23 2019 at 20:40, on Zulip):

Typing is good. I can always revisit to connect the pieces that way. @nikomatsakis

blitzerr (Sep 29 2019 at 05:52, on Zulip):

I made the first set of changes @nikomatsakis .

blitzerr (Sep 29 2019 at 05:54, on Zulip):

I am testing this with

trait Object<U> {
    type Output;
}

impl<T: ?Sized, U> Object<U> for T {
    type Output = U;
}

fn foo<T: ?Sized, U>(x: <T as Object<U>>::Output) -> U {
    x
}

fn transmute<T, U>(x: T) -> U {
    foo::<dyn Object<U, Output = T>, U>(x)
}
blitzerr (Sep 29 2019 at 05:55, on Zulip):
[DEBUG rustc::traits::object_safety] trait_pred.def_id() == sized_def_id: false ;; trait_pred.skip_binder().self_ty().is_param(param_ty.index): true
[DEBUG rustc::traits::object_safety] trait_pred.def_id() == sized_def_id: true ;; trait_pred.skip_binder().self_ty().is_param(param_ty.index): false
[DEBUG rustc::traits::object_safety] trait_pred.def_id() == sized_def_id: true ;; trait_pred.skip_binder().self_ty().is_param(param_ty.index): false
[DEBUG rustc::traits::object_safety] trait_pred.def_id() == sized_def_id: false ;; trait_pred.skip_binder().self_ty().is_param(param_ty.index): true
[DEBUG rustc::traits::object_safety] trait_pred.def_id() == sized_def_id: true ;; trait_pred.skip_binder().self_ty().is_param(param_ty.index): false
[DEBUG rustc::traits::object_safety] trait_pred.def_id() == sized_def_id: true ;; trait_pred.skip_binder().self_ty().is_param(param_ty.index): false
[DEBUG rustc::traits::object_safety] Match found = false; for param_ty T
[DEBUG rustc::traits::object_safety] trait_pred.def_id() == sized_def_id: false ;; trait_pred.skip_binder().self_ty().is_param(param_ty.index): true
blitzerr (Sep 29 2019 at 05:56, on Zulip):

For

impl<T: ?Sized, U> Object<U> for T {
    type Output = U;
}

(from above), the type parameter is ?Sized, so we should have got true in the log line
[DEBUG rustc::traits::object_safety] Match found = false; for param_ty T
right ?

nikomatsakis (Sep 30 2019 at 14:09, on Zulip):

@blitzerr yep, I would think so, I'll try to look more closely in a bit

nikomatsakis (Sep 30 2019 at 17:33, on Zulip):

@blitzerr Sorry, my fault. The match occurs if self.generics_require_sized_param returns false -- that is, we are looking for an impl like impl<T: ?Sized> Foo for T

blitzerr (Sep 30 2019 at 17:35, on Zulip):

@nikomatsakis ? means may or maynot, is it ?

nikomatsakis (Sep 30 2019 at 17:36, on Zulip):

@blitzerr right -- by default, if you have impl<T>, we add an implicit T: Sized. The T: ?Sized notation suppresses that default

blitzerr (Sep 30 2019 at 17:36, on Zulip):

Okay, that would make sense

blitzerr (Sep 30 2019 at 17:39, on Zulip):

@nikomatsakis I was wondering, if there is a way to see the generated code, like the additional impl that would be added by the compiler (that we are trying to solve) ?

nikomatsakis (Sep 30 2019 at 17:56, on Zulip):

@blitzerr no, that impl is sort of "hard-coded" into the trait solver

blitzerr (Sep 30 2019 at 18:35, on Zulip):

@nikomatsakis I see. Is it true for the implicit T: Sized also ?

blitzerr (Sep 30 2019 at 18:36, on Zulip):

(That no way to see it somehow).

nikomatsakis (Sep 30 2019 at 19:10, on Zulip):

@blitzerr no, the T: Sized does show up in the result of predicates and so forth, but I don't know an easy way to dump that out

blitzerr (Sep 30 2019 at 19:48, on Zulip):

I see. Thanks @nikomatsakis

blitzerr (Sep 30 2019 at 19:48, on Zulip):

What should be the next steps here? Should we do the optimizations ?

blitzerr (Sep 30 2019 at 19:51, on Zulip):

And the tests @nikomatsakis

nikomatsakis (Sep 30 2019 at 20:01, on Zulip):

Well @blitzerr do you have it working now?

nikomatsakis (Sep 30 2019 at 20:01, on Zulip):

I think the next step is to try and see what breaks first, and if we've fixed the bug

nikomatsakis (Sep 30 2019 at 20:07, on Zulip):

@blitzerr what branch are you working off on in your repo?

nikomatsakis (Sep 30 2019 at 20:08, on Zulip):

ah I guess https://github.com/blitzerr/rust/tree/degenerate-object-safety-57893

nikomatsakis (Sep 30 2019 at 20:08, on Zulip):

well, I'd say to try that code (once you fix the bug) and see whether the weaponized transmute example gives an error or not

blitzerr (Sep 30 2019 at 20:11, on Zulip):

@nikomatsakis degenerate-object-safety-57893 branch

blitzerr (Sep 30 2019 at 20:21, on Zulip):
 build/x86_64-apple-darwin/stage1/bin/rustc weaponized_transmute.rs                                    341ms  Mon Sep 30 13:20:55 2019
error[E0391]: cycle detected when determine object safety of trait `std::ops::Deref`
  --> /Users/blitz/rustc-dev/rust/src/libcore/ops/deref.rs:64:1
   |
64 | pub trait Deref {
   | ^^^^^^^^^^^^^^^
   |
   = note: ...which requires normalizing `ParamEnvAnd { param_env: ParamEnv { caller_bounds: [Binder(TraitPredicate(<Self as std::ops::Deref>))], reveal: All, def_id: None }, value: &dyn std::ops::Deref<Target = <Self as std::ops::Deref>::Target> }`...
   = note: ...which requires normalizing `Canonical { max_universe: U0, variables: [], value: ParamEnvAnd { param_env: ParamEnv { caller_bounds: [Binder(TraitPredicate(<Self as std::ops::Deref>))], reveal: All, def_id: None }, value: ProjectionTy { substs: [Self], item_def_id: DefId(2:1704 ~ core[d402]::ops[0]::deref[0]::Deref[0]::Target[0]) } } }`...
   = note: ...which again requires determine object safety of trait `std::ops::Deref`, completing the cycle
   = note: cycle used when evaluating trait selection obligation `<T as std::ops::Deref>::Target == U`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0391`.
blitzerr (Sep 30 2019 at 20:25, on Zulip):

@nikomatsakis I am getting this error currently :point_of_information:

nikomatsakis (Sep 30 2019 at 20:27, on Zulip):

sigh

nikomatsakis (Sep 30 2019 at 20:32, on Zulip):

@blitzerr did rustc bootstrap though?

nikomatsakis (Sep 30 2019 at 20:33, on Zulip):

hmm

nikomatsakis (Sep 30 2019 at 20:33, on Zulip):

I don't think we need the full normalization here

nikomatsakis (Sep 30 2019 at 20:36, on Zulip):

ok @blitzerr hmm maybe I was wrong to suggest re-using the same helper.

nikomatsakis (Sep 30 2019 at 20:39, on Zulip):

in particular that helper is used from two distinct places

nikomatsakis (Sep 30 2019 at 20:39, on Zulip):

actually 3

nikomatsakis (Sep 30 2019 at 20:40, on Zulip):

one of them requires us to be conservative -- ie., we need to detect where Self: Sized because it's an error if it is present

nikomatsakis (Sep 30 2019 at 20:40, on Zulip):

but in the other two cases, it's more than the presernce of where Self: Sized avoids errors that would otherwise occur, so it'd be ok to miss one

nikomatsakis (Sep 30 2019 at 20:41, on Zulip):

the reason we're getting a cycle error comes from having to normalize associated types

nikomatsakis (Sep 30 2019 at 20:41, on Zulip):

i.e., converting something like <Foo as Iterator>::Item into its equivalent type (if we can figure out a better one)

nikomatsakis (Sep 30 2019 at 20:41, on Zulip):

but that isn't really needed for the check I had in mind

nikomatsakis (Sep 30 2019 at 20:42, on Zulip):

that normalization is done as part of the elaborate_predicates step

nikomatsakis (Sep 30 2019 at 20:43, on Zulip):

I think what you want to do -- at least for now -- is to create a clone of that helper function generics_require_sized_self which does not do the elaborate_predicates call

nikomatsakis (Sep 30 2019 at 20:43, on Zulip):

so something ike

    fn generics_require_sized_self(self, def_id: DefId) -> bool {
        let sized_def_id = match self.lang_items().sized_trait() {
            Some(def_id) => def_id,
            None => { return false; /* No Sized trait, can't require it! */ }
        };

        // Search for a predicate like `Self : Sized` amongst the trait bounds.
        let predicates = self.predicates_of(def_id);
        let predicates = predicates.instantiate_identity(self).predicates;
        predicates.iter() // not elaborate_predicates(self, predicates)
            .any(|predicate|  /* as before */      )
    }
nikomatsakis (Sep 30 2019 at 20:44, on Zulip):

this could miss some cases, e.g. if the user wrote something like impl<T: ?Sized + Foo> and you had trait Foo: Sized

nikomatsakis (Sep 30 2019 at 20:44, on Zulip):

but that's ok because that will make us more conservative in this case

nikomatsakis (Sep 30 2019 at 20:44, on Zulip):

we're really interested in that syntactic check of "did they disable the default or not"

blitzerr (Sep 30 2019 at 20:57, on Zulip):

but in the other two cases, it's more than the presernce of where Self: Sized avoids errors that would otherwise occur, so it'd be ok to miss one

Can you elaborate on this one ? *It's more than the presence *

blitzerr (Sep 30 2019 at 20:58, on Zulip):

the reason we're getting a cycle error comes from having to normalize associated types

i.e., converting something like <Foo as Iterator>::Item into its equivalent type (if we can figure out a better one)

So because we are introducing associative types that are cyclical in the weaponized-transmute ?

blitzerr (Sep 30 2019 at 20:59, on Zulip):

@nikomatsakis :point_of_information:

blitzerr (Sep 30 2019 at 21:00, on Zulip):

By the way, is weaponized-transmute a standard term in the PL literature ?

nikomatsakis (Sep 30 2019 at 21:01, on Zulip):

not at all

nikomatsakis (Sep 30 2019 at 21:01, on Zulip):

Can you elaborate on this one ? *It's more than the presence *

it should have been "it's more that the presence of where Self: Sized avoids errors"

nikomatsakis (Sep 30 2019 at 21:03, on Zulip):

i.e., if there is a where Self: Sized, then there is no error; so if we have a check that looks for where Self: Sized, but sometimes it misses some, that might cause extra errors -- but it won't cause us to accept code we should not have accepted

nikomatsakis (Sep 30 2019 at 21:03, on Zulip):

By the way, is weaponized-transmute a standard term in the PL literature ?

we sometimes say "weaponized" to mean "we showed how you could use this bug to violate memory safety"

nikomatsakis (Sep 30 2019 at 21:05, on Zulip):

So because we are introducing associative types that are cyclical in the weaponized-transmute ?

the new check we are adding to the object_safety.rs code is now causing us to do normalizations we weren't doing before; but doing those normalizations requires us to check object safety (that itself is perhaps also fixable, now that I think about it... but in any case I'd still like to push forward on the simpler version of this branch mostly so that we can tell if this approach is even viable)

blitzerr (Oct 01 2019 at 03:02, on Zulip):

Can you elaborate on this one ? *It's more than the presence *

it should have been "it's more that the presence of where Self: Sized avoids errors"

Got it. Now it makes sense. :slight_smile:

blitzerr (Oct 01 2019 at 03:50, on Zulip):

@nikomatsakis The state of the world does not change by much :slight_smile:

error[E0391]: cycle detected when determine object safety of trait `std::ops::Deref`
  --> /Users/blitz/rustc-dev/rust/src/libcore/ops/deref.rs:64:1
   |
64 | pub trait Deref {
   | ^^^^^^^^^^^^^^^
   |
   = note: ...which requires normalizing `ParamEnvAnd { param_env: ParamEnv { caller_bounds: [Binder(TraitPredicate(<Self as std::ops::Deref>))], reveal: All, def_id: None }, value: &dyn std::ops::Deref<Target = <Self as std::ops::Deref>::Target> }`...
   = note: ...which requires normalizing `Canonical { max_universe: U0, variables: [], value: ParamEnvAnd { param_env: ParamEnv { caller_bounds: [Binder(TraitPredicate(<Self as std::ops::Deref>))], reveal: All, def_id: None }, value: ProjectionTy { substs: [Self], item_def_id: DefId(2:1704 ~ core[d402]::ops[0]::deref[0]::Deref[0]::Target[0]) } } }`...
   = note: ...which again requires determine object safety of trait `std::ops::Deref`, completing the cycle
   = note: cycle used when evaluating trait selection obligation `<T as std::ops::Deref>::Target == U`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0391`.
blitzerr (Oct 01 2019 at 03:51, on Zulip):

Maybe we need to get rid of let predicates = predicates.instantiate_identity(self).predicates; ?

blitzerr (Oct 01 2019 at 04:06, on Zulip):
let predicates = &self.predicates_of(def_id).predicates;
        // let predicates = predicates.instantiate_identity(self).predicates;
        predicates
            .iter()
blitzerr (Oct 01 2019 at 04:08, on Zulip):

I went a step futher, and removed the call to the instantiate_identity and yet we still get the same error

error[E0391]: cycle detected when determine object safety of trait `std::ops::Deref`
  --> /Users/blitz/rustc-dev/rust/src/libcore/ops/deref.rs:64:1
   |
64 | pub trait Deref {
   | ^^^^^^^^^^^^^^^
   |
   = note: ...which requires normalizing `ParamEnvAnd { param_env: ParamEnv { caller_bounds: [Binder(TraitPredicate(<Self as std::ops::Deref>))], reveal: All, def_id: None }, value: &dyn std::ops::Deref<Target = <Self as std::ops::Deref>::Target> }`...
   = note: ...which requires normalizing `Canonical { max_universe: U0, variables: [], value: ParamEnvAnd { param_env: ParamEnv { caller_bounds: [Binder(TraitPredicate(<Self as std::ops::Deref>))], reveal: All, def_id: None }, value: ProjectionTy { substs: [Self], item_def_id: DefId(2:1704 ~ core[d402]::ops[0]::deref[0]::Deref[0]::Target[0]) } } }`...
   = note: ...which again requires determine object safety of trait `std::ops::Deref`, completing the cycle
   = note: cycle used when evaluating trait selection obligation `<T as std::ops::Deref>::Target == U`
blitzerr (Oct 01 2019 at 04:09, on Zulip):

My branch has the latest changes. I will look again tomorrow.

blitzerr (Oct 01 2019 at 16:31, on Zulip):

@nikomatsakis Maybe there is no straight forward way to avoid the cycle and we have to keep track of elements so that we don't ?

nikomatsakis (Oct 01 2019 at 20:12, on Zulip):

I'll have to check out your branch @blitzerr

nikomatsakis (Oct 01 2019 at 21:46, on Zulip):

ok, I think maybe I am starting to see the problem actually

nikomatsakis (Oct 01 2019 at 21:47, on Zulip):

well, not quite yet :)

nikomatsakis (Oct 01 2019 at 21:47, on Zulip):

(I am reproducing it, though)

nikomatsakis (Oct 03 2019 at 13:21, on Zulip):

@blitzerr ok I understand the problem now

nikomatsakis (Oct 03 2019 at 13:21, on Zulip):

it's not really related to the code you wrote, it's actually related to the code I wrote

nikomatsakis (Oct 03 2019 at 13:21, on Zulip):

I think we want to separate the "object safety" test from the "degeneracy" test

blitzerr (Oct 03 2019 at 13:22, on Zulip):

:clap: awesome

nikomatsakis (Oct 03 2019 at 13:22, on Zulip):

right now we have this code

pub(super) fn object_safety_provider(tcx: TyCtxt<'_>, trait_def_id: DefId) -> ObjectSafety {
    if !tcx.object_safety_violations(trait_def_id).is_empty() {
        return ObjectSafety::Not;
    }

    if tcx.impl_potentially_overlapping_dyn_trait(tcx, trait_def_id) {
        return ObjectSafety::Degenerate;
    }

    ObjectSafety::Full
}
nikomatsakis (Oct 03 2019 at 13:23, on Zulip):

the thing that's causing the cycles though is this addition I made to the select.rs code:

nikomatsakis (Oct 03 2019 at 13:23, on Zulip):
        // First check if the trait is object-safe -- if not fully object safe,
        // then we would never supply an impl in the first case.
        match self.tcx().object_safety(obligation.predicate.def_id()) {
            ObjectSafety::Full => { }
            ObjectSafety::Degenerate | ObjectSafety::Not => return,
        }
nikomatsakis (Oct 03 2019 at 13:23, on Zulip):

you'll see that if you remove the call to impl_potentially_overlapping_dyn_trait altogether you still get a cycle

nikomatsakis (Oct 03 2019 at 13:24, on Zulip):

I think what we want to do is add a separate query -- something like dyn_implements_self or degenerate_object_safety -- that just do the impl_potentially_overlapping_dyn_trait check for a given trait

nikomatsakis (Oct 03 2019 at 13:24, on Zulip):

and that is what we check in the select.rs code

nikomatsakis (Oct 03 2019 at 13:25, on Zulip):

I'm not sure yet what I think this means in the bigger picture, but it's a step in the right direction

blitzerr (Oct 03 2019 at 13:28, on Zulip):

So objecy_safety just checks Fullor Not and we have another query for the Degenerate ?

blitzerr (Oct 03 2019 at 13:29, on Zulip):

Who calls the degenerate check ?

blitzerr (Oct 03 2019 at 13:29, on Zulip):

@nikomatsakis

nikomatsakis (Oct 03 2019 at 13:29, on Zulip):

So objecy_safety just checks Fullor Not and we have another query for the Degenerate ?

yeah I think my early commits are kind of wrong-ish now

nikomatsakis (Oct 03 2019 at 13:30, on Zulip):

I refactored to introduce that enum I thnk

nikomatsakis (Oct 03 2019 at 13:30, on Zulip):

but if we're splitting up the check it's probably not what we want

nikomatsakis (Oct 03 2019 at 13:30, on Zulip):

Who calls the degenerate check ?

the select.rs quote I added, which I copied above

nikomatsakis (Oct 03 2019 at 13:30, on Zulip):

there might however be another way to handle this

nikomatsakis (Oct 03 2019 at 13:30, on Zulip):

I'm debating :)

nikomatsakis (Oct 03 2019 at 13:43, on Zulip):

well, we can try more than one thing

nikomatsakis (Oct 09 2019 at 21:25, on Zulip):

@blitzerr I ugess I kind of left you hanging here, huh?

blitzerr (Oct 09 2019 at 21:28, on Zulip):

I am still looking at this. To be very honest, this weekend I hardly had any time.

blitzerr (Oct 09 2019 at 21:28, on Zulip):

@nikomatsakis

nikomatsakis (Oct 23 2019 at 14:15, on Zulip):

Hey @blitzerr so I did some investigation here and I'm happy to report some progress

nikomatsakis (Oct 23 2019 at 14:16, on Zulip):

though I seem to be having an issue pushing to your branch for some reason

nikomatsakis (Oct 23 2019 at 14:16, on Zulip):

oh, I guess permission denied

nikomatsakis (Oct 23 2019 at 14:16, on Zulip):

well, on my fork, the branch nikomatsakis/degenerate-object-safe-issue-57893 contains a fix for the cycle query

nikomatsakis (Oct 23 2019 at 14:16, on Zulip):

I also fixed another minor bug

nikomatsakis (Oct 23 2019 at 14:16, on Zulip):

the good news is that the unsoundness seems to be corrected,and that we are also able to bootstrap

nikomatsakis (Oct 23 2019 at 14:16, on Zulip):

I've not yet dared to run test suite, will start that now

nikomatsakis (Oct 23 2019 at 14:25, on Zulip):

lol

nikomatsakis (Oct 23 2019 at 14:25, on Zulip):

I thnk i broke something

nikomatsakis (Oct 23 2019 at 14:25, on Zulip):
running 9016 tests
FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF 100/9016
FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF. 200/9016
FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF 300/9016
FFFFFF...FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF..FFFF.FFFF 400/9016
FFFFFFFF.FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFiFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF 500/9016
FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF 600/9016
FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF 700/9016
blitzerr (Oct 23 2019 at 15:00, on Zulip):

Not sure why you are getting permission denied. @nikomatsakis

blitzerr (Oct 23 2019 at 15:01, on Zulip):

That looks like a lot to fix :grinning:

nikomatsakis (Oct 23 2019 at 15:03, on Zulip):

(I don't think that's related to the PR, I think something went wrong in the build)

nikomatsakis (Oct 23 2019 at 15:04, on Zulip):

hmm, although a make clean yields the same problem

nikomatsakis (Oct 23 2019 at 15:04, on Zulip):

the errors are weird though

nikomatsakis (Oct 23 2019 at 15:04, on Zulip):

oh I bet I know what it's related to

nikomatsakis (Oct 23 2019 at 15:05, on Zulip):

I think the problem is that the query is not defined on traits from other crates

blitzerr (Oct 26 2019 at 15:10, on Zulip):

I missed this message. I am sorry

blitzerr (Oct 26 2019 at 15:12, on Zulip):

Looking at your updates on the GitHub issue @nikomatsakis , it looks like you have figured out bigger atuufs that might need to change here. Should we carve out some time to scope and then solve each of the issues you are talking about ?

blitzerr (Oct 29 2019 at 22:28, on Zulip):

@nikomatsakis ^

Last update: Nov 12 2019 at 15:30UTC