lcnr / solver-woes

Documenting changes due to the the new trait solver and my worries about stabilization
4 stars 0 forks source link

alias bound is unsound #9

Closed lcnr closed 1 year ago

lcnr commented 1 year ago

if we have type Assoc: OtherTrait.

1) given <T as Trait>::Assoc, we can assume OtherTrait 2) for this, we have to prove <T as Trait>::Assoc: OtherTrait inside of each impl

It feels like this should be unsound by using the assumption while proving that it holds, but may require more coinduction or something, idk.

There are sound alternatives to AliasBound candidates but they may slightly change inference or result in a performance regression.

lcnr commented 1 year ago

given the following changes to rustc:

diff --git a/compiler/rustc_trait_selection/src/solve/assembly.rs b/compiler/rustc_trait_selection/src/solve/assembly.rs
index dec9f8016b0..0f633ab5cb6 100644
--- a/compiler/rustc_trait_selection/src/solve/assembly.rs
+++ b/compiler/rustc_trait_selection/src/solve/assembly.rs
@@ -484,6 +484,8 @@ pub(super) fn merge_candidates_and_discard_reservation_impls(
             _ => {}
         }

+        return self.try_merge_responses(candidates.iter().map(|c| Ok(c.result)));
+
         if candidates.len() > 1 {
             let mut i = 0;
             'outer: while i < candidates.len() {
diff --git a/compiler/rustc_trait_selection/src/traits/project.rs b/compiler/rustc_trait_selection/src/traits/project.rs
index f7559a3f10a..12eb6f7d1e7 100644
--- a/compiler/rustc_trait_selection/src/traits/project.rs
+++ b/compiler/rustc_trait_selection/src/traits/project.rs
@@ -463,7 +463,7 @@ fn fold_binder<T: TypeFoldable<TyCtxt<'tcx>>>(
     }

     fn fold_ty(&mut self, ty: Ty<'tcx>) -> Ty<'tcx> {
-        if !needs_normalization(&ty, self.param_env.reveal()) {
+        if !needs_normalization(&ty, self.param_env.reveal()) || self.interner().trait_solver_next() {
             return ty;
         }

these end up removing eager normalization, and choose always applicable candidates in case there are multiple. Both changes we consider to be desirable.

This ends up causing the following example to segfault with -Ztrait-solver=next:

use std::rc::Rc;
trait Trait {
    type Assoc: Send;
}

impl<T> Trait for T
where
    <T as Trait>::Assoc: Send,
{
    type Assoc = Rc<String>;
}

fn send<T: Clone>(x: &T) {
    let _ = x.clone();
}

fn foo(x: Rc<String>) {
    std::thread::scope(|s| {
        loop {
            println!("strong count: {}", Rc::strong_count(&x));
            for _ in 0..100 {
                s.spawn(|| 
                    // oh no, `Rc<String>` is not `Send`
                    send::<<() as Trait>::Assoc>(&x)
                );
            }
        }
    })
}

fn main() {
    foo(Rc::new(String::from("hey, this is a string")));
}
compiler-errors commented 1 year ago

Another alias bound unsoundness segfault in the new solver, with #![feature(trivial_bounds)], but it shouldn't be too hard to make this into not a trivial bound with some lifetime arg:

// compile-flags: -Ztrait-solver=next
#![feature(trivial_bounds)]

trait Foo {
    type Item: Copy where <Self as Foo>::Item: Copy;

    fn copy_me(x: &Self::Item) -> Self::Item {
        *x
    }
}

impl Foo for () {
    type Item = String where String: Copy;
}

fn main() {
    let x = String::from("hello, world");
    drop(<() as Foo>::copy_me(&x));
    println!("{x}");
}
lcnr commented 1 year ago

moved to https://github.com/rust-lang/trait-system-refactor-initiative/issues/6