Skip to content

Comments

HirIdification: add key HirId methods#58090

Merged
bors merged 3 commits intorust-lang:masterfrom
ljedrz:HirIdification_phase_2
Feb 4, 2019
Merged

HirIdification: add key HirId methods#58090
bors merged 3 commits intorust-lang:masterfrom
ljedrz:HirIdification_phase_2

Conversation

@ljedrz
Copy link
Contributor

@ljedrz ljedrz commented Feb 3, 2019 •
edited
Loading

This is another PR in a series dedicated to HirId-ification, i.e. deprecating ast::NodeIds after the AST > HIR lowering process. The bigger proof of concept can be seen in #57578.

Phase 2: add key HirId methods mirroring the NodeId ones.

These should be counterparts of the most widely used Hir methods using NodeIds. Note that this expands hir::map::Definitions with an additional hir_to_def_index map (with the intention of later removing node_to_def_index).

As a bonus there is also a small cleanup commit removing unnecessary calls to node_to_hir_id where HirId is already available.

r? @Zoxc
Cc @varkor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 3, 2019
@ljedrz ljedrz force-pushed the HirIdification_phase_2 branch from 10fb15c to 3c0628f Compare February 3, 2019 09:28
@rust-highfive
Copy link
Contributor

rust-highfive commented Feb 3, 2019

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:0d6ef74a:start=1549186175652682521,finish=1549186176522873573,duration=870191052
$ 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
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:24:50] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:24:50] 
[00:24:50] note: rustc 1.34.0-dev running on x86_64-unknown-linux-gnu
[00:24:50] 
[00:24:50] note: compiler flags: -Z force-unstable-if-unmarked -C opt-level=2 -C prefer-dynamic -C debug-assertions=y -C codegen-units=1 -C link-args=-Wl,-rpath,$ORIGIN/../lib --crate-type lib
[00:24:50] note: some of the compiler flags provided by cargo are hidden
[00:24:50] 
[00:24:50] error: Could not compile `core`.
[00:24:50] warning: build failed, waiting for other jobs to finish...
[00:24:50] warning: build failed, waiting for other jobs to finish...
[00:24:55] error: build failed
[00:24:55] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:24:55] expected success, got: exit code: 101
[00:24:55] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:24:55] Build completed unsuccessfully in 0:20:43
[00:24:55] make: *** [all] Error 1
[00:24:55] Makefile:18: recipe for target 'all' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:071d8872
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Feb  3 09:54:43 UTC 2019
---
travis_time:end:126d2ae0:start=1549187683763573155,finish=1549187683768337482,duration=4764327
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:05a57740
$ 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:0338c1a8
travis_time:start:0338c1a8
$ 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:035ffb1d
$ 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
Copy link
Contributor Author

ljedrz commented Feb 3, 2019 •
edited
Loading

The error was fixed.

@ljedrz ljedrz force-pushed the HirIdification_phase_2 branch 2 times, most recently from 55227f8 to 54d937b Compare February 3, 2019 18:00
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
Zoxc
Zoxc reviewed Feb 3, 2019
View reviewed changes
@ljedrz ljedrz force-pushed the HirIdification_phase_2 branch from 54d937b to e8aeb83 Compare February 3, 2019 19:37
@ljedrz
Copy link
Contributor Author

ljedrz commented Feb 3, 2019

@Zoxc thanks for the thorough review! I guess I was a bit overzealous with moving away from NodeIds ^^.

Comments addressed.

@ljedrz
Copy link
Contributor Author

ljedrz commented Feb 3, 2019

@Zoxc done; if everything is ok now, you can delegate the PR to me so I can later squash the correctional commit and approve it on your behalf afterwards.

@Zoxc
Copy link
Contributor

Zoxc commented Feb 3, 2019

@bors delegate+

@bors
Copy link
Collaborator

bors commented Feb 3, 2019

✌️ @ljedrz can now approve this pull request

@Zoxc
Copy link
Contributor

Zoxc commented Feb 4, 2019

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 4, 2019

📌 Commit 272f4df 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 Feb 4, 2019
@bors
Copy link
Collaborator

bors commented Feb 4, 2019

⌛ Testing commit 272f4df with merge 4314dba...

bors added a commit that referenced this pull request Feb 4, 2019
@bors
Auto merge of #58090 - ljedrz:HirIdification_phase_2, r=Zoxc
4314dba 
HirIdification: add key HirId methods

This is another PR in a series dedicated to `HirId`-ification, i.e. deprecating `ast::NodeId`s after the AST > HIR lowering process. The bigger proof of concept can be seen in #57578.

**Phase 2**: add key `HirId` methods mirroring the `NodeId` ones.

These should be counterparts of the most widely used `Hir` methods using `NodeId`s. Note that this expands `hir::map::Definitions` with an additional `hir_to_def_index` map (with the intention of later removing `node_to_def_index`).

As a bonus there is also a small cleanup commit removing unnecessary calls to `node_to_hir_id` where `HirId` is already available.

r? @Zoxc
Cc @varkor
@bors
Copy link
Collaborator

bors commented Feb 4, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: Zoxc
Pushing 4314dba to master...

@bors bors merged commit 272f4df into rust-lang:master Feb 4, 2019
@rust-highfive
Copy link
Contributor

rust-highfive commented Feb 4, 2019

📣 Toolstate changed by #58090!

Tested on commit 4314dba.
Direct link to PR: #58090

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

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Feb 4, 2019
@rust-highfive
📣 Toolstate changed by rust-lang/rust#58090!
d8d018c 
Tested on commit rust-lang/rust@4314dba.
Direct link to PR: <rust-lang/rust#58090>

💔 rls on linux: test-pass → test-fail (cc @nrc @Xanewok, @rust-lang/infra).
@ljedrz ljedrz deleted the HirIdification_phase_2 branch February 4, 2019 06:05
@ljedrz
Copy link
Contributor Author

ljedrz commented Feb 4, 2019

@Zoxc sorry for not having squashed in time, I had to get some sleep. Hopefully the git historians won't mind ^^.

I'll start working on switching the functions now.

eddyb
eddyb reviewed Feb 4, 2019
View reviewed changes
src/librustc/ty/item_path.rs
}

// FIXME(@ljedrz): replace the NodeId variant
pub fn hir_path_str(self, id: hir::HirId) -> String {
Copy link
Member

@eddyb eddyb Feb 4, 2019

Choose a reason for hiding this comment

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

Please do not use this - #57967 (or the refactoring that it's based on, rather, will split it into a separate PR) removes node_path_str.

If you really need to replace node_path_str uses, you can call item_path_str directly (a lot of the time you might even have a DefId that was being converted to a NodeId!).

Copy link
Contributor Author

@ljedrz ljedrz Feb 4, 2019

Choose a reason for hiding this comment

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

@eddyb thanks for the heads up, I'll keep that in mind; I'll also remove this function with a drive-by commit.

Copy link
Member

@eddyb eddyb Feb 4, 2019

Choose a reason for hiding this comment

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

I opened #58140 in the meanwhile, and it removes hir_path_str, so it's compatible with any PR also removing hir_path_str.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Zoxc Zoxc Zoxc left review comments

@eddyb eddyb eddyb left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Zoxc Zoxc

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ljedrz @rust-highfive @Zoxc @bors @eddyb