Don't ICE on path-collision in dep-graph

[pnkfelix]

Collisions in the dep-graph due to path-reuse are rare but can occur.

So, instead of ICE'ing, just fail to mark green in such cases (for DepKind::{Hir, HirBody, CrateMetadata}).

Fix #62649.

[rust-highfive]

r? @cramertj

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

[pnkfelix]

r? @michaelwoerister

its possible you had some more targeted fix in mind, e.g. trying to tease out what particular edge in the dep-graph construction this was arising from.

In case you want to know more, this gist has a dot-file with the curr and prev dep-graphs that caused this bug to arise: https://gist.github.com/pnkfelix/7e0d820945fbde43011e2690b96cf312

[michaelwoerister]

Thanks for the PR, @pnkfelix! I'll try to get a closer look next week.

[michaelwoerister]

I'm not sure this is a valid fix. IIRC, it's an invariant of the system that we mark all HIR nodes with colors before the query system kicks in. A def-path being re-used should not really change that. I'll try to dig a little deeper.

[michaelwoerister]

@pnkfelix Do you have a small reproduction for the issue somewhere?

[michaelwoerister]

@pnkfelix Do you have a small reproduction for the issue somewhere?

D'oh! ... regression test ...

[michaelwoerister]

Hm, now I understand what's going on: In revision 1 ::Something::foo is a method, i.e. something that has its own DepNode. In revision 2 ::Something::foo is a field, i.e. something that does not have its own DepNode, but does have a valid DefId/DefPath. The check assumes that if something has a DefId it should also have a corresponding Hir or HirBody dep-node, which, as demonstrated here, is not actually the case.

So a more targeted fix would be something like:

if let Some(def_id) = dep_dep_node.extract_def_id(tcx) {
    if is_something_that_corresponds_to_a_dep_node(def_id) {
        // If the node does exist, it should have
        // been marked as either red or green
        bug!(
            "DepNode {:?} should have been \
              pre-marked as red or green but wasn't.",
            dep_dep_node
        )
    } else {
        // This is something that has a valid DefPath 
        // but does not have a corresponding `DepNode`,
        // e.g. a struct field. This branch is hit if
        // a proper item with the given DefPath existed
        // in the previous compilation session.
        return None;
    }
} else {
    // If the node does not exist anymore, we
    // just fail to mark green.
    return None;
}

The tricky part is coming up with a solid implementation for is_something_that_corresponds_to_a_dep_node(). I'm not sure if that is worth the trouble...

[michaelwoerister]

Here is an option for implementing the above check:

fn def_id_corresponds_to_hir_dep_node(tcx: TyCtxt<'_>, def_id: DefId) -> bool {
    let hir_id = tcx.hir.as_local_hir_id(def_id).unwrap();
    match tcx.hir.get(hir_id) {
        Node::Item(_) |
        Node::TraitItem(_) |
        Node::ImplItem(_) |
        Node::Crate => true,

        Node::Param(_) |
        Node::ForeignItem(_) |
        Node::Variant(_) |
        Node::Field(_) |
        Node::AnonConst(_|
        Node::Expr(_) |
        Node::Stmt(_) |
        Node::PathSegment(_) |
        Node::Ty(_) |
        Node::TraitRef(_) |
        Node::Binding(_) |
        Node::Pat(_) |
        Node::Arm(_) |
        Node::Block(_) |
        Node::Local(_) |
        Node::MacroDef(_) |
        Node::Ctor(_) |
        Node::Lifetime(_|
        Node::GenericParam(_) |
        Node::Visibility(_) => false
    }
}

Hopefully the whole notion of pre-allocating HIR dep-nodes will go away at some point.

[michaelwoerister]

Here's an improved, less brittle option:

fn def_id_corresponds_to_hir_dep_node(tcx: TyCtxt<'_>, def_id: DefId) -> bool {
    let hir_id = tcx.hir.as_local_hir_id(def_id).unwrap();
    def_id.index == hir_id.owner
}

[pnkfelix]

@michaelwoerister I'll look into using your revised approach

[pnkfelix]

@michaelwoerister okay I've incorporated your proposal into this version.

[michaelwoerister]

@bors r+ rollup

Thanks, @pnkfelix! Let's merge it like this. If other incremental compilation errors keep cropping up, we can still think about printing a suggestion about clearing the cache to the user.

[bors]

📌 Commit 6f4b904 has been approved by michaelwoerister