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

rustc: replace TyCtxt<'a, 'gcx, 'tcx> with TyCtxt<'gcx, 'tcx>. #61722

Merged
merged 7 commits into from
Jun 12, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jun 10, 2019

This first lifetime parameter of TyCtxt has been phantom for a while, thanks to @Zoxc, but was never removed, and I'm doing this now in preparation for removing the 'gcx/'tcx split.

I wasn't going to do this as a separate step, and instead start converting uses of TyCtxt to a single-lifetime alias of it (e.g. type TyCx<'tcx> = TyCtxt<'tcx, 'tcx, 'tcx>;) but it turns out (as @Zoxc rightly predicted) that there is far more fallout from not needing a lifetime for the first parameter of TyCtxt.

That is, going from TyCtxt<'a, 'gcx, 'tcx> to TyCtxt<'tcx, 'gcx, 'tcx> (the first commit in this PR) has the largest amount of fallout out of all the changes we might make (because it can require removing the 'a parameter of structs containing tcx: TyCtxt<'a, ...>), and is the hardest to automate (because 'a is used everywhere, not just with TyCtxt, unlike, say 'gcx, 'tcx -> 'tcx).

So I'm submitting this now to get it out of the way and reduce further friction in the future.

EDIT: for the rustfmt commit, I used rust-lang/rustfmt#1324 (comment), and manually filtered out some noise, like in #61735, but unlike that PR, there was also a weird bug to work around.
It should be reviewed separately, and dropped if unwanted.

cc @rust-lang/compiler r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 10, 2019
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

bors added a commit that referenced this pull request Jun 11, 2019
Add deny(unused_lifetimes) to all the crates that have deny(internal).

@Zoxc brought up, regarding #61722, that we don't force the removal of unused lifetimes.
Turns out that it's not that bad to enable for compiler crates (I wonder why it's not `warn` by default?).

I would've liked to enable `single_use_lifetimes` as well, but #53738 makes it unusable for now.

For the `rustfmt` commit, I used rust-lang/rustfmt#1324 (comment), and manually filtered out some noise.

r? @oli-obk cc @rust-lang/compiler
@eddyb eddyb force-pushed the vowel-exclusion-zone branch 2 times, most recently from 1b99bba to 0472b6c Compare June 11, 2019 21:13
@eddyb

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@estebank

This comment has been minimized.

@eddyb

This comment has been minimized.

@bors

This comment has been minimized.

@eddyb eddyb force-pushed the vowel-exclusion-zone branch 2 times, most recently from 8af839a to 18f8668 Compare June 12, 2019 10:15
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

r=me with leftover unnecessary lifetimes removed

@@ -494,11 +494,11 @@ impl VidValuePair<'tcx> for (Ty<'tcx>, ty::TyVid) {
}
}

impl<D> TypeRelation<'me, 'gcx, 'tcx> for TypeRelating<'me, 'gcx, 'tcx, D>
impl<D> TypeRelation<'gcx, 'tcx> for TypeRelating<'me, 'gcx, 'tcx, D>
Copy link
Contributor

Choose a reason for hiding this comment

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

The 'me lifetime can be '_ here.

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 believe that'd be handled by the "single-use lifetime" lint, which we haven't turned on yet.

src/librustc_codegen_llvm/base.rs Outdated Show resolved Hide resolved
src/librustc_codegen_llvm/base.rs Outdated Show resolved Hide resolved
@eddyb
Copy link
Member Author

eddyb commented Jun 12, 2019

@bors rollup=never p=10 (high bitrot probability)

@eddyb
Copy link
Member Author

eddyb commented Jun 12, 2019

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jun 12, 2019

📌 Commit 4c98cb6 has been approved by oli-obk

@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 Jun 12, 2019
@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 12, 2019

⌛ Testing commit 4c98cb6 with merge 24ddd16...

bors added a commit that referenced this pull request Jun 12, 2019
rustc: replace `TyCtxt<'a, 'gcx, 'tcx>` with `TyCtxt<'gcx, 'tcx>`.

This first lifetime parameter of `TyCtxt` has been phantom for a while, thanks to @Zoxc, but was never removed, and I'm doing this now in preparation for removing the `'gcx`/`'tcx` split.

I wasn't going to do this as a separate step, and instead start converting uses of `TyCtxt` to a single-lifetime alias of it (e.g. `type TyCx<'tcx> = TyCtxt<'tcx, 'tcx, 'tcx>;`) but it turns out (as @Zoxc rightly predicted) that there is far more fallout from not needing a lifetime for the first parameter of `TyCtxt`.

That is, going from `TyCtxt<'a, 'gcx, 'tcx>` to `TyCtxt<'tcx, 'gcx, 'tcx>` (the first commit in this PR) has the largest amount of fallout out of all the changes we might make (because it can require removing the `'a` parameter of `struct`s containing `tcx: TyCtxt<'a, ...>`), and is the hardest to automate (because `'a` is used everywhere, not just with `TyCtxt`, unlike, say `'gcx, 'tcx` -> `'tcx`).

So I'm submitting this now to get it out of the way and reduce further friction in the future.

**EDIT**: for the `rustfmt` commit, I used rust-lang/rustfmt#1324 (comment), and manually filtered out some noise, like in #61735, but unlike that PR, there was also a weird bug to work around.
It should be reviewed separately, and dropped if unwanted.

cc @rust-lang/compiler r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Jun 12, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 24ddd16 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 12, 2019
@bors bors merged commit 4c98cb6 into rust-lang:master Jun 12, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #61722!

Tested on commit 24ddd16.
Direct link to PR: #61722

💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jun 12, 2019
Tested on commit rust-lang/rust@24ddd16.
Direct link to PR: <rust-lang/rust#61722>

💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).
@eddyb eddyb deleted the vowel-exclusion-zone branch June 12, 2019 16:32
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jun 12, 2019
Fix wrong lifetime of TyCtxt

Rustup rust-lang/rust#61722

changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jun 12, 2019
Fix wrong lifetime of TyCtxt

Rustup rust-lang/rust#61722

changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jun 12, 2019
Fix wrong lifetime of TyCtxt

Rustup rust-lang/rust#61722

changelog: none
@RalfJung RalfJung mentioned this pull request Jun 13, 2019
bors added a commit that referenced this pull request Jun 14, 2019
Unify all uses of 'gcx and 'tcx.

This is made possible by @Zoxc landing #57214 (see #57214 (comment) for the decision).

A bit of context for the approach: just like #61722, this is *not* how I originally intended to go about this, but @Zoxc and my own experimentation independently resulted in the same conclusion:
The interim alias `type TyCx<'tcx> = TyCtxt<'tcx, 'tcx>;` attempt required more work (adding `use`s), even only for handling the `TyCtxt<'tcx, 'tcx>` case and not the general `TyCtxt<'gcx, 'tcx>` one.

What this PR is based on is the realization that `'gcx` is a special-enough name that it can be replaced, without caring for context, with `'tcx`, and then repetitions of the name `'tcx` be compacted away.
After that, only a small number of error categories remained, each category easily dealt with with either more mass replacements (e.g. `TyCtxt<'tcx, '_>` -> `TyCtxt<'tcx>`) or by hand.

For the `rustfmt` commit, I used rust-lang/rustfmt#1324 (comment), and manually filtered out some noise, like in #61735 and #61722, and like the latter, there was also a weird bug to work around.
It should be reviewed separately, and dropped if unwanted (in this PR it's pretty significant).

cc @rust-lang/compiler r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants