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

Almost fully deprecate hir::map::Map.hir_to_node_id #62975

wants to merge 3 commits into
base: master


Copy link

commented Jul 25, 2019

  • HirIdify
  • HirIdify hir::Crate.modules
  • introduce a HirId to DefIndex map in map::Definitions

The only last uses of hir::map::Map.hir_to_node_id in the compiler are:

  • for the purposes of driver::pretty (in map::nodes_matching_suffix), but I don't know if we can remove NodeIds in there (I think when I attempted it previously there was some issue due to HirId not being representable with an integer)
  • in ty::query::on_disk_cache (not sure about the purpose of this one)
  • in mir::transform::check_unsafety (only important for error message order)

Any suggestions how to kill these off?

r? @Zoxc


This comment was marked as resolved.

Copy link

commented Jul 25, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (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.
2019-07-25T14:27:18.6721709Z ##[command]git remote add origin
2019-07-25T14:27:18.6935507Z ##[command]git config 0
2019-07-25T14:27:18.7000849Z ##[command]git config --get-all http.
2019-07-25T14:27:18.7058701Z ##[command]git config --get-all http.proxy
2019-07-25T14:27:18.7205447Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/62975/merge:refs/remotes/pull/62975/merge
2019-07-25T14:27:53.3248713Z do so (now or later) by using -b with the checkout command again. Example:
2019-07-25T14:27:53.3249123Z   git checkout -b <new-branch-name>
2019-07-25T14:27:53.3249407Z HEAD is now at 3856514b7 Merge 0baccc7e7593dac83b126675a561380231a74950 into eedf6ce4ef54bb03818ab21d714f1b9f13a6b31c
2019-07-25T14:27:53.3390766Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-07-25T14:27:53.3395372Z ==============================================================================
2019-07-25T14:27:53.3395434Z Task         : Bash
2019-07-25T14:27:53.3395501Z Description  : Run a Bash script on macOS, Linux, or Windows
2019-07-25T14:34:24.5598132Z    Compiling serde_json v1.0.40
2019-07-25T14:34:29.1645400Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-07-25T14:34:38.0934284Z     Finished release [optimized] target(s) in 1m 33s
2019-07-25T14:34:38.1005974Z tidy check
2019-07-25T14:34:38.4701473Z tidy error: /checkout/src/librustc/hir/ line longer than 100 chars
2019-07-25T14:34:40.1821045Z some tidy checks failed
2019-07-25T14:34:40.1825556Z 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"
2019-07-25T14:34:40.1840117Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-07-25T14:34:40.1840208Z Build completed unsuccessfully in 0:01:36
2019-07-25T14:34:40.1840208Z Build completed unsuccessfully in 0:01:36
2019-07-25T14:34:41.5915909Z ##[error]Bash exited with code '1'.
2019-07-25T14:34:41.5947113Z ##[section]Starting: Checkout
2019-07-25T14:34:41.5948783Z ==============================================================================
2019-07-25T14:34:41.5948830Z Task         : Get sources
2019-07-25T14:34:41.5948869Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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 ljedrz:kill_off_hir_to_node_id branch from 0baccc7 to b9f2e81 Jul 25, 2019


This comment has been minimized.

Copy link

commented Aug 2, 2019

Ping from triage. @Zoxc any updates on this?

Show resolved Hide resolved src/librustc/hir/ Outdated

@ljedrz ljedrz force-pushed the ljedrz:kill_off_hir_to_node_id branch from b9f2e81 to f470005 Aug 5, 2019


This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@Zoxc done 👍


This comment has been minimized.

Copy link

commented Aug 10, 2019

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

pub hir_to_def_index: HirIdMap<DefIndex>,
/// `DefIndex`es created by DefCollector::create_def; used only to complete the hir_to_def_index
/// mapping after the lowering
defs_awaiting_hir_id: NodeMap<DefIndex>,

This comment has been minimized.

Copy link

eddyb Aug 11, 2019


This is another reason why I would prefer if everything with a DefId was a HirId root: you wouldn't need any mappings, a HIR node with a DefId would have a HirId of (def_id, 0).
cc @michaelwoerister @nikomatsakis @petrochenkov

This comment has been minimized.

Copy link

michaelwoerister Aug 12, 2019


IIRC, we wanted HirId owners to correspond to item-likes mainly because that's the incremental compilation granularity we were shooting for.

This comment has been minimized.

Copy link

eddyb Aug 12, 2019


I see, but that seems like it's introducing artificial complexity (with perhaps the exception of TypeckTables - but even in that case, we could just have separate TypeckTables for closures, that are kept in a map in the parent body's TypeckTables).

Bonus: right now things like embedded constants have their own TypeckTables but share a HirId space with their enclosing item, meaning their intra-item IDs are not in a nice 0..n range (if they were, we could maybe use something more efficient than hashmaps).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
8 participants
You can’t perform that action at this time.