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

Use obligation ctxt instead of dyn TraitEngine #104509

Merged
merged 3 commits into from
Nov 23, 2022
Merged

Use obligation ctxt instead of dyn TraitEngine #104509

merged 3 commits into from
Nov 23, 2022

Conversation

spastorino
Copy link
Member

@spastorino spastorino commented Nov 16, 2022 •
edited
Loading

r? @lcnr

@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 Nov 16, 2022
compiler-errors
compiler-errors reviewed Nov 16, 2022
View reviewed changes
compiler/rustc_traits/src/implied_outlives_bounds.rs
Comment on lines +117 to 121
match ocx.select_all_or_error().as_slice() {
[] => (),
_ => return Err(NoSolution),
}
Copy link
Member

Choose a reason for hiding this comment

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

What does calling select_all_or_error here do to the select_* calls that happen in make_query_response, since we're inside of a canonical query here?

Copy link
Member

Choose a reason for hiding this comment

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

(Maybe it does nothing -- just curious, since we're passing down the ObligationCtxt from the canonical query instead of creating a trait engine inline, it might change behavior -- but maybe not!)

Copy link
Contributor

@lcnr lcnr Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

it is necessary to constrain inference variables which we use further down when computing the implied bounds. Can you add a comment mentioning that here?

@compiler-errors
Copy link
Member

(nit: also the commits for fully_solve_obligation, fully_solve_obligation, and fully_solve_obligation could be squashed since they're right next to each other and also almost identical -- or they could be reworked so they all call fully_solve_obligations 😃 )

@spastorino
Copy link
Member Author

Yes, I'm going to squash them all. Right now with the cellphone but will do ASAP and also what you're suggesting.

@compiler-errors
Copy link
Member

No rush anyways, thanks 👍

@lcnr
Copy link
Contributor

lcnr commented Nov 17, 2022

r=me after squashing some commits and adding a comment to compiler/rustc_traits/src/implied_outlives_bounds.rs

@spastorino
Copy link
Member Author

This is now ready!

@lcnr
Copy link
Contributor

lcnr commented Nov 17, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 17, 2022

📌 Commit 9864a63 has been approved by lcnr

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 17, 2022

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.

@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 Nov 17, 2022
@spastorino
Copy link
Member Author

Unsure what happened with this PR but I think it needs to be re-approved.

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Nov 22, 2022

📌 Commit 9864a63 has been approved by lcnr

It is now in the queue for this repository.

Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 22, 2022
@Manishearth
Rollup merge of rust-lang#104509 - spastorino:use-obligation-ctxt, r=…
bbe1843 
…lcnr

Use obligation ctxt instead of dyn TraitEngine

r? `@lcnr`
@Manishearth Manishearth mentioned this pull request Nov 22, 2022
Closed
@compiler-errors
Copy link
Member

compiler-errors commented Nov 22, 2022 •
edited
Loading

This needs re-bless, cc #104738

@Manishearth
Copy link
Member

@bors r-

#104738 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 22, 2022
@spastorino
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Nov 23, 2022

📌 Commit 859b147 has been approved by lcnr

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 23, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 23, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Nov 23, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 23, 2022
@bors
Auto merge of rust-lang#104776 - Dylan-DPC:rollup-rf4c2u0, r=Dylan-DPC
d121aa3 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#104269 (Fix hang in where-clause suggestion with `predicate_can_apply`)
 - rust-lang#104286 (copy doc output files by format)
 - rust-lang#104509 (Use obligation ctxt instead of dyn TraitEngine)
 - rust-lang#104721 (Remove more `ref` patterns from the compiler)
 - rust-lang#104744 (rustdoc: give struct fields CSS `display: block`)
 - rust-lang#104751 (Fix an ICE parsing a malformed attribute.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0a79138 into rust-lang:master Nov 23, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

@lcnr lcnr lcnr left review comments

Assignees

@lcnr lcnr

Labels
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
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.
6 participants
@spastorino @compiler-errors @lcnr @bors @Manishearth @rustbot