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: disallow cloning HIR nodes. #61968

Merged
merged 2 commits into from
Jun 20, 2019
Merged

rustc: disallow cloning HIR nodes. #61968

merged 2 commits into from
Jun 20, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jun 19, 2019

Besides being inefficient, cloning also risks creating broken HIR (without properly recreating all the IDs and whatnot, in which case you might as well reconstruct the entire node without ever Clone-ing anything).

We detect some detrimental situations (based on the occurrence of HirIds, I believe?), but it's better to statically disallow it, IMO.

One of the examples that is fixed by this PR is tcx.hir().fn_decl{,_by_hir_id}, which was cloning an entire hir::FnDecl every single time it was called.

r? @petrochenkov cc @rust-lang/compiler

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2019
@eddyb
Copy link
Member Author

eddyb commented Jun 19, 2019

@bors try (for perf)

@bors
Copy link
Contributor

bors commented Jun 19, 2019

⌛ Trying commit 673c3fc with merge 748a202...

bors added a commit that referenced this pull request Jun 19, 2019
rustc: disallow cloning HIR nodes.

Besides being inefficient, cloning also risks creating broken HIR (without properly recreating all the IDs and whatnot, in which case you might as well reconstruct the entire node without ever `Clone`-ing anything).

We detect *some* detrimental situations (based on the occurrence of `HirId`s, I believe?), but it's better to statically disallow it, IMO.

One of the examples that is fixed by this PR is `tcx.hir().fn_decl{,_by_hir_id}`, which was cloning an entire `hir::FnDecl` *every single time it was called*.

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

bors commented Jun 19, 2019

☀️ Try build successful - checks-travis
Build commit: 748a202

@petrochenkov
Copy link
Contributor

@rust-timer build 748a202

@rust-timer
Copy link
Collaborator

Success: Queued 748a202 with parent e79b2a1, comparison URL.

@petrochenkov petrochenkov added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 19, 2019
@petrochenkov
Copy link
Contributor

Nice.

@petrochenkov
Copy link
Contributor

r=me when the perf run is complete

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 748a202, comparison URL.

@cramertj
Copy link
Member

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jun 20, 2019

📌 Commit 673c3fc has been approved by petrochenkov

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 20, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 20, 2019
rustc: disallow cloning HIR nodes.

Besides being inefficient, cloning also risks creating broken HIR (without properly recreating all the IDs and whatnot, in which case you might as well reconstruct the entire node without ever `Clone`-ing anything).

We detect *some* detrimental situations (based on the occurrence of `HirId`s, I believe?), but it's better to statically disallow it, IMO.

One of the examples that is fixed by this PR is `tcx.hir().fn_decl{,_by_hir_id}`, which was cloning an entire `hir::FnDecl` *every single time it was called*.

r? @petrochenkov cc @rust-lang/compiler
bors added a commit that referenced this pull request Jun 20, 2019
Rollup of 4 pull requests

Successful merges:

 - #60454 (Add custom nth_back to Skip)
 - #60772 (Implement nth_back for slice::{Iter, IterMut})
 - #61782 (suggest tuple struct syntax)
 - #61968 (rustc: disallow cloning HIR nodes.)

Failed merges:

r? @ghost
@eddyb
Copy link
Member Author

eddyb commented Jun 20, 2019

Uhhh I thought @Zoxc had concerns regarding this, but now it's already in a rollup.

@bors
Copy link
Contributor

bors commented Jun 20, 2019

⌛ Testing commit 673c3fc with merge 4fb77a0...

@bors bors merged commit 673c3fc into rust-lang:master Jun 20, 2019
@eddyb eddyb deleted the hir-noclone branch June 20, 2019 10:42
@cramertj
Copy link
Member

Apologies-- I missed that there was discussion happening about this elsewhere.

bors added a commit to rust-lang/rust-clippy that referenced this pull request Jun 21, 2019
Fix breakage due to rust-lang/rust#61968

<!--
Thank you for making Clippy better!

We're collecting our changelog from pull request descriptions.
If your PR only updates to the latest nightly, you can leave the
`changelog` entry as `none`. Otherwise, please write a short comment
explaining your change.

If your PR fixes an issue, you can add "fixes #issue_number" into this
PR description. This way the issue will be automatically closed when
your PR is merged.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

- [ ] Followed [lint naming conventions][lint_naming]
- [ ] Added passing UI tests (including committed `.stderr` file)
- [ ] `cargo test` passes locally
- [ ] Executed `util/dev update_lints`
- [ ] Added lint documentation
- [ ] Run `cargo fmt`

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR -->

changelog: none
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2019
rustc: use a separate copy of P for HIR than for AST.

Note: this currently includes/is based on top of rust-lang#61987.

Like rust-lang#61968, but goes one step further and uses a separate `P<...>` for the HIR, with no `Clone`, or the ability to mutate after allocation.
There is still `into_inner`/`into_iter`, but they're only exposed for `hir::lowering`, and they would take more work to untangle.

r? @petrochenkov cc @rust-lang/compiler
bors added a commit that referenced this pull request Jul 3, 2019
rustc: use a separate copy of P for HIR than for AST.

Note: this currently includes/is based on top of #61987.

Like #61968, but goes one step further and uses a separate `P<...>` for the HIR, with no `Clone`, or the ability to mutate after allocation.
There is still `into_inner`/`into_iter`, but they're only exposed for `hir::lowering`, and they would take more work to untangle.

r? @petrochenkov cc @rust-lang/compiler
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. S-waiting-on-perf Status: Waiting on a perf run to be completed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants