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

correctly deal with user type ascriptions in pat #96515

Merged
merged 4 commits into from
May 22, 2022

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Apr 28, 2022

supersedes #93856

thir::PatKind::AscribeUserType previously resulted in CanonicalUserTypeAnnotations where the inferred type already had a subtyping relation according to variance to the user_ty.

The bug can pretty much be summarized as follows:

  • during mir building
    • user_ty -> inferred_ty: considers variance
    • StatementKind::AscribeUserType: inferred_ty is the type of the place, so no variance needed
  • during mir borrowck
    • user_ty -> inferred_ty: does not consider variance
    • StatementKind::AscribeUserType: applies variance

This mostly worked fine. The lifetimes in inferred_ty were only bound by its relation to user_ty and to the place of StatementKind::AscribeUserType, so it doesn't matter where exactly the subtyping happens.

It does however matter when having higher ranked subtying. At this point the place where the subtyping happens is forced, causing this mismatch between building and borrowck to result in unintended errors.

cc #96514 which is pretty much the same issue

r? @nikomatsakis

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 28, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2022
@rust-log-analyzer

This comment has been minimized.

@inquisitivecrystal inquisitivecrystal added the A-patterns Relating to patterns and pattern matching label Apr 29, 2022
@nikomatsakis
Copy link
Contributor

I don't really understand the PR description, @lcnr :( maybe we can find some time to sync up on this. I'd like to understand better what is going on!

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me if you add back in the "inferred type" in the pretty printer and have an open PR against rustc-dev-guide that at least starts to explain wtf is going on here :)

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 20, 2022

📌 Commit 9b5a0f55a7d08ec900a7ca3e08de1e9ef4b634fa has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 20, 2022
@bors
Copy link
Contributor

bors commented May 21, 2022

⌛ Testing commit 9b5a0f55a7d08ec900a7ca3e08de1e9ef4b634fa with merge 35e0e69c59029e2659cc6b35c3d1006856e44995...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 21, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 21, 2022
@lcnr
Copy link
Contributor Author

lcnr commented May 21, 2022

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented May 21, 2022

📌 Commit 7637008 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2022
@bors
Copy link
Contributor

bors commented May 21, 2022

⌛ Testing commit 7637008 with merge e52e711...

@bors
Copy link
Contributor

bors commented May 22, 2022

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing e52e711 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 22, 2022
@bors bors merged commit e52e711 into rust-lang:master May 22, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 22, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e52e711): comparison url.

Instruction count

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

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 0 2 0 0 0
mean2 N/A 2.2% N/A N/A N/A
max N/A 2.7% N/A N/A N/A

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 1 0 1 2 2
mean2 1.8% N/A -1.2% -2.7% 0.3%
max 1.8% N/A -1.2% -3.7% 1.8%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes 2

  2. the arithmetic mean of the percent change 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-patterns Relating to patterns and pattern matching merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants