Skip to content

Make alias-eq have a relation direction (and rename it to alias-relate) #109462

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Mar 23, 2023

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Mar 21, 2023

Emitting an "alias-eq" is too strict in some situations, since we don't always want strict equality between a projection and rigid ty. Adds a relation direction.

  • I could probably just reuse this RelationDir -- happy to uplift that struct into middle and use that instead, but I didn't feel compelled to... 🤷
  • Some of the matching in compute_alias_relate_goal is a bit verbose -- I guess I could simplify it by using At::relate and mapping the relation-dir to a variance.
  • Alternatively, I coulld simplify things by making more helper functions on EvalCtxt (e.g. EvalCtxt::relate_with_direction(T, T) that also does the nested goal registration). No preference.

r? @lcnr cc @BoxyUwU though boxy can claim it if she wants
NOTE: first commit is all the changes, the second is just renaming stuff

@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Mar 21, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 21, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to the core trait solver

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

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

for caching it would be better if we only have AliasRelateDir::Eq and AliasRelateDir::Sub, or maybe in that case AliasRelateMode::Eq and AliasRelateMode::Sub, and instead of flipping the direction, flipping the order of the aliases 🤔

apart from that and nits 👍

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

This comment has been minimized.

///
/// FIXME: rename `AliasTy` to `AliasTerm` and make sure we correctly
/// deal with constants.
pub fn to_alias_term_no_opaque(&self, tcx: TyCtxt<'tcx>) -> Option<AliasTy<'tcx>> {
pub fn to_projection_term(&self, tcx: TyCtxt<'tcx>) -> Option<AliasTy<'tcx>> {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of this rename, when we start representing free type aliases in Ty I would expect this function to return Some for them. It's also not really accurate for ConstKind::Unevaluated which represents anon consts which are sort of like free consts (although nothing about normalization or equality of ConstKind::Unevaluated is really implemented right for new solver yet so shrug)

Copy link
Member Author

Choose a reason for hiding this comment

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

I just think the old name is unnecessarily obtuse though, and also kinda long -- I'd rather have the behavior not be defined by what it is "not" if possible. If you can think of a better one, though, happy to consider naming it something else.

@compiler-errors
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2023
@BoxyUwU
Copy link
Member

BoxyUwU commented Mar 23, 2023

@bors r=BoxyUwU,lcnr
(I assume from the "apart from that and nits 👍" lcnr is fine with this PR though can always r- if misinterpreted that)

@bors
Copy link
Collaborator

bors commented Mar 23, 2023

📌 Commit 244cdaa has been approved by BoxyUwU,lcnr

It is now in the queue for this repository.

@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 Mar 23, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2023
…xyUwU,lcnr

Make alias-eq have a relation direction (and rename it to alias-relate)

Emitting an "alias-eq" is too strict in some situations, since we don't always want strict equality between a projection and rigid ty. Adds a relation direction.

* I could probably just reuse this [`RelationDir`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/combine/enum.RelationDir.html) -- happy to uplift that struct into middle and use that instead, but I didn't feel compelled to... 🤷
* Some of the matching in `compute_alias_relate_goal` is a bit verbose -- I guess I could simplify it by using [`At::relate`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/at/struct.At.html#method.relate) and mapping the relation-dir to a variance.
* Alternatively, I coulld simplify things by making more helper functions on `EvalCtxt` (e.g. `EvalCtxt::relate_with_direction(T, T)` that also does the nested goal registration). No preference.

r? `@lcnr` cc `@BoxyUwU` though boxy can claim it if she wants
NOTE: first commit is all the changes, the second is just renaming stuff
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2023
…xyUwU,lcnr

Make alias-eq have a relation direction (and rename it to alias-relate)

Emitting an "alias-eq" is too strict in some situations, since we don't always want strict equality between a projection and rigid ty. Adds a relation direction.

* I could probably just reuse this [`RelationDir`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/combine/enum.RelationDir.html) -- happy to uplift that struct into middle and use that instead, but I didn't feel compelled to... 🤷
* Some of the matching in `compute_alias_relate_goal` is a bit verbose -- I guess I could simplify it by using [`At::relate`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/at/struct.At.html#method.relate) and mapping the relation-dir to a variance.
* Alternatively, I coulld simplify things by making more helper functions on `EvalCtxt` (e.g. `EvalCtxt::relate_with_direction(T, T)` that also does the nested goal registration). No preference.

r? ``@lcnr`` cc ``@BoxyUwU`` though boxy can claim it if she wants
NOTE: first commit is all the changes, the second is just renaming stuff
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#108541 (Suppress `opaque_hidden_inferred_bound` for nested RPITs)
 - rust-lang#109137 (resolve: Querify most cstore access methods (subset 2))
 - rust-lang#109380 (add `known-bug` test for unsoundness issue)
 - rust-lang#109462 (Make alias-eq have a relation direction (and rename it to alias-relate))
 - rust-lang#109475 (Simpler checked shifts in MIR building)
 - rust-lang#109504 (Stabilize `arc_into_inner` and `rc_into_inner`.)
 - rust-lang#109506 (make param bound vars visibly bound vars with -Zverbose)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5d28853 into rust-lang:master Mar 23, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 23, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 24, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#108541 (Suppress `opaque_hidden_inferred_bound` for nested RPITs)
 - rust-lang#109137 (resolve: Querify most cstore access methods (subset 2))
 - rust-lang#109380 (add `known-bug` test for unsoundness issue)
 - rust-lang#109462 (Make alias-eq have a relation direction (and rename it to alias-relate))
 - rust-lang#109475 (Simpler checked shifts in MIR building)
 - rust-lang#109504 (Stabilize `arc_into_inner` and `rc_into_inner`.)
 - rust-lang#109506 (make param bound vars visibly bound vars with -Zverbose)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@compiler-errors compiler-errors deleted the alias-relate branch August 11, 2023 19:54
# for free to join this conversation on GitHub. Already have an account? # to comment
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. T-rustdoc Relevant to the rustdoc 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
Development

Successfully merging this pull request may close these issues.

6 participants