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

The (almost) culmination of HirIdification #62168

Merged
merged 7 commits into from
Jul 5, 2019

Conversation

ljedrz
Copy link
Contributor

@ljedrz ljedrz commented Jun 27, 2019

It's finally over.

This PR removes old FIXMEs and renames some functions so that the HirId variant has the shorter name.
All that remains (and rightfully so) is stuff in resolve, save_analysis and (as far as I can tell) in a few places where we can't replace NodeId with HirId.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

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

Centril commented Jun 27, 2019

r? @Zoxc

@rust-highfive rust-highfive assigned Zoxc and unassigned eddyb Jun 27, 2019
@Zoxc
Copy link
Contributor

Zoxc commented Jun 27, 2019

I think there a few more things to do:

  • Rename local_def_id to local_def_id_from_node_id and then local_def_id_from_hir_id to local_def_id. Similar renames should be done for the other functions.
  • Skip generating the HIR id to node id map in normal compilation, only computing that for RLS and rustdoc.

@ljedrz
Copy link
Contributor Author

ljedrz commented Jun 27, 2019

I could add the name cleanups to this PR; I'll look into map generation when I have a bit more time.

src/librustc_driver/pretty.rs Outdated Show resolved Hide resolved
@ljedrz ljedrz force-pushed the the_culmination_of_hiridification branch from 9e2e0ca to 6c473c9 Compare June 27, 2019 10:07
@ljedrz
Copy link
Contributor Author

ljedrz commented Jun 27, 2019

@Zoxc comment addressed; since a fair number of files were altered, I'd leave the adjustment to the HidId > NodeId map generation to a follow up PR.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:176fa528:start=1561630168025888517,finish=1561630170306347822,duration=2280459305
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---

[00:04:55] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:56] tidy error: /checkout/src/librustc_interface/passes.rs:904: line longer than 100 chars
[00:04:56] tidy error: /checkout/src/librustdoc/clean/mod.rs:2990: line longer than 100 chars
[00:04:57] some tidy checks failed
[00:04:57] 
[00:04:57] 
[00:04:57] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
[00:04:57] 
[00:04:57] 
[00:04:57] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:57] Build completed unsuccessfully in 0:01:13
---
travis_time:end:087a0dc0:start=1561630478938679118,finish=1561630478943498661,duration=4819543
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00cab030
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1318a127
travis_time:start:1318a127
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:19d5f7be
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@ljedrz ljedrz force-pushed the the_culmination_of_hiridification branch from 6c473c9 to 0e1e1c2 Compare June 27, 2019 10:48
@ljedrz
Copy link
Contributor Author

ljedrz commented Jun 27, 2019

I actually already had the tidy errors fixed, but forgot to git add them ^^.

@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 2, 2019

@Zoxc: can we merge this one? It might take me a while to fully kill off hir_to_node_id (as it is still used in some spots I thought were NodeId bound) and I wouldn't like these bits to rot.

#[inline]
pub fn opt_local_def_id_from_hir_id(&self, hir_id: HirId) -> Option<DefId> {
pub fn opt_local_def_id(&self, hir_id: HirId) -> Option<DefId> {
let node_id = self.hir_to_node_id(hir_id);
self.definitions.opt_local_def_id(node_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this stil uses node ids too. Should add it to the todo list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already did 👍.

@@ -1500,7 +1500,7 @@ pub fn check_crate<'tcx, T: for<'a> LateLintPass<'a, 'tcx>>(
time(tcx.sess, "module lints", || {
// Run per-module lints
par_iter(&tcx.hir().krate().modules).for_each(|(&module, _)| {
tcx.ensure().lint_mod(tcx.hir().local_def_id(module));
tcx.ensure().lint_mod(tcx.hir().local_def_id_from_node_id(module));
Copy link
Contributor

Choose a reason for hiding this comment

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

The Crate.modules field should use a HirId. Add that to the todo list too

@ljedrz ljedrz changed the title The culmination of HirIdification The (almost) culmination of HirIdification Jul 2, 2019
@Zoxc
Copy link
Contributor

Zoxc commented Jul 2, 2019

@bors r+

@Zoxc Zoxc closed this Jul 2, 2019
@Zoxc Zoxc reopened this Jul 2, 2019
@Zoxc
Copy link
Contributor

Zoxc commented Jul 2, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Jul 2, 2019

📌 Commit ba54e768db1b09982f5543f2bb37c4a80ad0d149 has been approved by Zoxc

@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 Jul 2, 2019
@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 2, 2019

@Zoxc: any hints on how I can get from HirId to DefId without an intermediate NodeId step? Do I need to introduce some new collection populated hir_to_node_id style in Map or Definitions?

@Zoxc
Copy link
Contributor

Zoxc commented Jul 2, 2019

@ljedrz You probably need some map in Definitions yes. It might be non-trivial if Definitions lives through HIR lowering. I'd probably start with the easier things.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 3, 2019
@bors
Copy link
Contributor

bors commented Jul 4, 2019

☔ The latest upstream changes (presumably #62355) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 4, 2019
@ljedrz ljedrz force-pushed the the_culmination_of_hiridification branch from dbcb919 to a6030ff Compare July 4, 2019 10:55
@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 4, 2019

@bors r=Zoxc

@bors
Copy link
Contributor

bors commented Jul 4, 2019

📌 Commit a6030ff has been approved by Zoxc

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 4, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019
…ation, r=Zoxc

The (almost) culmination of HirIdification

It's finally over.

This PR removes old `FIXME`s and renames some functions so that the `HirId` variant has the shorter name.
All that remains (and rightfully so) is stuff in `resolve`, `save_analysis` and (as far as I can tell) in a few places where we can't replace `NodeId` with `HirId`.
Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019
…ation, r=Zoxc

The (almost) culmination of HirIdification

It's finally over.

This PR removes old `FIXME`s and renames some functions so that the `HirId` variant has the shorter name.
All that remains (and rightfully so) is stuff in `resolve`, `save_analysis` and (as far as I can tell) in a few places where we can't replace `NodeId` with `HirId`.
Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019
…ation, r=Zoxc

The (almost) culmination of HirIdification

It's finally over.

This PR removes old `FIXME`s and renames some functions so that the `HirId` variant has the shorter name.
All that remains (and rightfully so) is stuff in `resolve`, `save_analysis` and (as far as I can tell) in a few places where we can't replace `NodeId` with `HirId`.
bors added a commit that referenced this pull request Jul 5, 2019
Rollup of 13 pull requests

Successful merges:

 - #61545 (Implement another internal lints)
 - #62110 (Improve -Ztime-passes)
 - #62133 (Feature gate `rustc` attributes harder)
 - #62158 (Add MemoryExtra in InterpretCx constructor params)
 - #62168 (The (almost) culmination of HirIdification)
 - #62193 (Create async version of the dynamic-drop test)
 - #62369 (Remove `compile-pass` from compiletest)
 - #62380 (rustc_target: avoid negative register counts in the SysV x86_64 ABI.)
 - #62381 (Fix a typo in Write::write_vectored docs)
 - #62390 (Update README.md)
 - #62396 (remove Scalar::is_null_ptr)
 - #62406 (Lint on invalid values passed to x.py --warnings)
 - #62414 (Remove last use of mem::uninitialized in SGX)

Failed merges:

r? @ghost
@bors bors merged commit a6030ff into rust-lang:master Jul 5, 2019
@ljedrz ljedrz deleted the the_culmination_of_hiridification branch July 6, 2019 00:45
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jul 6, 2019
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants