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

interpret region vars as universals in implied bounds query #109388

Closed
wants to merge 1 commit into from

Conversation

aliemjay
Copy link
Member

Implied bounds query is different to other canonical trait queries in that we should treat all region variables as universals. This fixes #106569.

To illustrate the problem in #106569:

  • query: implied_outlives_bounds(Canonical<Equal<'^1, '^2>)
  • instantiate the query: implied_outlives_bounds(Equal<'?1, '?2>)
  • unify '?1 == '?2 during normalization.
  • query response: ['?1: '?2, '?2: ?1] (as expected)
  • when canonicalizing the query response we resolve unified region vars opportunistically. The query response is now: ['?1: '?1, '?1: '?1]

This PR mitigates this problem by mapping every existential region to a unique universal one before executing the query.

cc @lcnr
r? types

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 20, 2023
@aliemjay aliemjay added T-types Relevant to the types team, which will review and decide on the PR/issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2023
@lcnr
Copy link
Contributor

lcnr commented Mar 20, 2023

this is definitely complex and feels intuitively wrong to me1, will try to look at this at some point this week.

r? @lcnr

Footnotes

  1. which doesn't mean that it necessarily is

@rustbot rustbot assigned lcnr and unassigned oli-obk Mar 20, 2023
@aliemjay aliemjay marked this pull request as ready for review March 20, 2023 11:20
@aliemjay aliemjay added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2023
@aliemjay
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 20, 2023

@aliemjay: 🔑 Insufficient privileges: not in try users

@oli-obk
Copy link
Contributor

oli-obk commented Mar 20, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 20, 2023
@bors
Copy link
Contributor

bors commented Mar 20, 2023

⌛ Trying commit 4be4829 with merge 6a52a02ebbfeb6f8adfd1aa63e64a290f201d98c...

@bors
Copy link
Contributor

bors commented Mar 20, 2023

☀️ Try build successful - checks-actions
Build commit: 6a52a02ebbfeb6f8adfd1aa63e64a290f201d98c (6a52a02ebbfeb6f8adfd1aa63e64a290f201d98c)

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 20, 2023

☀️ Try build successful - checks-actions
Build commit: 6a52a02ebbfeb6f8adfd1aa63e64a290f201d98c (6a52a02ebbfeb6f8adfd1aa63e64a290f201d98c)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6a52a02ebbfeb6f8adfd1aa63e64a290f201d98c): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This 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.

mean range count
Regressions ❌
(primary)
1.6% [0.4%, 3.0%] 7
Regressions ❌
(secondary)
2.6% [0.9%, 4.3%] 57
Improvements ✅
(primary)
-1.1% [-1.6%, -0.7%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.0% [-1.6%, 3.0%] 9

Cycles

Results

This 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.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.4%, 0.4%] 1

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 20, 2023
@lcnr
Copy link
Contributor

lcnr commented Mar 31, 2023

we talked about this in sync at some point 😅 this is missing an explanatory comment, apart from that r=me

@lcnr lcnr removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2023
@lcnr lcnr added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-implied-bounds Area: Implied bounds / inferred outlives-bounds labels Mar 31, 2023
@aliemjay
Copy link
Member Author

aliemjay commented May 6, 2023

blocked on #109763. I touches the same and and I want to prioritize #109763.

@aliemjay aliemjay added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 6, 2023
@Dylan-DPC Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Feb 8, 2024
@Dylan-DPC
Copy link
Member

@aliemjay what's the status of this? this was blocked on a pr that's now closed but it was close to being approved

@Dylan-DPC
Copy link
Member

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

@Dylan-DPC Dylan-DPC closed this Mar 23, 2024
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-implied-bounds Area: Implied bounds / inferred outlives-bounds S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implied bounds: lifetime equality lost after normalization
7 participants