-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Coherence can be bypassed by an indirect impl for a trait object #57893
Comments
@arielb1 Interesting. I guess we would need a deeper check, something along the lines of: impl<T> Object for SomeType<T> where WC {
/* ... */
}
|
Sure. What makes this non-trivial is auto-traits ( impl<T> Object<'a> for SomeType<T> where WC Then the "ideal" lowering would be something of this format:
If you ignore binders in unification (i.e., you consider
While I don't think this is insurmountable, it is fairly annoying and places some restrictions on how you encode auto-traits. |
I'm interested in working on this. |
It turns out that only one trait is necessary to reproduce this: (playground) trait Object {
type Output;
}
impl<T: ?Sized> Object for T {
type Output = &'static u64;
}
fn foo<'a, T: ?Sized>(x: <T as Object>::Output) -> &'a u64 {
x
}
fn transmute_lifetime<'a, 'b>(x: &'a u64) -> &'b u64 {
foo::<dyn Object<Output=&'a u64>>(x)
}
// And yes this is a genuine `transmute_lifetime`!
fn get_dangling<'a>() -> &'a u64 {
let x = 0;
transmute_lifetime(&x)
}
fn main() {
let r = get_dangling();
println!("{}", r);
} This seems quite bad, as simply writing a blanket impl is enough to expose the issue. |
One way to fix this issue would be the following: If a trait has any associated items, and a blanket impl exists for it, that trait cannot be object-safe. This is pretty extreme - however, the only other solution I can think of would be to deny all blanket impls of a trait with associated items - which seems even worse, given that the trait might not have even been object-safe to begin with. |
Unfortunately, it gets worse. Here's an implementation of 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)
} I think blame should be assigned to not normalizing I checked the code above with godbolt and the problem has existed since Rust 1.0.0. |
The first comments in the thread seem to indicate that the problem is instead with the blanket impl itself being accepted by the compiler. If I understood correctly, @Centril's impl above can be applied to |
Seems like a good idea to discuss in language and compiler team, nominating. |
Notably, in my reproducer above, if you change fn foo<T: ?Sized + Object<U>, U>(x: <T as Object<U>>::Output) -> U {
x
} then you will get:
|
Is that not just due to where-clauses having precedence over impls? AFAIK trait selection will prefer the |
I made a first attempt at a fix here: https://github.com/Aaron1011/rust/tree/fix/trait-obj-coherence My idea was to extend the obligations generated by
This is combeind with an extra flag in Unfortunately, I wasn't able to get this working, due to how the well-formed predicate generation is integrated with the rest of the compiler. I would need to significantly refactor I'm hoping that someone can come up with a better approach. However, any solution will need to deal with the fact that |
This is interesting! The fact that I wouldn't be sad if we would fix this by being less aggressive about exploiting coherence during type-checking -- that makes analysis much more complicated, and this example shows why. But there's likely already tons of code out there that exploits this. Another fix is to refuse to create trait objects that "contradict" the kind of coherence knowledge that the type checker might exploit, but that seems extremely fragile. @Aaron1011 your plan seems to be to encode in the type of Once again, exploiting assumptions implicitly instead of inferring them to an explicit form proves to be an issue. This reminds me that we still implicitly exploit WF everywhere instead of turning that into explicit assumptions... |
triage: P-high due to unsoundness. Leaving nominated in hope of discussing today. Not assigning to anyone yet. |
assigning to @nikomatsakis with expectation that they will delegate. (and removing nomination label) |
Still nominated for t-lang. |
I am not sure that giving up on coherence is the right choice here. I think it is too easy to split the code into functions, such that no one function sees the coherence violation: struct Data<T: ?Sized, U> where T: Object<U> {
data: <T as Object<U>>::Output
}
trait Object<U> {
type Output;
}
trait Mark {
type Output;
}
impl<T: ?Sized, U: Mark<Output=V>, V> Object<U> for T {
type Output = V;
}
fn iso_1<T, U>(data: T) -> Data<dyn Object<U, Output=T>, U> {
// in any coherence-less solution, this shouldn't "see" the
// blanket impl, as it might not apply (e.g., `Local` might
// be in a third-party crate).
Data { data }
}
fn iso_2<X: ?Sized, U, V>(data: Data<X, U>) -> V
where U: Mark<Output=V>
{
// similarly, this shouldn't "see" the trait-object impl.
data.data
}
fn transmute_m<T, U, V>(data: T) -> V
where U: Mark<Output=V>
{
// This function *also* shouldn't see the blanket impl - `Local`
// might be in a third-party crate.
iso_2::<dyn Object<U, Output=T>, U, V>(
iso_1::<T, U>(data)
)
}
// These 3 functions could be in a child crate
struct Local<T>(T);
impl<T> Mark for Local<T> {
type Output = T;
}
fn transmute<T, U>(x: T) -> U {
// and this function shouldn't see *anything* that looks like a
// problematic associated type.
transmute_m::<_, Local<_>, _>(x)
} |
I think that conditioning object safety on an object type being coherent is a reasonable-enough way out of this. If we want to be tricky, we might put the condition "directly" on the impl - i.e., have |
@arielb1 your |
…-dyn, r=lcnr Don't resolve generic impls that may be shadowed by dyn built-in impls **NOTE:** This is a hack. This is not trying to be a general fix for the issue that we've allowed overlapping built-in trait object impls and user-written impls for quite a long time, and traits like `Any` rely on this (rust-lang#57893) -- this PR specifically aims to mitigate a new unsoundness that is uncovered by the MIR inliner (rust-lang#114928) that interacts with this pre-existing issue. Builtin `dyn Trait` impls may overlap with user-provided blanket impls (`impl<T: ?Sized> Trait for T`) in generic contexts. This leads to bugs when instances are resolved in polymorphic contexts, since we typically prefer object candidates over impl candidates. This PR implements a (hacky) heuristic to `resolve_associated_item` to account for that unfortunate hole in the type system -- we now bail with ambiguity if we try to resolve a non-rigid instance whose self type is not known to be sized. This makes sure we can still inline instances like `impl<T: Sized> Trait for T`, which can never overlap with `dyn Trait`'s built-in impl, but we avoid inlining an impl that may be shadowed by a `dyn Trait`. Fixes rust-lang#114928
Rollup merge of rust-lang#114941 - compiler-errors:inline-shadowed-by-dyn, r=lcnr Don't resolve generic impls that may be shadowed by dyn built-in impls **NOTE:** This is a hack. This is not trying to be a general fix for the issue that we've allowed overlapping built-in trait object impls and user-written impls for quite a long time, and traits like `Any` rely on this (rust-lang#57893) -- this PR specifically aims to mitigate a new unsoundness that is uncovered by the MIR inliner (rust-lang#114928) that interacts with this pre-existing issue. Builtin `dyn Trait` impls may overlap with user-provided blanket impls (`impl<T: ?Sized> Trait for T`) in generic contexts. This leads to bugs when instances are resolved in polymorphic contexts, since we typically prefer object candidates over impl candidates. This PR implements a (hacky) heuristic to `resolve_associated_item` to account for that unfortunate hole in the type system -- we now bail with ambiguity if we try to resolve a non-rigid instance whose self type is not known to be sized. This makes sure we can still inline instances like `impl<T: Sized> Trait for T`, which can never overlap with `dyn Trait`'s built-in impl, but we avoid inlining an impl that may be shadowed by a `dyn Trait`. Fixes rust-lang#114928
Given these facts, might it be justifiable, as a soundness fix, to add a One wrinkle is that downstream impls of |
This solution is unnecessarily restrictive. The problem is not the dyn-safety of the trait, the problem is the conflict between the blanket and compiler-generated trait impls. Suppressing the latter is sufficient to resolve the incoherence. For example, the trait below could be fully object safe: trait Foo {
type Assoc;
}
impl<T: ?Sized> Foo for T {
type Assoc = i32;
} |
(HRTBs complicate the story somewhat. Given |
I tried that, and it's not really great. |
Hmm, looking at those regressions, perhaps edition-dependent name resolution offers us a way out. Potential plan:
I think that would fix the vast majority of the crater regressions. |
Document behavior of `<dyn Any as Any>::type_id()` See also rust-lang#57893 `@rustbot` label A-docs T-libs
Rollup merge of rust-lang#118028 - Jules-Bertholet:dyn-any-doc, r=thomcc Document behavior of `<dyn Any as Any>::type_id()` See also rust-lang#57893 `@rustbot` label A-docs T-libs
Another possible solution:
|
…pl, r=<try> Reject blanket object impls that are possibly incoherent wrt associated types I would like to make this test more sophisticated. Namely, we should plug the unconstrained associated types of the object type with placeholders, and then detect cases where the placeholders *don't* end up being what the blanket impl would have predicted. In that case, we know that we can use a `dyn Trait` to abuse the unsoundness in rust-lang#57893. However, first I'd like to see what the most naïve fallout of this is. r? `@ghost`
(already linked above via back-link) FYI the reproducer for this miscompile ICEs Miri because said reproducer currently compiles to invalid MIR: #127667 |
this pattern also affects the impl of trait ToString {
type Assoc;
}
trait Display {}
impl<T: ?Sized + Display> ToString for T {
type Assoc = usize;
}
trait Super: ToString + Display {}
fn via_display<T: ?Sized + Display>() -> <T as ToString>::Assoc {
0
}
fn dyn_trait() -> u32 {
*via_display::<dyn Super<Assoc = Box<u32>>>()
}
fn main() {
println!("{}", dyn_trait());
} however, we only get unsoundness if the affected trait has an associated type. |
…eteness, r=<try> Disqualify built-in trait impl if it seems likely to overlap in an unsound way with a blanket impl cc rust-lang#57893 r? lcnr
…eteness, r=<try> Disqualify built-in trait impl if it seems likely to overlap in an unsound way with a blanket impl cc rust-lang#57893 r? lcnr
rework winnowing to sensibly handle global where-bounds There may be multiple ways to prove a given trait-bound. In case there are multiple such applicable candidates we need to somehow merge them or bail with ambiguity. When merging candidates we prefer some over others for multiple reasons: - we want to guide inference during typeck, even if not strictly necessary - to avoid ambiguity if there if there are at most lifetime differences - old solver needs exactly one candidate - new solver only needs to handle lifetime differences - we disable normalization via impls if the goal is proven by using a where-bound ## The approach in this PR[^1] - always prefer trivial builtin impls[^6] - then prefer non-global[^global] where-bounds - if there exists exactly one where-bound, guide inference - if there are multiple where-bounds even if some of them are global, ambig - then prefer alias bounds[^2] and builtin trait object candidates[^3][^2] - merge everything ignoring global where-bounds - if there are no other candidates, try using global where-bounds[^5] **We disable normalization via impls when using non-global where-bounds or alias-bounds, even if we're unable to normalize by using the where-bound.** [^1]: see the source for more details [^2]: [we arbitrary select a single object and alias-bound candidate in case multiple apply and they don't impact inference](https://github.com/rust-lang/rust/blob/a4cedecc9ec76b46dcbb954750068c832cf2dd43/compiler/rustc_trait_selection/src/traits/select/mod.rs#L1906-L1911). This should be unnecessary in the new solver. [^3]: Necessary for `dyn Any` and rust-lang#57893 [^global]: a where-bound is global if it is not higher-ranked and doesn't contain any generic parameters, `'static` is ok [^5]: global where-bounds are only used if they are unsatisfiable, i.e. no impl candidate exists [^6]: they don't constrain inference and don't add any lifetime constraints ## Why this behavior? ### inference guidance via where-bounds and alias-bounds #### where-bounds ```rust fn method_selection<T: Into<u64>>(x: T) -> Option<u32> { x.into().try_into().ok() // prove `T: Into<?0>` and then select a method `?0`, // needs eager inference. } ``` While the above pattern exists in the wild, I think that most inference guidance due to where-bounds is actually unintended. I believe we may want to restrict inference guidance in the future, e.g. limit it to where-bounds whose self-type is a param. #### alias-bounds ```rust pub trait Dyn { type Word: Into<u64>; fn d_tag(&self) -> Self::Word; fn tag32(&self) -> Option<u32> { self.d_tag().into().try_into().ok() // prove `Self::Word: Into<?0>` and then select a method // on `?0`, needs eager inference. } } ``` ### Disable normalization via impls when using where-bounds cc rust-lang/trait-system-refactor-initiative#125 ```rust trait Trait<'a> { type Assoc; } impl<T> Trait<'static> for T { type Assoc = (); } // normalizing requires `'a == 'static`, the trait bound does not. fn foo<'a, T: Trait<'a>>(_: T::Assoc) {} ``` If an impl adds constraints not required by a where-bound, using the impl may cause compilation failure avoided by treating the associated type as rigid. This is also why we can always use trivial builtin impls, even for normalization. They are guaranteed to never add any requirements. ### Lower priority for global where-bounds A where-bound is considered global if it does not refer to any generic parameters and is not higher-ranked. It may refer to `'static`. This means global where-bounds are either always fully implied by an impl or unsatisfiable. We don't really care about the inference behavior of unsatisfiable where-bounds :3 If a where-bound is fully implied then using an applicable impl for normalization cannot result in additional constraints. As this is the - afaict only - reason why we disable normalization via impls in the first place, we don't have to disable normalization via impls when encountering global where-bounds. ### Consider global where-bounds at all Given that we just use impls even if there exists a global where-bounds, you may ask why we don't just ignore these global where-bounds entirely: we use them to weaken the inference guidance from non-global where-bounds. Without a global where-bound, we currently prefer non-global where bounds even though there would be an applicable impl as well. By adding a non-global where-bound, this *unnecessary* inference guidance is disabled, allowing the following to compile: ```rust fn check<Color>(color: Color) where Vec: Into<Color> + Into<f32>, { let _: f32 = Vec.into(); // Without the global `Vec: Into<f32>` bound we'd // eagerly use the non-global `Vec: Into<Color>` bound // here, causing this to fail. } struct Vec; impl From<Vec> for f32 { fn from(_: Vec) -> Self { loop {} } } ``` [There exist multiple crates which rely on this behavior](rust-lang#124592 (comment)). ## Design considerations We would like to be able to normalize via impls as much as possible. Disabling normalization simply because there exists a where-bound is undesirable. For the sake of backwards compatability I intend to mostly mirror the current inference guidance rules and then explore possible improvements once the new solver is done. I do believe that removing unnecessary inference guidance where possible is desirable however. Whether a where-bound is global depends on whether used lifetimes are `'static`. The where-bound `u32: Trait<'static>` is either entirely implied by an impl, meaning that it does not have to disable normalization via impls, **while `u32: Trait<'a>` needs to disable normalization via impls as the impl may only hold for `'static`**. Considering all where-bounds to be non-global once they contain any region is unfortunately a breaking change. ## How does this differ from stable The currently stable approach is order dependent: - it prefers impls over global where-bounds: impl > global - it prefers non-global where-bounds over impls: non-global > impl - it treats all where-bounds equally: global = non-global This means that whether we bail with ambiguity or simply use the non-global where bound depending on the *order of where-clauses* and *number of applicable impl candidates*. See the tests added in the first commit for more details. With this PR we now always bail with ambiguity. I've previously tried to always use the non-global candidate, causing unnecessary inference guidance and undesirable breakage. This already went through an FCP in rust-lang#124592. However, I consider the new approach to be preferable as it exclusively removes incompleteness. It also doesn't cause any crater breakage. ## How to support this in the new solver :o **This is separately implemented in rust-lang#133643 and not part of this FCP!** To implement the global vs non-global where-bound distinction, we have to either keep `'static` in the `param_env` when canonicalizing, or eagerly distinguish global from non-global where-bounds and provide that information to the canonical query. The old solver currently keeps `'static` only the `param_env`, replacing it with an inference variable in the `value`. https://github.com/rust-lang/rust/blob/a4cedecc9ec76b46dcbb954750068c832cf2dd43/compiler/rustc_infer/src/infer/canonical/canonicalizer.rs#L49-L64 I dislike that based on *vibes* and it may end up being a problem once we extend the environment inside of the solver as [we must not rely on `'static` in the `predicate` as it would get erased in MIR typeck](rust-lang/trait-system-refactor-initiative#30). An alternative would be to eagerly detect trivial where-bounds when constructing the `ParamEnv`. We can't entirely drop them [as explained above](https://hackmd.io/qoesqyzVTe2v9cOgFXd2SQ#Consider-true-global-where-bounds-at-all), so we'd instead replace them with a new clause kind `TraitImpliedByImpl` which gets entirely ignored except when checking whether we should eagerly guide inference via a where-bound. This approach can be extended to where-bounds which are currently not considered global to stop disabling normalization for them as well. Keeping `'static` in the `param_env` is the simpler solution here and we should be able to move to the second approach without any breakage. I therefore propose to keep `'static` in the environment for now. --- r? `@compiler-errors`
Comments
The check for manual
impl Object for Object
only makes sure there is no directimpl Object for dyn Object
- it does not consider such indirect impls. Therefore, you can write a blanketimpl<T: ?Sized> Object for T
that conflicts with the builtinimpl Object for dyn Object
.Reproducer
edit: minimal reproducer from #57893 (comment)
I had some difficulties with getting the standard "incoherence ICE" reproducer, because the object candidate supersedes the impl candidate in selection. So here's a "transmute_lifetime" reproducer.
Duplicates, see also
The text was updated successfully, but these errors were encountered: