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

weaker leak-check in new solver #34

Closed
aliemjay opened this issue Jun 23, 2023 · 4 comments
Closed

weaker leak-check in new solver #34

aliemjay opened this issue Jun 23, 2023 · 4 comments

Comments

@aliemjay
Copy link
Member

The new solver breaks the following:

trait Leak<'a> {}
impl Leak<'_>      for Box<u32> {}
impl Leak<'static> for Box<u16> {}

fn impl_trait<T: for<'a> Leak<'a>>() {}

fn main() {
    impl_trait::<Box<_>>();
    //~^ ERROR type annotations needed
}

This needs investigation. The old and new solvers share the same leak-check logic. They shouldn't behave differently.

@compiler-errors
Copy link
Member

This has to do with region uniquification on query input.

for<'a> Box<?0>: Leak<'a> gets instantiated with placeholders like Box<?0>: Leak<'!1_0> (placeholder in universe 1). But that gets canonicalized and uniquified to Box<?0>: Leak<'?0_0> (re vid in universe 0). The ambiguity occurs because we've totally erased all region information, we can't actually use the leak check to disqualify the Leak<'static> candidate at this point.

@aliemjay
Copy link
Member Author

Then I think the leak check should be done in merge_candidates. This way this issue is fixed and we would have an arguably more reasonable location to invoke the leak check than in compute_external_query_constraints.

At this point, we can change the the leak checker to construct the constraint graph directly from the canonical response in order to avoid the overhead of entering a new snapshot and instantiating the query response for each candidate.

@compiler-errors
Copy link
Member

compiler-errors commented Jun 24, 2023

I'm not sure if I understand how moving the leak check to merge_candidates would fix this issue. This has to do with the fact that we erase the universe region in canonicalization, and as long as we're canonicalizing nested obligations, we can always push this issue one layer deeper into the goal stack, e.g.

trait Leak2<'a> {}
impl Leak2<'_>      for Box<u32> {}
impl Leak2<'static> for Box<u16> {}

trait Leak1<'a> {}
impl<'a, T> Leak1<'a> for T where T: Leak2<'a> {} 

fn impl_trait<T: for<'a> Leak1<'a>>() {}

fn main() {
    impl_trait::<Box<_>>();
    //~^ ERROR type annotations needed
}

@lcnr lcnr changed the title incomplete leak-check weaker leak-check Jul 7, 2023
@compiler-errors compiler-errors changed the title weaker leak-check weaker leak-check in new solver Oct 9, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 10, 2024
remove the `coherence_leak_check` future compat lint

and adapt the old solver to not rely on universe errors for higher ranked goals to impact candidate selection. This matches the behavior of the new solver: rust-lang/trait-system-refactor-initiative#34

r? `@nikomatsakis`
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 1, 2024
instantiate higher ranked goals outside of candidate selection, remove the `coherence_leak_check` future compat lint

- rust-lang#59490
- rust-lang#56105

---

and adapt the old solver to not rely on universe errors for higher ranked goals to impact candidate selection. This matches the behavior of the new solver: rust-lang/trait-system-refactor-initiative#34. See https://hackmd.io/gstwC83ORaG7V29w54nMdA for more details about this change

r? `@nikomatsakis`
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 1, 2024
instantiate higher ranked goals outside of candidate selection, remove the `coherence_leak_check` future compat lint

- rust-lang#59490
- rust-lang#56105

---

and adapt the old solver to not rely on universe errors for higher ranked goals to impact candidate selection. This matches the behavior of the new solver: rust-lang/trait-system-refactor-initiative#34. See https://hackmd.io/gstwC83ORaG7V29w54nMdA for more details about this change

r? `@nikomatsakis`
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 2, 2024
instantiate higher ranked goals outside of candidate selection, remove the `coherence_leak_check` future compat lint

- rust-lang#59490
- rust-lang#56105

---

and adapt the old solver to not rely on universe errors for higher ranked goals to impact candidate selection. This matches the behavior of the new solver: rust-lang/trait-system-refactor-initiative#34. See https://hackmd.io/gstwC83ORaG7V29w54nMdA for more details about this change

r? `@nikomatsakis`
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 2, 2024
instantiate higher ranked goals outside of candidate selection, remove the `coherence_leak_check` future compat lint

- rust-lang#59490
- rust-lang#56105

---

and adapt the old solver to not rely on universe errors for higher ranked goals to impact candidate selection. This matches the behavior of the new solver: rust-lang/trait-system-refactor-initiative#34. See https://hackmd.io/gstwC83ORaG7V29w54nMdA for more details about this change

r? `@nikomatsakis`
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 5, 2024
instantiate higher ranked goals outside of candidate selection, remove the `coherence_leak_check` future compat lint

- rust-lang#59490
- rust-lang#56105

---

and adapt the old solver to not rely on universe errors for higher ranked goals to impact candidate selection. This matches the behavior of the new solver: rust-lang/trait-system-refactor-initiative#34. See https://hackmd.io/gstwC83ORaG7V29w54nMdA for more details about this change

r? `@nikomatsakis`
@lcnr
Copy link
Contributor

lcnr commented May 21, 2024

accepted as part of rust-lang/rust#119820

@lcnr lcnr closed this as completed May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants