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

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

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 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 👍

compiler/rustc_infer/src/infer/nll_relate/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/solve/mod.rs Outdated Show resolved Hide resolved
@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.

compiler/rustc_infer/src/infer/combine.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/glb.rs Outdated Show resolved Hide resolved
///
/// 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/rustc_middle/src/ty/structural_impls.rs Outdated Show resolved Hide resolved
src/tools/clippy/clippy_utils/src/qualify_min_const_fn.rs Outdated Show resolved Hide resolved
@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
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants