Auto merge of rust-lang#79721 - Aaron1011:fix/reuse-def-path-hash, r=wesleywiser

Properly re-use def path hash in incremental mode

Fixes rust-lang#79661

In incremental compilation mode, we update a `DefPathHash -> DefId`
mapping every time we create a `DepNode` for a foreign `DefId`.
This mapping is written out to the on-disk incremental cache, and is
read by the next compilation session to allow us to lazily decode
`DefId`s.

When we decode a `DepNode` from the current incremental cache, we need
to ensure that any previously-recorded `DefPathHash -> DefId` mapping
gets recorded in the new mapping that we write out. However, PR rust-lang#74967
didn't do this in all cases, leading to us being unable to decode a
`DefPathHash` in certain circumstances.

This PR refactors some of the code around `DepNode` deserialization to
prevent this kind of mistake from happening again.

Author: bors

compiler/rustc_middle/src/ty/query/mod.rs

@@ -220,7 +220,7 @@ pub(crate) fn try_load_from_on_disk_cache<'tcx>(tcx: TyCtxt<'tcx>, dep_node: &De
                                          .map(|c| c.is_green())
                                          .unwrap_or(false));
 
-                        let key = <query_keys::$name<'tcx> as DepNodeParams<TyCtxt<'_>>>::recover(tcx, dep_node).unwrap();
+                        let key = <query_keys::$name<'tcx> as DepNodeParams<TyCtxt<'_>>>::recover(tcx, dep_node).unwrap_or_else(|| panic!("Failed to recover key for {:?} with hash {}", dep_node, dep_node.hash));
                         if queries::$name::cache_on_disk(tcx, &key, None) {
                             let _ = tcx.$name(key);
                         }

compiler/rustc_query_system/src/dep_graph/dep_node.rs

@@ -53,6 +53,19 @@ use std::hash::Hash;
 #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Encodable, Decodable)]
 pub struct DepNode<K> {
     pub kind: K,
+    // Important - whenever a `DepNode` is constructed, we need to make
+    // sure to register a `DefPathHash -> DefId` mapping if needed.
+    // This is currently done in two places:
+    //
+    // * When a `DepNode::construct` is called, `arg.to_fingerprint()`
+    //   is responsible for calling `OnDiskCache::store_foreign_def_id_hash`
+    //   if needed
+    // * When a `DepNode` is loaded from the `PreviousDepGraph`,
+    //   then `PreviousDepGraph::index_to_node` is responsible for calling
+    //   `tcx.register_reused_dep_path_hash`
+    //
+    // FIXME: Enforce this by preventing manual construction of `DefNode`
+    // (e.g. add a `_priv: ()` field)
     pub hash: PackedFingerprint,
 }
 

compiler/rustc_query_system/src/dep_graph/graph.rs

@@ -7,7 +7,6 @@ use rustc_data_structures::sync::{AtomicU32, AtomicU64, Lock, Lrc, Ordering};
 use rustc_data_structures::unlikely;
 use rustc_errors::Diagnostic;
 use rustc_index::vec::{Idx, IndexVec};
-use rustc_span::def_id::DefPathHash;
 
 use parking_lot::{Condvar, Mutex};
 use smallvec::{smallvec, SmallVec};
@@ -555,7 +554,7 @@ impl<K: DepKind> DepGraph<K> {
         // We never try to mark eval_always nodes as green
         debug_assert!(!dep_node.kind.is_eval_always());
 
-        debug_assert_eq!(data.previous.index_to_node(prev_dep_node_index), *dep_node);
+        data.previous.debug_assert_eq(prev_dep_node_index, *dep_node);
 
         let prev_deps = data.previous.edge_targets_from(prev_dep_node_index);
 
@@ -573,7 +572,7 @@ impl<K: DepKind> DepGraph<K> {
                         "try_mark_previous_green({:?}) --- found dependency {:?} to \
                             be immediately green",
                         dep_node,
-                        data.previous.index_to_node(dep_dep_node_index)
+                        data.previous.debug_dep_node(dep_dep_node_index),
                     );
                     current_deps.push(node_index);
                 }
@@ -586,12 +585,12 @@ impl<K: DepKind> DepGraph<K> {
                         "try_mark_previous_green({:?}) - END - dependency {:?} was \
                             immediately red",
                         dep_node,
-                        data.previous.index_to_node(dep_dep_node_index)
+                        data.previous.debug_dep_node(dep_dep_node_index)
                     );
                     return None;
                 }
                 None => {
-                    let dep_dep_node = &data.previous.index_to_node(dep_dep_node_index);
+                    let dep_dep_node = &data.previous.index_to_node(dep_dep_node_index, tcx);
 
                     // We don't know the state of this dependency. If it isn't
                     // an eval_always node, let's try to mark it green recursively.
@@ -700,18 +699,6 @@ impl<K: DepKind> DepGraph<K> {
             data.current.intern_node(*dep_node, current_deps, fingerprint)
         };
 
-        // We have just loaded a deserialized `DepNode` from the previous
-        // compilation session into the current one. If this was a foreign `DefId`,
-        // then we stored additional information in the incr comp cache when we
-        // initially created its fingerprint (see `DepNodeParams::to_fingerprint`)
-        // We won't be calling `to_fingerprint` again for this `DepNode` (we no longer
-        // have the original value), so we need to copy over this additional information
-        // from the old incremental cache into the new cache that we serialize
-        // and the end of this compilation session.
-        if dep_node.kind.can_reconstruct_query_key() {
-            tcx.register_reused_dep_path_hash(DefPathHash(dep_node.hash.into()));
-        }
-
         // ... emitting any stored diagnostic ...
 
         // FIXME: Store the fact that a node has diagnostics in a bit in the dep graph somewhere
@@ -814,7 +801,7 @@ impl<K: DepKind> DepGraph<K> {
         for prev_index in data.colors.values.indices() {
             match data.colors.get(prev_index) {
                 Some(DepNodeColor::Green(_)) => {
-                    let dep_node = data.previous.index_to_node(prev_index);
+                    let dep_node = data.previous.index_to_node(prev_index, tcx);
                     tcx.try_load_from_on_disk_cache(&dep_node);
                 }
                 None | Some(DepNodeColor::Red) => {

compiler/rustc_query_system/src/dep_graph/prev.rs

@@ -1,7 +1,9 @@
 use super::serialized::{SerializedDepGraph, SerializedDepNodeIndex};
 use super::{DepKind, DepNode};
+use crate::dep_graph::DepContext;
 use rustc_data_structures::fingerprint::Fingerprint;
 use rustc_data_structures::fx::FxHashMap;
+use rustc_span::def_id::DefPathHash;
 
 #[derive(Debug, Encodable, Decodable)]
 pub struct PreviousDepGraph<K: DepKind> {
@@ -31,7 +33,44 @@ impl<K: DepKind> PreviousDepGraph<K> {
     }
 
     #[inline]
-    pub fn index_to_node(&self, dep_node_index: SerializedDepNodeIndex) -> DepNode<K> {
+    pub fn index_to_node<CTX: DepContext<DepKind = K>>(
+        &self,
+        dep_node_index: SerializedDepNodeIndex,
+        tcx: CTX,
+    ) -> DepNode<K> {
+        let dep_node = self.data.nodes[dep_node_index];
+        // We have just loaded a deserialized `DepNode` from the previous
+        // compilation session into the current one. If this was a foreign `DefId`,
+        // then we stored additional information in the incr comp cache when we
+        // initially created its fingerprint (see `DepNodeParams::to_fingerprint`)
+        // We won't be calling `to_fingerprint` again for this `DepNode` (we no longer
+        // have the original value), so we need to copy over this additional information
+        // from the old incremental cache into the new cache that we serialize
+        // and the end of this compilation session.
+        if dep_node.kind.can_reconstruct_query_key() {
+            tcx.register_reused_dep_path_hash(DefPathHash(dep_node.hash.into()));
+        }
+        dep_node
+    }
+
+    /// When debug assertions are enabled, asserts that the dep node at `dep_node_index` is equal to `dep_node`.
+    /// This method should be preferred over manually calling `index_to_node`.
+    /// Calls to `index_to_node` may affect global state, so gating a call
+    /// to `index_to_node` on debug assertions could cause behavior changes when debug assertions
+    /// are enabled.
+    #[inline]
+    pub fn debug_assert_eq(&self, dep_node_index: SerializedDepNodeIndex, dep_node: DepNode<K>) {
+        debug_assert_eq!(self.data.nodes[dep_node_index], dep_node);
+    }
+
+    /// Obtains a debug-printable version of the `DepNode`.
+    /// See `debug_assert_eq` for why this should be preferred over manually
+    /// calling `dep_node_index`
+    pub fn debug_dep_node(&self, dep_node_index: SerializedDepNodeIndex) -> impl std::fmt::Debug {
+        // We're returning the `DepNode` without calling `register_reused_dep_path_hash`,
+        // but `impl Debug` return type means that it can only be used for debug printing.
+        // So, there's no risk of calls trying to create new dep nodes that have this
+        // node as a dependency
         self.data.nodes[dep_node_index]
     }
 

src/test/incremental/auxiliary/issue-79661.rs

@@ -0,0 +1,6 @@
+#![feature(rustc_attrs)]
+
+#[cfg_attr(any(rpass2, rpass3), doc = "Some comment")]
+pub struct Foo;
+
+pub struct Wrapper(Foo);

src/test/incremental/issue-79661-missing-def-path-hash.rs

@@ -0,0 +1,14 @@
+// aux-build:issue-79661.rs
+// revisions: rpass1 rpass2 rpass3
+
+// Regression test for issue #79661
+// We were failing to copy over a DefPathHash->DefId mapping
+// from the old incremental cache to the new incremental cache
+// when we ended up forcing a query. As a result, a subsequent
+// unchanged incremental run would crash due to the missing mapping
+
+extern crate issue_79661;
+use issue_79661::Wrapper;
+
+pub struct Outer(Wrapper);
+fn main() {}