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

[MCP] introduce ty::WhereClause to align chalk and rustc dyn repr #85466

Closed
wants to merge 2 commits into from

Conversation

csmoe
Copy link
Member

@csmoe csmoe commented May 19, 2021

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2021
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 4, 2021

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

@csmoe
Copy link
Member Author

csmoe commented Jun 8, 2021

@nikomatsakis ping for a review🔔

@@ -716,11 +716,13 @@ pub enum ExistentialPredicate<'tcx> {
AutoTrait(DefId),
}

impl<'tcx> ExistentialPredicate<'tcx> {
pub type ExistentialPredicate<'tcx> = Binder<'tcx, WhereClause<'tcx>>;
Copy link
Member

@jackh726 jackh726 Jul 3, 2021

Choose a reason for hiding this comment

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

This isn't quite right. The difference between rustc and Chalk isn't solely the name. In Chalk, there's actually two layers of Binders. One that contains a single type that represents Self, and one that represents the vars on the predicates itself. See https://github.com/rust-lang/chalk/blob/802599ce2ca02f322c078f472e64c5cd3a208225/chalk-solve/src/rust_ir.rs#L233

edit: this isn't actually accurate; let me write up a better comment

Copy link
Member

Choose a reason for hiding this comment

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

I guess the big thing missing here is the removal of erase_self_ty. And along with that, make the Self ty actually be a bound var.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2021
@bors
Copy link
Contributor

bors commented Jul 26, 2021

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

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@JohnCSimon JohnCSimon 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 Sep 13, 2021
@JohnCSimon
Copy link
Member

ping from triage:
@csmoe
Returning to you to address merge conflicts and after that switch this back to S-waiting-on-review.
thanks.

@nikomatsakis
Copy link
Contributor

@csmoe sorry this PR disappeared from my radar

@nikomatsakis
Copy link
Contributor

Are you still around?

@csmoe
Copy link
Member Author

csmoe commented Sep 24, 2021

Are you still around?

yep, time was tough. I'm back on this:)

@nikomatsakis
Copy link
Contributor

Augh! Let me give this a quick review @csmoe

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.

OK, so, I read the PR. From what I can tell, before this PR:

  • We have Binder<ExistentialPredicate>, where ExistentialPredicate is a subset of predicates that don't have Self position specified.

After this PR:

  • We introduce WhereClause to be the old ExistentialPredicate, and we make ExistentialPredicate an alias for Binder<WC>

This seems good, but indeed only a first step in the ultimate direction. Ultimately we want to extend WhereClause so it includes the Self type, and @jackh726 is right that this will mean introducing another level of Binder.

I think we also want to share WhereClause with Predicate.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 8, 2021

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

@JohnCSimon JohnCSimon removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 24, 2021
@JohnCSimon JohnCSimon added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 24, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 9, 2021

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

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.58.0-nightly (6f7dc9a98 2021-11-14) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z symbol-mangling-version=legacy -Z macro-backtrace -Z crate-attr=doc(html_root_url="https://doc.rust-lang.org/nightly/") -Z binary-dep-depinfo -Z force-unstable-if-unmarked -C opt-level=3 -C embed-bitcode=no -C codegen-units=1 -C debuginfo=0 -C debug-assertions=on -C link-args=-Wl,-rpath,$ORIGIN/../lib -C prefer-dynamic -C llvm-args=-import-instr-limit=10 -C embed-bitcode=yes --crate-type lib
note: some of the compiler flags provided by cargo are hidden

query stack during panic:
query stack during panic:
#0 [inferred_outlives_crate] computing the inferred outlives predicates for items in this crate
#1 [inferred_outlives_of] computing inferred outlives predicates of `cell::UnsafeCell`
error: could not compile `core`
Build completed unsuccessfully in 0:04:06

@bors
Copy link
Contributor

bors commented Nov 28, 2021

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

@crlf0710
Copy link
Member

@csmoe Ping from triage: Any updates here?

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 6, 2022
@Dylan-DPC
Copy link
Member

Closing this as inactive

@Dylan-DPC Dylan-DPC closed this Feb 22, 2022
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
10 participants