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

check universes when instantiating infer vars with placeholders #109813

Closed
wants to merge 2 commits into from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Mar 31, 2023

fixes #109505

probably not going to finish that soon and would like someone to take this over.

what needs to be done:

  • add tests triggering all codepaths: at least the combine and the const generalizer
  • we should merge the 3 generalizers into 1 here. Having to duplicate the same checks 36 times is bound to cause issues at some point

r? @compiler-errors @BoxyUwU

@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 31, 2023
@rust-log-analyzer

This comment has been minimized.

@BoxyUwU BoxyUwU self-assigned this Mar 31, 2023
Copy link
Member

@aliemjay aliemjay left a comment

Choose a reason for hiding this comment

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

In addition to merging the generalizer, another couple notes:

  • rename TypeVariableTable::{instantiate => instantiate_ignoring_universe} to make it clear that the caller is responsible for checking universes.

compiler/rustc_infer/src/infer/combine.rs Show resolved Hide resolved
Comment on lines +843 to 844
// Need to manually relate as `super_relate_consts` is not simply structural.
ty::ConstKind::Unevaluated(ty::UnevaluatedConst { def, substs }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, though this requires changing super_relate_consts to not do const evaluation. Probably out of the scope of this PR.

@lcnr
Copy link
Contributor Author

lcnr commented Apr 12, 2023

won't finish this work, would be good for someone to take this over 🤔

@lcnr lcnr closed this Apr 12, 2023
@BoxyUwU
Copy link
Member

BoxyUwU commented Apr 12, 2023

If nobody has done it by the time im done working on proof tree output for new solver and finished experimenting with representing universes via trees instead of a counter then I'll get to this

@lcnr lcnr mentioned this pull request May 2, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 15, 2023
…lcnr

Combine three generalizer implementations

Fixes rust-lang#111092
Fixes rust-lang#109505

This code is a bit delicate and there were subtle changes between them, so I'll leave inline comments where further inspection is needed.

Regarding this comment from rust-lang#109813 -- "add tests triggering all codepaths: at least the combine and the const generalizer", can't really do that now, and I don't really know how we'd get a higher-ranked const error since non-lifetime binders doesn't *really* support `for<const ..>` (it errors out when you try to use it).

r? `@lcnr`
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.

No universe checks for ty/const infer vars and placeholders
6 participants