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

Relate alias ty with variance #116219

Merged
merged 3 commits into from
Oct 11, 2023

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Sep 28, 2023

In the new solver, turns out that the subst-relate branch of the alias-relate predicate was relating args invariantly even for opaques, which have variance 💀.

This change is a bit more invasive, but I'd rather not special-case it here and then have it break elsewhere. I'm doing a perf run to see if the extra call to def_kind is that expensive, if it is, I'll reconsider.

r? @lcnr

@compiler-errors
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@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. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 28, 2023
@bors
Copy link
Contributor

bors commented Sep 28, 2023

⌛ Trying commit e32b697 with merge 484b61b...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 28, 2023
…-variance, r=<try>

Relate alias ty with variance

In the new solver, turns out that the subst-relate branch of the alias-relate predicate was relating args invariantly even for opaques, which have variance 💀.

This change is a bit more invasive, but I'd rather not special-case it [here](https://github.com/rust-lang/rust/blob/aeaa5c30e5c9041264a2e8314b68ad84c2dc3169/compiler/rustc_trait_selection/src/solve/alias_relate.rs#L171-L190) and then have it break elsewhere.

r? `@lcnr`
@rust-cloud-vms rust-cloud-vms bot force-pushed the relate-alias-ty-with-variance branch from e32b697 to f310e3c Compare September 28, 2023 06:44
@compiler-errors

This comment was marked as duplicate.

1 similar comment
@compiler-errors
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Sep 28, 2023

⌛ Trying commit f310e3c with merge 834904f...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 28, 2023
…-variance, r=<try>

Relate alias ty with variance

In the new solver, turns out that the subst-relate branch of the alias-relate predicate was relating args invariantly even for opaques, which have variance 💀.

This change is a bit more invasive, but I'd rather not special-case it [here](https://github.com/rust-lang/rust/blob/aeaa5c30e5c9041264a2e8314b68ad84c2dc3169/compiler/rustc_trait_selection/src/solve/alias_relate.rs#L171-L190) and then have it break elsewhere.

r? `@lcnr`
@bors
Copy link
Contributor

bors commented Sep 28, 2023

⌛ Trying commit f310e3c with merge 2e44f1a...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 28, 2023
…-variance, r=<try>

Relate alias ty with variance

In the new solver, turns out that the subst-relate branch of the alias-relate predicate was relating args invariantly even for opaques, which have variance 💀.

This change is a bit more invasive, but I'd rather not special-case it [here](https://github.com/rust-lang/rust/blob/aeaa5c30e5c9041264a2e8314b68ad84c2dc3169/compiler/rustc_trait_selection/src/solve/alias_relate.rs#L171-L190) and then have it break elsewhere.

r? `@lcnr`
@rust-log-analyzer

This comment was marked as outdated.

b.args,
false, // do not fetch `type_of(a_def_id)`, as it will cause a cycle
)?,
DefKind::AssocTy | DefKind::AssocConst | DefKind::TyAlias => {
Copy link
Member

Choose a reason for hiding this comment

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

What about lazy type aliases? Shouldn't they be related with variances, too?

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 considered this, but it shouldn't matter in practice, since they're always normalizable.

This only really concerns things that must be considered as aliases.

@bors
Copy link
Contributor

bors commented Sep 28, 2023

☀️ Try build successful - checks-actions
Build commit: 2e44f1a (2e44f1a9b70ccff3d894e6dc190b49b3580650de)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2e44f1a): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.2% [0.2%, 0.2%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.3% [-0.3%, -0.3%] 1
Improvements ✅
(secondary)
-0.3% [-0.3%, -0.3%] 1
All ❌✅ (primary) 0.0% [-0.3%, 0.2%] 3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.1% [-1.1%, -1.1%] 2
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.947s -> 631.255s (-0.11%)
Artifact size: 317.35 MiB -> 317.36 MiB (0.00%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 28, 2023
@@ -273,7 +274,20 @@ impl<'tcx> Relate<'tcx> for ty::AliasTy<'tcx> {
if a.def_id != b.def_id {
Err(TypeError::ProjectionMismatched(expected_found(relation, a.def_id, b.def_id)))
} else {
let args = relation.relate(a.args, b.args)?;
let args = match relation.tcx().def_kind(a.def_id) {
DefKind::OpaqueTy => relate_args_with_variances(
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it be fine to use variances_of for all other aliases as well? at this point you could use relation.relate_item_args which avoids any lookup in Equate (and needs to call variances_of regardless if we're not in Equate)

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 don't think variances_of is impl'd for projections. I guess I could impl it, but that may have other perf implications.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, alternatively, instead of using the DefKind, have the Ty::relate provide the AliasKind to the substs relate.

do we need ProjectionTy to implement Relate itself, or would an inherent method relate which also takes the AliasKind be possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

do we need ProjectionTy to implement Relate itself, or would an inherent method relate which also takes the AliasKind be possible?

This is pretty difficult, because in order to invoke a relation, you need something that implements Self: ToTrace. That requires Self: Relate. I guess I could poke a hole through with something like trace, but even then, it doesn't seem really ergonomic. Or I could separate out ToTrace from Relate or something...

This also makes this code significantly uglier:

fn assemble_subst_relate_candidate(
&mut self,
param_env: ty::ParamEnv<'tcx>,
alias_lhs: ty::AliasTy<'tcx>,
alias_rhs: ty::AliasTy<'tcx>,
direction: ty::AliasRelationDirection,
) -> QueryResult<'tcx> {
self.probe_misc_candidate("args relate").enter(|ecx| {
match direction {
ty::AliasRelationDirection::Equate => {
ecx.eq(param_env, alias_lhs, alias_rhs)?;
}
ty::AliasRelationDirection::Subtype => {
ecx.sub(param_env, alias_lhs, alias_rhs)?;
}
}
ecx.evaluate_added_goals_and_make_canonical_response(Certainty::Yes)
})
}

Since we can't just use the generic eq and sub methods on EvalCtxt.

@compiler-errors
Copy link
Member Author

I'm somewhat skeptical of the perf regression, but maybe it's real? Diesel looks spiky recently.

@rust-cloud-vms rust-cloud-vms bot force-pushed the relate-alias-ty-with-variance branch from f310e3c to be29d22 Compare October 4, 2023 04:22
@compiler-errors
Copy link
Member Author

Rebased and testing again

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 4, 2023
@bors
Copy link
Contributor

bors commented Oct 4, 2023

⌛ Trying commit be29d22 with merge 0027216...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2023
…-variance, r=<try>

Relate alias ty with variance

In the new solver, turns out that the subst-relate branch of the alias-relate predicate was relating args invariantly even for opaques, which have variance 💀.

This change is a bit more invasive, but I'd rather not special-case it [here](https://github.com/rust-lang/rust/blob/aeaa5c30e5c9041264a2e8314b68ad84c2dc3169/compiler/rustc_trait_selection/src/solve/alias_relate.rs#L171-L190) and then have it break elsewhere. I'm doing a perf run to see if the extra call to `def_kind` is that expensive, if it is, I'll reconsider.

r? `@lcnr`
@bors
Copy link
Contributor

bors commented Oct 4, 2023

☀️ Try build successful - checks-actions
Build commit: 0027216 (0027216f8bc36b3488f0390f02948671465ff957)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0027216): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.9% [-1.1%, -0.7%] 7
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.4% [1.4%, 1.4%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.2% [-3.2%, -3.2%] 1
All ❌✅ (primary) 1.4% [1.4%, 1.4%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 622.307s -> 623.821s (0.24%)
Artifact size: 272.04 MiB -> 272.04 MiB (0.00%)

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Oct 4, 2023
@lcnr
Copy link
Contributor

lcnr commented Oct 11, 2023

@bors r+ rollup (new solver)

@bors
Copy link
Contributor

bors commented Oct 11, 2023

📌 Commit be29d22 has been approved by 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 Oct 11, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 11, 2023
…th-variance, r=lcnr

Relate alias ty with variance

In the new solver, turns out that the subst-relate branch of the alias-relate predicate was relating args invariantly even for opaques, which have variance 💀.

This change is a bit more invasive, but I'd rather not special-case it [here](https://github.com/rust-lang/rust/blob/aeaa5c30e5c9041264a2e8314b68ad84c2dc3169/compiler/rustc_trait_selection/src/solve/alias_relate.rs#L171-L190) and then have it break elsewhere. I'm doing a perf run to see if the extra call to `def_kind` is that expensive, if it is, I'll reconsider.

r? `@lcnr`
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#116219 (Relate alias ty with variance)
 - rust-lang#116315 (Do not check for impossible predicates in const-prop lint.)
 - rust-lang#116436 (Structurally normalize for closure)
 - rust-lang#116597 (Prevent showing methods from blanket impls of not available foreign traits to show up in the search results)
 - rust-lang#116627 (small cleanup)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8ddc0df into rust-lang:master Oct 11, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Oct 11, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2023
Rollup merge of rust-lang#116219 - compiler-errors:relate-alias-ty-with-variance, r=lcnr

Relate alias ty with variance

In the new solver, turns out that the subst-relate branch of the alias-relate predicate was relating args invariantly even for opaques, which have variance 💀.

This change is a bit more invasive, but I'd rather not special-case it [here](https://github.com/rust-lang/rust/blob/aeaa5c30e5c9041264a2e8314b68ad84c2dc3169/compiler/rustc_trait_selection/src/solve/alias_relate.rs#L171-L190) and then have it break elsewhere. I'm doing a perf run to see if the extra call to `def_kind` is that expensive, if it is, I'll reconsider.

r? ``@lcnr``
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 12, 2023
48: Pull upstream master 2023 10 12 r=tshepang a=Dajamante

* rust-lang/rust#113487
* rust-lang/rust#116506
* rust-lang/rust#116448
* rust-lang/rust#116640
  * rust-lang/rust#116627
  * rust-lang/rust#116597
  * rust-lang/rust#116436
  * rust-lang/rust#116315
  * rust-lang/rust#116219
* rust-lang/rust#113218
* rust-lang/rust#115937
* rust-lang/rust#116014
* rust-lang/rust#116623
* rust-lang/rust#112818
* rust-lang/rust#115948
* rust-lang/rust#116622
* rust-lang/rust#116621
  * rust-lang/rust#116612
  * rust-lang/rust#116611
  * rust-lang/rust#116530
  * rust-lang/rust#95967
* rust-lang/rust#116578
* rust-lang/rust#113915
* rust-lang/rust#116605
  * rust-lang/rust#116574
  * rust-lang/rust#116560
  * rust-lang/rust#116559
  * rust-lang/rust#116503
  * rust-lang/rust#116444
  * rust-lang/rust#116250
  * rust-lang/rust#109422
* rust-lang/rust#116598
  * rust-lang/rust#116596
  * rust-lang/rust#116595
  * rust-lang/rust#116589
  * rust-lang/rust#116586
* rust-lang/rust#116551
* rust-lang/rust#116409
* rust-lang/rust#116548
* rust-lang/rust#116366
* rust-lang/rust#109882
* rust-lang/rust#116497
* rust-lang/rust#116532
* rust-lang/rust#116569
  * rust-lang/rust#116561
  * rust-lang/rust#116556
  * rust-lang/rust#116549
  * rust-lang/rust#116543
  * rust-lang/rust#116537
  * rust-lang/rust#115882
* rust-lang/rust#116142
* rust-lang/rust#115238
* rust-lang/rust#116533
* rust-lang/rust#116096
* rust-lang/rust#116468
* rust-lang/rust#116515
* rust-lang/rust#116454
* rust-lang/rust#116183
* rust-lang/rust#116514
* rust-lang/rust#116509
* rust-lang/rust#116487
* rust-lang/rust#116486
* rust-lang/rust#116450
* rust-lang/rust#114623
* rust-lang/rust#116416
* rust-lang/rust#116437
* rust-lang/rust#100806
* rust-lang/rust#116330
* rust-lang/rust#116310
* rust-lang/rust#115583
* rust-lang/rust#116457
* rust-lang/rust#116508
* rust-lang/rust#109214
* rust-lang/rust#116318
* rust-lang/rust#116501
  * rust-lang/rust#116500
  * rust-lang/rust#116458
  * rust-lang/rust#116400
  * rust-lang/rust#116277
* rust-lang/rust#114709
* rust-lang/rust#116492
  * rust-lang/rust#116484
  * rust-lang/rust#116481
  * rust-lang/rust#116474
  * rust-lang/rust#116466
  * rust-lang/rust#116423
  * rust-lang/rust#116297
  * rust-lang/rust#114564
* rust-lang/rust#114811
* rust-lang/rust#116489
* rust-lang/rust#115304

Co-authored-by: Peter Hall <peter.hall@hyperexponential.com>
Co-authored-by: Emanuele Vannacci <emanuele.vannacci@gmail.com>
Co-authored-by: Neven Villani <vanille@crans.org>
Co-authored-by: Alex Macleod <alex@macleod.io>
Co-authored-by: Tamir Duberstein <tamird@gmail.com>
Co-authored-by: Eduardo Sánchez Muñoz <eduardosm-dev@e64.io>
Co-authored-by: koka <koka.code@gmail.com>
Co-authored-by: bors <bors@rust-lang.org>
Co-authored-by: Philipp Krones <hello@philkrones.com>
Co-authored-by: Camille GILLOT <gillot.camille@gmail.com>
Co-authored-by: Esteban Küber <esteban@kuber.com.ar>
Co-authored-by: Ralf Jung <post@ralfj.de>
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 13, 2023
48: Pull upstream master 2023 10 12 r=tshepang a=Dajamante

* rust-lang/rust#113487
* rust-lang/rust#116506
* rust-lang/rust#116448
* rust-lang/rust#116640
  * rust-lang/rust#116627
  * rust-lang/rust#116597
  * rust-lang/rust#116436
  * rust-lang/rust#116315
  * rust-lang/rust#116219
* rust-lang/rust#113218
* rust-lang/rust#115937
* rust-lang/rust#116014
* rust-lang/rust#116623
* rust-lang/rust#112818
* rust-lang/rust#115948
* rust-lang/rust#116622
* rust-lang/rust#116621
  * rust-lang/rust#116612
  * rust-lang/rust#116611
  * rust-lang/rust#116530
  * rust-lang/rust#95967
* rust-lang/rust#116578
* rust-lang/rust#113915
* rust-lang/rust#116605
  * rust-lang/rust#116574
  * rust-lang/rust#116560
  * rust-lang/rust#116559
  * rust-lang/rust#116503
  * rust-lang/rust#116444
  * rust-lang/rust#116250
  * rust-lang/rust#109422
* rust-lang/rust#116598
  * rust-lang/rust#116596
  * rust-lang/rust#116595
  * rust-lang/rust#116589
  * rust-lang/rust#116586
* rust-lang/rust#116551
* rust-lang/rust#116409
* rust-lang/rust#116548
* rust-lang/rust#116366
* rust-lang/rust#109882
* rust-lang/rust#116497
* rust-lang/rust#116532
* rust-lang/rust#116569
  * rust-lang/rust#116561
  * rust-lang/rust#116556
  * rust-lang/rust#116549
  * rust-lang/rust#116543
  * rust-lang/rust#116537
  * rust-lang/rust#115882
* rust-lang/rust#116142
* rust-lang/rust#115238
* rust-lang/rust#116533
* rust-lang/rust#116096
* rust-lang/rust#116468
* rust-lang/rust#116515
* rust-lang/rust#116454
* rust-lang/rust#116183
* rust-lang/rust#116514
* rust-lang/rust#116509
* rust-lang/rust#116487
* rust-lang/rust#116486
* rust-lang/rust#116450
* rust-lang/rust#114623
* rust-lang/rust#116416
* rust-lang/rust#116437
* rust-lang/rust#100806
* rust-lang/rust#116330
* rust-lang/rust#116310
* rust-lang/rust#115583
* rust-lang/rust#116457
* rust-lang/rust#116508
* rust-lang/rust#109214
* rust-lang/rust#116318
* rust-lang/rust#116501
  * rust-lang/rust#116500
  * rust-lang/rust#116458
  * rust-lang/rust#116400
  * rust-lang/rust#116277
* rust-lang/rust#114709
* rust-lang/rust#116492
  * rust-lang/rust#116484
  * rust-lang/rust#116481
  * rust-lang/rust#116474
  * rust-lang/rust#116466
  * rust-lang/rust#116423
  * rust-lang/rust#116297
  * rust-lang/rust#114564
* rust-lang/rust#114811
* rust-lang/rust#116489
* rust-lang/rust#115304

Co-authored-by: Emanuele Vannacci <emanuele.vannacci@gmail.com>
Co-authored-by: Neven Villani <vanille@crans.org>
Co-authored-by: Alex Macleod <alex@macleod.io>
Co-authored-by: Tamir Duberstein <tamird@gmail.com>
Co-authored-by: Eduardo Sánchez Muñoz <eduardosm-dev@e64.io>
Co-authored-by: koka <koka.code@gmail.com>
Co-authored-by: bors <bors@rust-lang.org>
Co-authored-by: Philipp Krones <hello@philkrones.com>
Co-authored-by: Camille GILLOT <gillot.camille@gmail.com>
Co-authored-by: Esteban Küber <esteban@kuber.com.ar>
Co-authored-by: Ralf Jung <post@ralfj.de>
Co-authored-by: ShE3py <52315535+she3py@users.noreply.github.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants