HirIdify hir::ItemId

[ljedrz]

Pushing the limits of HirIdification (#57578).

Replaces NodeId with HirId in hir::ItemId. It currently fails during a debug_assert in https://github.com/rust-lang/rust/blob/master/src/librustc/ich/impls_hir.rs#L793.

r? @Zoxc

[rust-highfive]

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:064184fe:start=1552294150705271146,finish=1552294151637714759,duration=932443613
$ 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
Setting environment variables from .travis.yml
---
[00:27:16]    Compiling cmake v0.1.33
[00:27:16]    Compiling backtrace-sys v0.1.27
[00:27:17] thread 'rustc' panicked at 'assertion failed: `(left == right)`
[00:27:17]   left: `1`,
[00:27:17]  right: `0`', src/librustc/ich/impls_hir.rs:793:17
[00:27:17] 
[00:27:17] error: internal compiler error: unexpected panic
[00:27:17] 
[00:27:17] note: the compiler unexpectedly panicked. this is a bug.
[00:27:17] note: the compiler unexpectedly panicked. this is a bug.
[00:27:17] 
[00:27:17] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:27:17] 
[00:27:17] note: rustc 1.35.0-dev running on x86_64-unknown-linux-gnu
[00:27:17] 
[00:27:17] note: compiler flags: -Z external-macro-backtrace -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:27:17] note: some of the compiler flags provided by cargo are hidden
[00:27:17] 
[00:27:17] error: Could not compile `core`.
[00:27:17] warning: build failed, waiting for other jobs to finish...
---
travis_time:end:25d8657e:start=1552295802389197042,finish=1552295802394137888,duration=4940846
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:2d36c9c8
$ 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:1286d496
travis_time:start:1286d496
$ 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:0362ed08
$ 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]

Yep, that's the one; I thought I could adjust it to compare vs. 1 on the right hand side, but then it fails with 0 != 1 🤔.

[Zoxc]

Maybe you could try asserting that the local id of the item is 0 here?

You could also try printing out information about the item which isn't 0.

[ljedrz]

Got one:

non-zero local id in Item {
    ident: TryFrom#0, 
    hir_id: HirId { owner: DefIndex(0:147), local_id: 1 }, 
    attrs: [], 
    node: Use(path(convert::TryFrom), Single), 
    vis: Spanned { node: Inherited, span: Span { lo: BytePos(73297), hi: BytePos(73297), ctxt: #0 } }, 
    span: Span { lo: BytePos(73311), hi: BytePos(73318), ctxt: #0 }
}

[Zoxc]

This is likely an error in lowering.rs where we call lower_node_id in the wrong location, so our owner is wrong. Items should have 0 as their local index.

[ljedrz]

Are you sure all the items should have a local_id equal to 0? Shouldn't just ItemKind::Mod adhere to this rule? Like in the assertion I hit under insert_item, that was an ItemKind::Use and there could be multiple of those under one module.

[Zoxc]

I replaced this with:

        node_ids.into_iter().map(|node_id| hir::ItemId {
            id: self.lower_node_id_generic(node_id, |_| {
                panic!("expected node_id to be lowered already {:#?}", i)
            }).hir_id
        }).collect()

That panics and shows the the following item wasn't assigned a HirId:

expected node_id to be lowered already Item {
    ident: #0,
    attrs: [],
    id: NodeId(2249),
    node: Use(
        UseTree {
            prefix: path(convert),
            kind: Nested(
                [
                    (
                        UseTree {
                            prefix: path(TryFrom),
                            kind: Simple(
                                None,
                                NodeId(2252),
                                NodeId(2253)
                            ),
                        },
                        NodeId(2254)
                    ),
                    (
                        UseTree {
                            prefix: path(Infallible),
                            kind: Simple(
                                None,
                                NodeId(2256),
                                NodeId(2257)
                            ),
                        },
                        NodeId(2258)
                    )
                ]
            ),
        }
    ),
    vis: Spanned {
        node: Inherited,
    },
}

I guess this means that either too many NodeIds are in this list or some id is not being correctly lowered to a HirId.

@oli-obk Do you have any ideas?

[oli-obk]

These node ids haven't been lowered yet at all. lower_item_id is called before the item is lowered in order to obtain any sub-item's ids. If you want to error if a second lowering fails, you'll need to do that where the actual items are lowered (so after lower_item_id is called).

[Zoxc]

I see that only AST items had their ids lowered in MiscCollector. I changed that to also include the use trees in #59413.

[bors]

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

[Centril]

@Zoxc is this done now? should this be closed?

[ljedrz]

Yep, this was a part of the merged PR.