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.

Sign up for GitHub

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

Jump to bottom

Stop handling opaque types in queries and leave it to typeck #107891

Closed
wants to merge 4 commits into from
Closed

Stop handling opaque types in queries and leave it to typeck #107891

wants to merge 4 commits into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 10, 2023

Some cleanups now that all the lazy TAIT work has settled down.

We do not need to look into queries anymore, as all the opaque types get "equated away" early enough so that the trait solvers don't see opaque types unless they should treat them opaquely.

The last commit additionally flips the default on infcx.at(...).eq and friends, requiring you to opt-in to registering hidden types. We can probably do more cleanups here and possibly manage to eliminate the InferCtxt::defining_use_anchor field entirely by doing all processing at the define_opaque_types sites.

r? @lcnr

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

Please see the contribution instructions for more information.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 10, 2023

@bors try @rust-timer queue

@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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 10, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

cc @lcnr, @compiler-errors

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 10, 2023
@bors
Copy link
Contributor

bors commented Feb 10, 2023

⌛ Trying commit 89fe6b48916136f46d0c57faffa3841324c391ab with merge 4ae96c1c1c34188e3906cc8962b1d55407f0ae87...

@compiler-errors
Copy link
Member

This may need a crater run... 🤔

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 10, 2023

yea, this is a test coverage incubator due to the opt-in-ness

@bors
Copy link
Contributor

bors commented Feb 10, 2023

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

@rust-timer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 10, 2023

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-107891 created and queued.
🤖 Automatically detected try build 4ae96c1c1c34188e3906cc8962b1d55407f0ae87
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 10, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-107891 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4ae96c1c1c34188e3906cc8962b1d55407f0ae87): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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.4% [0.3%, 0.5%] 5
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.4% [0.3%, 0.5%] 5

Max RSS (memory usage)

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

Cycles

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

@rustbot rustbot added the perf-regression Performance regression. label Feb 10, 2023
@compiler-errors
Copy link
Member

looks like noise

@craterbot
Copy link
Collaborator

🎉 Experiment pr-107891 is completed!
📊 21 regressed and 2 fixed (255065 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Feb 11, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 13, 2023

@rustbot ready

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Feb 13, 2023

☔ The latest upstream changes (presumably #107924) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Feb 21, 2023

☔ The latest upstream changes (presumably #108286) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member

erg_compiler looks like a legitimate regression?

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 21, 2023

yes, i fixed it in 834b566

@oli-obk oli-obk mentioned this pull request Feb 21, 2023
Merged
Noratrieb
Noratrieb reviewed Feb 21, 2023
View reviewed changes
compiler/rustc_borrowck/src/region_infer/opaque_types.rs
let infcx = self
.tcx
.infer_ctxt()
// HACK This bubble is required for this tests to pass:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// HACK This bubble is required for this tests to pass:
// HACK This ignore is required for this tests to pass:

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2023
@bors
Auto merge of rust-lang#108311 - oli-obk:invert_defines, r=lcnr
f4c7596 
Make hidden type registration opt-in, so that each site can be reviewed on its own and we have the right defaults for trait solvers

r? `@lcnr`

pulled out of rust-lang#107891 as it is the uncontroversial part
@bors
Copy link
Contributor

bors commented Feb 21, 2023

☔ The latest upstream changes (presumably #108311) made this pull request unmergeable. Please resolve the merge conflicts.

@lcnr lcnr mentioned this pull request Feb 22, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 22, 2023
@matthiaskrgr
Rollup merge of rust-lang#108342 - lcnr:opaque-tys, r=oli-obk
2ad6a39 
apply query response: actually define opaque types

not sure whether this fixes any code considering that rust-lang#107891 doesn't break anything, but this is currently wrong as the `eq` there should just always fail right now.

We can definitely hit this code if we remove the `replace_opaque_types_with_inference_vars` hack. Doing so without this PR causes a few tests to ICE, e.g.

https://github.com/rust-lang/rust/blob/bd4a96a12d0bf6dc12edf20a45df3a33052c9d7d/tests/ui/impl-trait/issue-99642.rs#L1-L7

r? `@oli-obk`
@lcnr
Copy link
Contributor

lcnr commented Mar 31, 2023

i feel like we probably want to close this as we intend to refactor the DefiningAnchor anyways? 🤔

@lcnr lcnr added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2023
@oli-obk oli-obk closed this Mar 31, 2023
@apiraino apiraino removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Noratrieb Noratrieb Noratrieb left review comments

Assignees

@lcnr lcnr

Labels
perf-regression Performance regression. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@oli-obk @rustbot @rust-timer @bors @compiler-errors @craterbot @rust-log-analyzer @lcnr @Noratrieb @apiraino