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

Return all opaques in typeck results #111705

Closed

Conversation

compiler-errors
Copy link
Member

Return all (erased) opaque keys in writeback results, not just one type per opaque def id. This will be useful for guiding opaque type inference during borrowck with the new trait solver.

This also fixes a subtle corner case with opaque constraints and dead code, though it's not really a super compelling use-case, lol. Just a side effect of the more important reasoning for the change above.

r? @oli-obk

@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 May 18, 2023
@compiler-errors
Copy link
Member Author

@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 May 18, 2023
@bors
Copy link
Contributor

bors commented May 18, 2023

⌛ Trying commit d3dae85 with merge ff144bedfc2b16d3cfc9b750cf881653a49de873...

@bors
Copy link
Contributor

bors commented May 18, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ff144bedfc2b16d3cfc9b750cf881653a49de873): comparison URL.

Overall result: ❌ regressions - 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 is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

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)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-4.0% [-4.0%, -4.0%] 1
Improvements ✅
(secondary)
-1.2% [-1.2%, -1.2%] 1
All ❌✅ (primary) -4.0% [-4.0%, -4.0%] 1

Cycles

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

Binary size

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

Bootstrap: 645.339s -> 642.785s (-0.40%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 18, 2023
Comment on lines +799 to +800
// For backwards compatibility, we choose the first matching
// opaque type definition
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we check all of them?

Copy link
Member Author

@compiler-errors compiler-errors May 22, 2023

Choose a reason for hiding this comment

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

It's not that we have to check them, it's that we have to merge them. I guess I could do that. (nvm we don't have to merge them, they've all been passed thru remap_generic_params_to_declaration_params already)

Copy link
Contributor

Choose a reason for hiding this comment

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

r=me with that

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait wait wait... We've remapped all of the hidden types back to their declaration params. We don't need to put the substs in the writeback results at all. 🤦

@compiler-errors
Copy link
Member Author

Gonna rework this PR. Seems like it's also not exactly necessary for my opaques PR to land.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 23, 2023
…i-obk

Check opaques for mismatch during writeback

Revive rust-lang#111705.

I realized that we don't need to put any substs in the writeback results since all of the hidden types have already been remapped. See the comment in `compiler/rustc_middle/src/ty/typeck_results.rs`, which should make that clear for other explorers of the codebase.

Additionally, we need to do some diagnostic stashing because the diagnostics we produce during HIR typeck is very poor and we should prefer the diagnostic that comes from MIR, if we have one.

r? `@oli-obk`
@compiler-errors compiler-errors deleted the all-opaques branch August 11, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants