Remove Ord from chalk_ir::interner::DefId

[pierwill]

This change is required for rust-lang/rust#90749.

For background on the initiative of removing ordering traits from items like DefId in rustc, see rust-lang/rust#90317.

[pierwill]

@rustbot assign @jackh726 cc @nikomatsakis

[jackh726]

Interesting...I forgot that auto traits are sorted in chalk-integration, where the DefId is Ord. I can't think of any other place that needs Ord..

[rustbot]

Error: The feature relabel is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please let @rust-lang/release know if you're having trouble with this bot.

[jackh726]

If we didn't add the nodes before, why do we need to add them now? update_edge doesn't do anything different versus add_edge with respect to nodes.

[pierwill]

The nodes must be added because petgraph::graphmap::GraphMap::add_edge can take a generic type N (that must implement Ord), where petgraph::graph::Graph::update_edge takes a NodeIndex.

We need to use Graph since this PR removes Ord from ImplId.

[jackh726]

Ah, I was looking at Graph::add_edge.

Can this create duplicate nodes?

[pierwill]

petgraph::graph::Graph::add_node does create multiple nodes when given the same value. (Playground example.)

[pierwill]

I'm going to see if I can handle this logic with a match statement:

let node_impls: Vec<ImplId<_>> = forest.raw_nodes().iter().map(|x| x.weight).collect();
self.visit_specializations_of_trait(|less_special, more_special| {
    match (
        node_impls.contains(&less_special),
        node_impls.contains(&more_special),
    ) {
        (true, true) => todo!(),
        (true, false) => todo!(),
        (false, true) => todo!(),
        (false, false) => todo!(),
    }
}

[jackh726]

I see "Add or update an edge from a to b." - So it's basically add_edge without allowing duplicates.

[jackh726]

Seems like it might make sense to keep around a Map<ImplId, NodeIndex>

[jackh726]

Before visit_specialization_of_trait, forest will be empty...

[pierwill]

I'm trying to reason this out. The logic below seems right, but I'm not sure how to get the indices for existing nodes...

// Find all specializations. (This is implemented in coherence/solve.)
// Record them in the forest by adding an edge from the less special
// to the more special.
self.visit_specializations_of_trait(|less_special, more_special| {
    match (
        node_impls.contains(&less_special),
        node_impls.contains(&more_special),
    ) {
        (true, true) => {
            // but how do we get indices for l and m?
            forest.add_edge(l, m, ());
        }
        (true, false) => {
            let m = forest.add_node(more_special);
            // but how do we get index for l?
            forest.add_edge(l, m, ());
        }
        (false, true) => {
            let l = forest.add_node(less_special);
            // but how do we get index for m?
            forest.add_edge(l, m, ());
        }
        (false, false) => {
            // add L and M
            let l = forest.add_node(less_special);
            let m = forest.add_node(more_special);
            forest.add_edge(l, m, ());
        }
    }
})?;

Can this even be done correctly without GraphMap? 😬

Update: Ah. node_indices might work...

[jackh726]

I'm thinking something like

let mut forest = DiGraph::new();
let mut node_map = FxHashMap::new();
self.visit_specializations_of_trait(|less_special, more_special| {
    let less_special_node = *node_map.entry(less_special).or_insert_with(|| forest.add_node(less_special));
    let more_special_node = *node_map.entry(more_special).or_insert_with(|| forest.add_node(more_special));
    forest.update_edge(less_special_noe, more_special_node, ());
}

[jackh726]

Nits but LGTM otherwise

[jackh726]

@bors r+

[bors]

📌 Commit 29c1d17 has been approved by jackh726

[bors]

⌛ Testing commit 29c1d17 with merge f518c7c...

[bors]

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing f518c7c to master...