Address review comments

Author: Aaron1011

compiler/rustc_query_system/src/dep_graph/graph.rs

@@ -171,6 +171,57 @@ impl<K: DepKind> DepGraph<K> {
         K::with_deps(None, op)
     }
 
+    /// Used to wrap the deserialization of a query result from disk,
+    /// This method enforces that no new `DepNodes` are created during
+    /// query result deserialization.
+    ///
+    /// Enforcing this makes the query dep graph simpler - all nodes
+    /// must be created during the query execution, and should be
+    /// created from inside the 'body' of a query (the implementation
+    /// provided by a particular compiler crate).
+    ///
+    /// Consider the case of three queries `A`, `B`, and `C`, where
+    /// `A` invokes `B` and `B` invokes `C`:
+    ///
+    /// `A -> B -> C`
+    ///
+    /// Suppose that decoding the result of query `B` required invoking
+    /// a query `D`. If we did not create a fresh `TaskDeps` when
+    /// decoding `B`, we might would still be using the `TaskDeps` for query `A`
+    /// (if we needed to re-execute `A`). This would cause us to create
+    /// a new edge `A -> D`. If this edge did not previously
+    /// exist in the `DepGraph`, then we could end up with a different
+    /// `DepGraph` at the end of compilation, even if there were no
+    /// meaningful changes to the overall program (e.g. a newline was added).
+    /// In addition, this edge might cause a subsequent compilation run
+    /// to try to force `D` before marking other necessary nodes green. If
+    /// `D` did not exist in the new compilation session, then we might
+    /// get an ICE. Normally, we would have tried (and failed) to mark
+    /// some other query green (e.g. `item_children`) which was used
+    /// to obtain `D`, which would prevent us from ever trying to force
+    /// a non-existent `D`.
+    ///
+    /// It might be possible to enforce that all `DepNode`s read during
+    /// deserialization already exist in the previous `DepGraph`. In
+    /// the above example, we would invoke `D` during the deserialization
+    /// of `B`. Since we correctly create a new `TaskDeps` from the decoding
+    /// of `B`, this would result in an edge `B -> D`. If that edge already
+    /// existed (with the same `DepPathHash`es), then it should be correct
+    /// to allow the invocation of the query to proceed during deserialization
+    /// of a query result. However, this would require additional complexity
+    /// in the query infrastructure, and is not currently needed by the
+    /// decoding of any query results. Should the need arise in the future,
+    /// we should consider extending the query system with this functionality.
+    pub fn with_query_deserialization<OP, R>(&self, op: OP) -> R
+    where
+        OP: FnOnce() -> R,
+    {
+        let mut deps = TaskDeps::default();
+        deps.read_allowed = false;
+        let deps = Lock::new(deps);
+        K::with_deps(Some(&deps), op)
+    }
+
     /// Starts a new dep-graph task. Dep-graph tasks are specified
     /// using a free function (`task`) and **not** a closure -- this
     /// is intentional because we want to exercise tight control over
@@ -1121,7 +1172,12 @@ pub struct TaskDeps<K> {
     reads: EdgesVec,
     read_set: FxHashSet<DepNodeIndex>,
     phantom_data: PhantomData<DepNode<K>>,
-    pub read_allowed: bool,
+    /// Whether or not we allow `DepGraph::read_index` to run.
+    /// This is normally true, except inside `with_query_deserialization`,
+    /// where it set to `false` to enforce that no new `DepNode` edges are
+    /// created. See the documentation of `with_query_deserialization` for
+    /// more details.
+    read_allowed: bool,
 }
 
 impl<K> Default for TaskDeps<K> {

compiler/rustc_query_system/src/query/plumbing.rs

@@ -2,8 +2,7 @@
 //! generate the actual methods on tcx which find and execute the provider,
 //! manage the caches, and so forth.
 
-use crate::dep_graph::DepKind;
-use crate::dep_graph::{DepContext, DepNode, DepNodeIndex, DepNodeParams, TaskDeps};
+use crate::dep_graph::{DepContext, DepNode, DepNodeIndex, DepNodeParams};
 use crate::query::caches::QueryCache;
 use crate::query::config::{QueryDescription, QueryVtable};
 use crate::query::job::{
@@ -516,12 +515,12 @@ where
     if query.cache_on_disk {
         let prof_timer = tcx.dep_context().profiler().incr_cache_loading();
 
-        let mut deps = TaskDeps::default();
-        deps.read_allowed = false;
-        let deps = Lock::new(deps);
-        let result = CTX::DepKind::with_deps(Some(&deps), || {
-            query.try_load_from_disk(tcx, prev_dep_node_index)
-        });
+        // The call to `with_query_deserialization` enforces that no new `DepNodes`
+        // are created during deserialization. See the docs of that method for more
+        // details.
+        let result = dep_graph
+            .with_query_deserialization(|| query.try_load_from_disk(tcx, prev_dep_node_index));
+
         prof_timer.finish_with_query_invocation_id(dep_node_index.into());
 
         if let Some(result) = result {