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

Compute variances for lazy type aliases #114253

Merged
merged 2 commits into from
Aug 4, 2023

Conversation

fmease
Copy link
Member

@fmease fmease commented Jul 30, 2023

Fixes #114221.

CC @oli-obk
r? types

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) F-lazy_type_alias `#![feature(lazy_type_alias)]` T-types Relevant to the types team, which will review and decide on the PR/issue. labels Jul 30, 2023
@rust-log-analyzer

This comment has been minimized.

@fmease fmease marked this pull request as draft July 30, 2023 16:49
@fmease

This comment was marked as outdated.

@rustbot rustbot 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 Jul 30, 2023
@fmease fmease force-pushed the compute-variances-for-lazy-ty-aliases branch from 0e5d01b to 253495a Compare July 30, 2023 19:35
@fmease
Copy link
Member Author

fmease commented Jul 30, 2023

I'm pretty sure that all occurrences of tcx.features().lazy_type_alias in this patch should actually be tcx.features().lazy_type_alias || tcx.type_of(def_id).subst_identity().has_opaque_types() to consolidate (lazy) type aliases and TAITs.

I tried that locally and all UI tests succeeded except for

  1. a single TAIT test, namely type-alias-impl-trait/obligation_ice.rs (pass → fail): For some reason, the type parameter A on type alias I gets flagged as unused / bivariant leading to an error. Maybe it has something to do with the way variances are computed for opaques (I does contain one).
  2. several inherent associated type tests (pass / fail → cycle error): Since their currently implementation is makeshift & fragile, the breakage isn't that noteworthy I'd say.

I don't think I know enough about TAITs to be able to fix point (1).

@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 Jul 30, 2023
@fmease fmease marked this pull request as ready for review July 30, 2023 19:48
Comment on lines 136 to 137
if tcx.features().lazy_type_alias && let DefKind::TyAlias = tcx.def_kind(def_id) {
self.add_constraints_from_ty(current_item, ty, self.covariant);
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave a comment explaining that we need to check the DefKind, because type_of(def_id) returns the type behind the alias, not ty::Alias(ty::Weak, ..)

@oli-obk
Copy link
Contributor

oli-obk commented Aug 1, 2023

  1. a single TAIT test, namely type-alias-impl-trait/obligation_ice.rs (pass → fail): For some reason, the type parameter A on type alias I gets flagged as unused / bivariant leading to an error. Maybe it has something to do with the way variances are computed for opaques (I does contain one).

you could dump the variance (RUSTC_LOG=[crate_variances]) and the hidden type (RUSTC_LOG=[type_of]) to collect some more information on this. It seems like the right approach that you're doing.

Note that that test's RPIT equivalent does not compile (https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b00a68250b843d20bfa300f35fc7c29c) without also adding a where 'b: 'a bound to the function. So something is fishy here anyway

@fmease fmease marked this pull request as draft August 1, 2023 16:17
@fmease fmease force-pushed the compute-variances-for-lazy-ty-aliases branch from 253495a to 1abe10a Compare August 2, 2023 09:39
@fmease fmease marked this pull request as ready for review August 2, 2023 09:40
@fmease fmease force-pushed the compute-variances-for-lazy-ty-aliases branch from 1abe10a to 33faad6 Compare August 2, 2023 09:53
@rust-log-analyzer

This comment has been minimized.

@fmease fmease force-pushed the compute-variances-for-lazy-ty-aliases branch from 33faad6 to 16e3b80 Compare August 2, 2023 23:05
@rust-log-analyzer

This comment has been minimized.

@fmease fmease force-pushed the compute-variances-for-lazy-ty-aliases branch from 16e3b80 to 6f5d855 Compare August 3, 2023 00:19
@rustbot rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Aug 3, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this test affected? It has neither opaque types nor the lazy type alias feature gate

@fmease
Copy link
Member Author

fmease commented Aug 3, 2023

Note that inherent associated types bound to type aliases now lead to cycle errors as previously touched upon. In the relevant UI tests I've replaced type aliases with functions wherever possible.

Superficially the cycles are caused by the added type_of calls in variances_of but more fundamentally the current implementation of inherent associated types (written by me) is prone to such errors since selection (which tries to acquire a (normalized) ParamEnv) happens in astconv.

I'm pretty sure those regressions are unavoidable at the present time. I plan on attempting an IAT rewrite at some point of which I'm not sure if it will pan out or not.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 3, 2023

📌 Commit 6f5d855 has been approved by oli-obk

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 Aug 3, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 3, 2023
…ty-aliases, r=oli-obk

Compute variances for lazy type aliases

Fixes rust-lang#114221.

CC `@oli-obk`
r? types
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 4, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#114022 (Perform OpaqueCast field projection on HIR, too.)
 - rust-lang#114253 (Compute variances for lazy type aliases)
 - rust-lang#114355 (resolve before canonicalization in new solver, ICE if unresolved)
 - rust-lang#114427 (Handle non-utf8 rpaths (fix FIXME))
 - rust-lang#114440 (bootstrap: config: fix version comparison bug)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5ea536b into rust-lang:master Aug 4, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 4, 2023
@fmease fmease deleted the compute-variances-for-lazy-ty-aliases branch August 4, 2023 10:59
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 8, 2023
…e-specific, r=oli-obk

Store the laziness of type aliases in their `DefKind`

Previously, we would treat paths referring to type aliases as *lazy* type aliases if the current crate had lazy type aliases enabled independently of whether the crate which the alias was defined in had the feature enabled or not.

With this PR, the laziness of a type alias depends on the crate it is defined in. This generally makes more sense to me especially if / once lazy type aliases become the default in a new edition and we need to think about *edition interoperability*:

Consider the hypothetical case where the dependency crate has an older edition (and thus eager type aliases), it exports a type alias with bounds & a where-clause (which are void but technically valid), the dependent crate has the latest edition (and thus lazy type aliases) and it uses that type alias. Arguably, the bounds should *not* be checked since at any time, the dependency crate should be allowed to change the bounds at will with a *non*-major version bump & without negatively affecting downstream crates.

As for the reverse case (dependency: lazy type aliases, dependent: eager type aliases), I guess it rules out anything from slight confusion to mild annoyance from upstream crate authors that would be caused by the compiler ignoring the bounds of their type aliases in downstream crates with older editions.

---

This fixes rust-lang#114468 since before, my assumption that the type alias associated with a given weak projection was lazy (and therefore had its variances computed) did not necessarily hold in cross-crate scenarios (which [I kinda had a hunch about](rust-lang#114253 (comment))) as outlined above. Now it does hold.

`@rustbot` label F-lazy_type_alias
r? `@oli-obk`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Aug 10, 2023
…c, r=oli-obk

Store the laziness of type aliases in their `DefKind`

Previously, we would treat paths referring to type aliases as *lazy* type aliases if the current crate had lazy type aliases enabled independently of whether the crate which the alias was defined in had the feature enabled or not.

With this PR, the laziness of a type alias depends on the crate it is defined in. This generally makes more sense to me especially if / once lazy type aliases become the default in a new edition and we need to think about *edition interoperability*:

Consider the hypothetical case where the dependency crate has an older edition (and thus eager type aliases), it exports a type alias with bounds & a where-clause (which are void but technically valid), the dependent crate has the latest edition (and thus lazy type aliases) and it uses that type alias. Arguably, the bounds should *not* be checked since at any time, the dependency crate should be allowed to change the bounds at will with a *non*-major version bump & without negatively affecting downstream crates.

As for the reverse case (dependency: lazy type aliases, dependent: eager type aliases), I guess it rules out anything from slight confusion to mild annoyance from upstream crate authors that would be caused by the compiler ignoring the bounds of their type aliases in downstream crates with older editions.

---

This fixes #114468 since before, my assumption that the type alias associated with a given weak projection was lazy (and therefore had its variances computed) did not necessarily hold in cross-crate scenarios (which [I kinda had a hunch about](rust-lang/rust#114253 (comment))) as outlined above. Now it does hold.

`@rustbot` label F-lazy_type_alias
r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc A-variance Area: Variance (https://doc.rust-lang.org/nomicon/subtyping.html) F-lazy_type_alias `#![feature(lazy_type_alias)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lazy_type_alias causes lifetime error
6 participants