Do not erase late bound regions when selecting inherent associated types #118118
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
spastorino
force-pushed
the
do-not-erase-late-bound-regions-on-iat
branch
from
823a7ca
to
3dea5d5
Compare
compiler-errors
changed the title
|
Could not assign reviewer from: |
spastorino
force-pushed
the
do-not-erase-late-bound-regions-on-iat
branch
from
3dea5d5
to
697a756
Compare
This comment has been minimized.
This comment has been minimized.
spastorino
force-pushed
the
do-not-erase-late-bound-regions-on-iat
branch
from
697a756
to
2d0b397
Compare
|
Does it fix #112631 ? |
|
I believe it should fix that issue |
It "fixes" the problem but there's another error with this test case later in the compilation process. It's trying to hash a region variable and we get an ICE Backtrace
|
|
you probably have to resolve the region vars in the returned alias 🤔 cc @compiler-errors do we not have an existing folder for that apart from |
ah... we mostly already do so in the |
|
#118118 (comment) |
Yes, there's #112397 |
|
I've extracted the select function in 5cfc9f3. I'm not sure if I like how the code looks like to be honest. Maybe we should extract first this https://github.com/spastorino/rust/blob/5cfc9f34345f2d217562c685bad230d05880facb/compiler/rustc_hir_analysis/src/astconv/mod.rs#L1653-L1681 And leave the current extracted thing in the main function. @lcnr @compiler-errors thoughts? |
lcnr
reviewed
Nov 21, 2023
|
yeah, probably makes sense to refactor it further you could move https://github.com/spastorino/rust/blob/5cfc9f34345f2d217562c685bad230d05880facb/compiler/rustc_hir_analysis/src/astconv/mod.rs#L1686-L1718 back out of |
This is not possible because of |
spastorino
force-pushed
the
do-not-erase-late-bound-regions-on-iat
branch
from
5cfc9f3
to
0e29a47
Compare
|
I think the refactor done in 642b89e is better but would leave up to you @compiler-errors @lcnr Edit: force pushed because I've reordered |
spastorino
force-pushed
the
do-not-erase-late-bound-regions-on-iat
branch
from
0e29a47
to
642b89e
Compare
|
☔ The latest upstream changes (presumably #118143) made this pull request unmergeable. Please resolve the merge conflicts. |
spastorino
force-pushed
the
do-not-erase-late-bound-regions-on-iat
branch
3 times, most recently
from
f07f0e9
to
e5397d3
Compare
This comment has been minimized.
This comment has been minimized.
spastorino
force-pushed
the
do-not-erase-late-bound-regions-on-iat
branch
from
e5397d3
to
42fc2f9
Compare
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 25, 2023
… r=compiler-errors
Make PlaceholderReplacer shallow_resolver and recur when infer vars
This makes resolve type and const infer vars resolve.
Given:
```rust
#![feature(inherent_associated_types)]
#![allow(incomplete_features)]
struct Foo<T>(T);
impl<'a> Foo<fn(&'a ())> {
type Assoc = &'a ();
}
fn bar(_: for<'a> fn(Foo<fn(Foo<fn(&'static ())>::Assoc)>::Assoc)) {}
fn main() {}
```
We should normalize `for<'a> fn(Foo<fn(Foo<fn(&'static ())>::Assoc)>::Assoc)` to `for<'0> fn(&'1 ())` with `'1 == '0` and `'0 == 'static` constraints. We have to resolve `'1` to `'static` in the infcx associated to `PlaceholderReplacer`.
This is part of rust-lang#118118 but unrelated to that PR.
r? `@compiler-errors` `@lcnr`
fmease
added a commit
to fmease/rust
that referenced
this pull request
Nov 25, 2023
…infer, r=compiler-errors Move EagerResolution to rustc_infer::infer::resolve `EagerResolver` fits better in `rustc_infer::infer::resolver`. Started to disentagle rust-lang#118118 that has a lot of unrelated things. r? `@compiler-errors` `@lcnr`
fmease
added a commit
to fmease/rust
that referenced
this pull request
Nov 25, 2023
…=compiler-errors Relate Inherent Associated Types using eq We should call `eq` instead of `sup` as we're relating `Ty` directly and not `Binder<TraitRef>`. This is part of rust-lang#118118 but unrelated to that PR. r? `@compiler-errors` `@lcnr`
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 25, 2023
Rollup merge of rust-lang#118262 - spastorino:relate-iats-using-eq, r=compiler-errors Relate Inherent Associated Types using eq We should call `eq` instead of `sup` as we're relating `Ty` directly and not `Binder<TraitRef>`. This is part of rust-lang#118118 but unrelated to that PR. r? `@compiler-errors` `@lcnr`
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 25, 2023
Rollup merge of rust-lang#118259 - spastorino:move-eager-resolver-to-infer, r=compiler-errors Move EagerResolution to rustc_infer::infer::resolve `EagerResolver` fits better in `rustc_infer::infer::resolver`. Started to disentagle rust-lang#118118 that has a lot of unrelated things. r? `@compiler-errors` `@lcnr`
bors
added a commit
to rust-lang/miri
that referenced
this pull request
Nov 25, 2023
…er-errors
Make PlaceholderReplacer shallow_resolver and recur when infer vars
This makes resolve type and const infer vars resolve.
Given:
```rust
#![feature(inherent_associated_types)]
#![allow(incomplete_features)]
struct Foo<T>(T);
impl<'a> Foo<fn(&'a ())> {
type Assoc = &'a ();
}
fn bar(_: for<'a> fn(Foo<fn(Foo<fn(&'static ())>::Assoc)>::Assoc)) {}
fn main() {}
```
We should normalize `for<'a> fn(Foo<fn(Foo<fn(&'static ())>::Assoc)>::Assoc)` to `for<'0> fn(&'1 ())` with `'1 == '0` and `'0 == 'static` constraints. We have to resolve `'1` to `'static` in the infcx associated to `PlaceholderReplacer`.
This is part of rust-lang/rust#118118 but unrelated to that PR.
r? `@compiler-errors` `@lcnr`
spastorino
force-pushed
the
do-not-erase-late-bound-regions-on-iat
branch
from
42c7df9
to
6312382
Compare
|
I've just rebased and force pushed with the solution I think is more correct. There's also this alternative 42c7df9 @lcnr @compiler-errors let me know what do you prefer but I think this should be ready to be merged as is. |
spastorino
force-pushed
the
do-not-erase-late-bound-regions-on-iat
branch
from
6312382
to
440f46d
Compare
|
@compiler-errors very good suggestions, have applied all of them. |
|
you've had a bunch of review comments, so |
|
@lcnr no worries it's fine. @compiler-errors let me know if there's something else to do here or if this is just ready. |
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 28, 2023
…r=<try> Fix for TypeId exposes equality-by-subtyping vs normal-form-syntactic-equality unsoundness Fixes rust-lang#97156 This PR revives rust-lang#97427 idea, it sits on top of rust-lang#118118 because the idea uncovered some problems with IATs. r? `@lcnr` This is ICEing yet for `tests/ui/traits/new-solver/escaping-bound-vars-in-writeback-normalization.rs` using the new trait solver. After rust-lang#118118 and this ICE is fixed, we would need a rebase and a crater run. Opening as a WIP for now.
|
Finished benchmarking commit (b4c4664): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 676.072s -> 674.379s (-0.25%) |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the fix for #97156 we would want the following code:
to fail with ...
This PR fixes the ICE we are currently getting "was a subtype of Foo<Binder(fn(&ReStatic ()), [])> during selection but now it is not"
Also fixes #112631
r? @lcnr