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

Unify all uses of 'gcx and 'tcx. #61817

Merged
merged 2 commits into from
Jun 14, 2019
Merged

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jun 13, 2019

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 uses), 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

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

Centril commented Jun 13, 2019

@bors treeclosed=500

@Centril
Copy link
Contributor

Centril commented Jun 13, 2019

@bors treeclosed-

That was a bit premature 🤣

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@eddyb
Copy link
Member Author

eddyb commented Jun 14, 2019

Review has started, let's get the ball rolling: @bors treeclosed=500

@eddyb
Copy link
Member Author

eddyb commented Jun 14, 2019

@bors p=700 (oops, forgot to also set this)

@mark-i-m
Copy link
Member

@eddyb When this whole transition is over, could you please update the rustc-guide (or tell me how to)?

@eddyb
Copy link
Member Author

eddyb commented Jun 14, 2019

@mark-i-m Definitely! Not sure if I can do it today, sadly. I guess I could open the PR now and someone cam merge it once this PR is merged.

@oli-obk Oops! There were two instances of that, probably a VSCode stale file during mass-replacement (those changes look like something I did on a different branch but they never made it into a PR).
I've force-pushed with them removed.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 14, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Jun 14, 2019

📌 Commit afc39bb 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 14, 2019
@bors
Copy link
Contributor

bors commented Jun 14, 2019

⌛ Testing commit afc39bb with merge 9606f6f...

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
-> DepNode
where 'gcx: 'a + 'tcx,
where 'tcx: 'a,
'tcx: 'a
Copy link
Member

Choose a reason for hiding this comment

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

There's still some redundancy here, do you plan to clean up later?

Copy link
Contributor

Choose a reason for hiding this comment

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

there are a few others, they will often be detected by various lints that are still in the process of getting fixed so they have no false positives

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah (although everyone is free to try, there really isn't that much).
I only removed the cases where it was causing an error, I guess my mistake was matching on the exact number of spaces instead of \s+.

Copy link
Member Author

Choose a reason for hiding this comment

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

In general we should start removing lifetime bounds except in the rare cases when they're needed.

@ehuss ehuss mentioned this pull request Jun 14, 2019
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jun 14, 2019
Fix wrong lifetime of TyCtxt

Rustup rust-lang/rust#61817

changelog: none
@bors
Copy link
Contributor

bors commented Jun 14, 2019

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

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

📣 Toolstate changed by #61817!

Tested on commit 9606f6f.
Direct link to PR: #61817

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 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 14, 2019
Tested on commit rust-lang/rust@9606f6f.
Direct link to PR: <rust-lang/rust#61817>

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 miri on windows: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).
@eddyb eddyb deleted the begone-gcx-attempt-2 branch June 14, 2019 19:09
@matthewjasper
Copy link
Contributor

@bors treeclosed-

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

9 participants