remove obsolete givens from regionck
#107376
Merged
Conversation
This file contains hidden or 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
aliemjay
commented
Jan 27, 2023
rustbot
added
S-blocked
This comment has been minimized.
This comment has been minimized.
|
is this still blocked on #105982 now? |
|
Nope. |
rustbot
removed
the
S-blocked
rustbot
added
the
S-waiting-on-review
lcnr
reviewed
Feb 1, 2023
| // of *every* relationship that arises here, | ||
| // but presently we do not.) | ||
| if r_a.is_free_or_static() && r_b.is_free() { | ||
| self.region_relation.add(r_a, r_b) |
There was a problem hiding this comment.
can you still make this an exhaustive match on r_a?
Comment on lines
92
to
98
| match *r { | ||
| ty::ReVar(_) if self.resolve_to_universal => self | ||
| .infcx | ||
| .inner | ||
| .borrow_mut() | ||
| .unwrap_region_constraints() | ||
| .opportunistic_resolve_region(TypeFolder::tcx(self), r), |
There was a problem hiding this comment.
cna we always use opportunistic_resolve_region here?
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #108096) made this pull request unmergeable. Please resolve the merge conflicts. |
rustbot
added
S-blocked
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 8, 2023
always resolve to universal regions if possible `RegionConstraintCollector::opportunistic_resolve_var`, which is used in canonicalization and projection logic, doesn't resolve the region var to an equal universal region. So if we have equated `'static == '1 == '2`, it doesn't resolve `'1` or `'2` to `'static`. Now it does! Addresses review comment rust-lang#107376 (comment). r? `@lcnr`
bors
added
the
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (e84e5ff): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Warning ⚠: The following benchmark(s) failed to build:
cc @rust-lang/wg-compiler-performance 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.
CyclesResultsThis 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.
|
lcnr
reviewed
Mar 15, 2023
| @@ -52,6 +53,10 @@ impl<'a, 'tcx: 'a> InferCtxtExt<'a, 'tcx> for InferCtxt<'tcx> { | |||
| body_id: LocalDefId, | |||
| ty: Ty<'tcx>, | |||
| ) -> Vec<OutlivesBound<'tcx>> { | |||
| let ty = self.resolve_vars_if_possible(ty); | |||
| let ty = OpportunisticRegionResolver::new(self).fold_ty(ty); | |||
There was a problem hiding this comment.
is it still needed to fold twice here?
|
This PR broke compilation of |
|
@aliemjay can you look into that breakage and whether that's something we can quickly fix? otherwise we should probably revert this 🤔 |
|
I've prepared a draft revert just in case in #109183, and the pub trait MapAccess {
type Error;
fn next_key_seed(&mut self) -> Option<Self::Error>;
}
struct Access<'a> {
_marker: std::marker::PhantomData<&'a ()>,
}
impl<'a, 'b: 'a> MapAccess for Access<'a> {
type Error = ();
fn next_key_seed(&mut self) -> Option<Self::Error> {
unimplemented!()
}
}Feel free to close the revert PR if we can land the fix soon. It may be a simple fix since the update: the revert is now in the bors queue. |
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Mar 15, 2023
Revert rust-lang#107376 to fix potential `bincode` breakage and `rustc-perf` benchmark. rust-lang#107376 caused `rustc-perf`'s `webrender` benchmark to break, by regressing on the `bincode-1.3.3` crate. ~~This PR is a draft revert in case we can't land a fix soon enough, and we'd like to land the revert instead~~ (Though I myself think it'd be safer to do the revert, and run crater when relanding rust-lang#107376.) cc `@aliemjay`
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 16, 2023
Revert rust-lang#107376 to fix potential `bincode` breakage and `rustc-perf` benchmark. rust-lang#107376 caused `rustc-perf`'s `webrender` benchmark to break, by regressing on the `bincode-1.3.3` crate. ~~This PR is a draft revert in case we can't land a fix soon enough, and we'd like to land the revert instead~~ (Though I myself think it'd be safer to do the revert, and run crater when relanding rust-lang#107376.) cc `@aliemjay`
@rustbot label: +perf-regression-triaged |
rustbot
added
the
perf-regression-triaged
Noratrieb
added a commit
to Noratrieb/rust
that referenced
this pull request
Mar 28, 2023
remove obsolete `givens` from regionck Revives rust-lang#107376. The only change is the last commit (rust-lang@2a3177a) which should fix the regression. Fixes rust-lang#106567 r? `@lcnr`
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.
Fixes #106567
r? @lcnr (feel free to reassign)