Skip to content
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

normalization does not use impls covered by param env candidates #12

Open
lcnr opened this issue Apr 24, 2023 · 5 comments
Open

normalization does not use impls covered by param env candidates #12

lcnr opened this issue Apr 24, 2023 · 5 comments
Labels
A-normalization not-blocking-coherence An issue we can resolve after stabilizing the new solver during coherence S-increased-expressiveness

Comments

@lcnr
Copy link
Contributor

lcnr commented Apr 24, 2023

the old solver does not consider impl candidates for normalization if there is also a candidate from the environment:

trait Trait {
    type Assoc;
}

impl<T> Trait for T {
    type Assoc = T;
}

fn foo<T: Trait>(x: T) -> <T as Trait>::Assoc {
    x
}

godbolt

This will presumably break if we instead normalize projections for environment trait bounds to "rigid projections" because there will now be ambiguity again. This is concerning as "rigid projections" are considered as one of the longterm solutions for #1.

@lcnr
Copy link
Contributor Author

lcnr commented Apr 24, 2023

see https://rust-lang.zulipchat.com/#narrow/stream/362009-t-types.2Fetc.2Flazy-norm-strategems/topic/.60Projection.60.20assumption.20vs.20requirement/near/350538718 for some discussion about using rigid projections to solve #1.

I remember there being some discussion on zulip whether this behavior is desirable but I can't find that discussion right now.

I am also worried that this is a breaking change:

#![allow(warnings)]
trait Trait<U> {
    type Assoc;
}

impl<T> Trait<u64> for T {
    type Assoc = T;
}

fn lazy_init<T: Trait<U>, U>() -> (T, <T as Trait<U>>::Assoc) {
    todo!()
}

fn foo<T: Trait<u32, Assoc = T>>(x: T) {
    // `U` can be both `u32` and `u64` here.
    let (delayed, mut proj) = lazy_init::<_, _>();
    proj = x;
    let _: T = delayed;
}

This snippet is currently incomplete and I think only works with -Ztrait-solver=next because the new solver still relies on the old project code.

@lcnr
Copy link
Contributor Author

lcnr commented May 16, 2023

this impacts inference (and causes issues for eager norm)

pub trait Trait {
    type Assoc;
}

pub trait ImplementedDownstream {}

impl<T: ImplementedDownstream> Trait for Vec<T> {
    type Assoc = T;
}

pub fn foo<T>()
where
    Vec<T>: Trait
{
   let _: <Vec<_> as Trait>::Assoc = todo!();
} 

@aliemjay
Copy link
Member

From #31 (comment):

This means that we're using a different candidate when solving T: Trait than when normalizing <T as Trait>::Ty. I don't think it is unsound but it can lead to some surprising behavior like this requiring 'a == 'static despite using the impl candidate for normalization:

trait Trait<'a> {
    type Ty;
}

impl<T> Trait<'_> for T {
    type Ty = ();
}

fn test<'a, T: Trait<'static>>() {
    let _: <T as Trait<'a>>::Ty = ();
    //~^ ERROR lifetime may not live long enough
}

@lcnr lcnr added the not-blocking-coherence An issue we can resolve after stabilizing the new solver during coherence label Sep 12, 2023
@lcnr
Copy link
Contributor Author

lcnr commented Nov 13, 2023

this results in a fun breaking change: #76

@lcnr
Copy link
Contributor Author

lcnr commented Dec 20, 2023

another breaking change because of this:

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) {}

@lcnr lcnr changed the title normalization uses impls covered by param env candidates normalization does not use impls covered by param env candidates Feb 8, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 9, 2024
hide impls if trait bound is proven from env

AVERT YOUR EYES `@compiler-errors`

fixes rust-lang/trait-system-refactor-initiative#76 and rust-lang/trait-system-refactor-initiative#12 (comment)

this is kinda ugly and I hate it, but I wasn't able to think of a cleaner approach for now. I am also unsure whether we have to refine this filtering later on, so by making the change pretty minimal it should be easier to improve going forward.

r? `@BoxyUwU`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 9, 2024
Rollup merge of rust-lang#120836 - lcnr:param-env-hide-impl, r=BoxyUwU

hide impls if trait bound is proven from env

AVERT YOUR EYES `@compiler-errors`

fixes rust-lang/trait-system-refactor-initiative#76 and rust-lang/trait-system-refactor-initiative#12 (comment)

this is kinda ugly and I hate it, but I wasn't able to think of a cleaner approach for now. I am also unsure whether we have to refine this filtering later on, so by making the change pretty minimal it should be easier to improve going forward.

r? `@BoxyUwU`
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Feb 10, 2024
hide impls if trait bound is proven from env

AVERT YOUR EYES `@compiler-errors`

fixes rust-lang/trait-system-refactor-initiative#76 and rust-lang/trait-system-refactor-initiative#12 (comment)

this is kinda ugly and I hate it, but I wasn't able to think of a cleaner approach for now. I am also unsure whether we have to refine this filtering later on, so by making the change pretty minimal it should be easier to improve going forward.

r? `@BoxyUwU`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-normalization not-blocking-coherence An issue we can resolve after stabilizing the new solver during coherence S-increased-expressiveness
Projects
None yet
Development

No branches or pull requests

2 participants