Repository: rust-lang/rust
Pull request: Make `setup_dep_graph` incremental-only and more straightforward 

The existing code contains some very strange control flow that can put the dep graph into an inconsistent and untested state if streaming output setup fails.

(Specifically, that failure would put `DepGraph` into an "empty" state intended for non-incremental compilation, but other parts of the compiler would still think that incremental mode is enabled due to `sess.opts.incremental`.)

This PR therefore performs a big overhaul of `setup_dep_graph` by:

- Returning immediately in non-incremental mode.
- Exiting immediately if dep-graph streaming output couldn't be set up.
- Inlining some "helper" functions that were more confusing than helpful.

Changed files:
filename: compiler/rustc_incremental/src/lib.rs
patch: @@ -10,9 +10,8 @@ mod errors;
 mod persist;
 
 pub use persist::{
-    LoadResult, copy_cgu_workproduct_to_incr_comp_cache_dir, finalize_session_directory,
-    in_incr_comp_dir, in_incr_comp_dir_sess, load_query_result_cache, save_work_product_index,
-    setup_dep_graph,
+    copy_cgu_workproduct_to_incr_comp_cache_dir, finalize_session_directory, in_incr_comp_dir,
+    in_incr_comp_dir_sess, load_query_result_cache, save_work_product_index, setup_dep_graph,
 };
 use rustc_middle::util::Providers;
 

filename: compiler/rustc_incremental/src/persist/fs.rs
patch: @@ -215,9 +215,7 @@ pub(crate) fn prepare_session_directory(
     crate_name: Symbol,
     stable_crate_id: StableCrateId,
 ) {
-    if sess.opts.incremental.is_none() {
-        return;
-    }
+    assert!(sess.opts.incremental.is_some());
 
     let _timer = sess.timer("incr_comp_prepare_session_directory");
 

filename: compiler/rustc_incremental/src/persist/load.rs
patch: @@ -1,75 +1,44 @@
 //! Code to load the dep-graph from files.
 
+use std::io;
 use std::path::PathBuf;
 use std::sync::Arc;
 
 use rustc_data_structures::unord::UnordMap;
 use rustc_hashes::Hash64;
 use rustc_middle::dep_graph::{DepGraph, SerializedDepGraph, WorkProductMap};
 use rustc_middle::query::on_disk_cache::OnDiskCache;
-use rustc_serialize::Decodable;
-use rustc_serialize::opaque::MemDecoder;
+use rustc_serialize::opaque::{FileEncoder, MemDecoder};
+use rustc_serialize::{Decodable, Encodable};
 use rustc_session::config::IncrementalStateAssertion;
 use rustc_session::{Session, StableCrateId};
 use rustc_span::Symbol;
 use tracing::{debug, warn};
 
 use super::data::*;
 use super::fs::*;
-use super::save::build_dep_graph;
 use super::{file_format, work_product};
 use crate::errors;
 use crate::persist::file_format::{OpenFile, OpenFileError};
 
 #[derive(Debug)]
 /// Represents the result of an attempt to load incremental compilation data.
-pub enum LoadResult<T> {
+enum LoadResult {
     /// Loading was successful.
-    Ok {
-        #[allow(missing_docs)]
-        data: T,
-    },
+    Ok { prev_graph: Arc<SerializedDepGraph>, prev_work_products: WorkProductMap },
     /// The file either didn't exist or was produced by an incompatible compiler version.
     DataOutOfDate,
-    /// Loading the dep graph failed.
-    LoadDepGraph(PathBuf, std::io::Error),
-}
-
-impl<T: Default> LoadResult<T> {
-    /// Accesses the data returned in [`LoadResult::Ok`].
-    pub fn open(self, sess: &Session) -> T {
-        // Emit a fatal error if `-Zassert-incr-state` is present and unsatisfied.
-        maybe_assert_incr_state(sess, &self);
-
-        match self {
-            LoadResult::LoadDepGraph(path, err) => {
-                sess.dcx().emit_warn(errors::LoadDepGraph { path, err });
-                Default::default()
-            }
-            LoadResult::DataOutOfDate => {
-                if let Err(err) = delete_all_session_dir_contents(sess) {
-                    sess.dcx()
-                        .emit_err(errors::DeleteIncompatible { path: dep_graph_path(sess), err });
-                }
-                Default::default()
-            }
-            LoadResult::Ok { data } => data,
-        }
-    }
+    /// Loading failed due to an unexpected I/O error.
+    IoError { path: PathBuf, err: io::Error },
 }
 
 fn delete_dirty_work_product(sess: &Session, swp: SerializedWorkProduct) {
     debug!("delete_dirty_work_product({:?})", swp);
     work_product::delete_workproduct_files(sess, &swp.work_product);
 }
 
-fn load_dep_graph(sess: &Session) -> LoadResult<(Arc<SerializedDepGraph>, WorkProductMap)> {
-    let prof = sess.prof.clone();
-
-    if sess.opts.incremental.is_none() {
-        // No incremental compilation.
-        return LoadResult::Ok { data: Default::default() };
-    }
+fn load_dep_graph(sess: &Session) -> LoadResult {
+    assert!(sess.opts.incremental.is_some());
 
     let _timer = sess.prof.generic_activity("incr_comp_prepare_load_dep_graph");
 
@@ -117,11 +86,11 @@ fn load_dep_graph(sess: &Session) -> LoadResult<(Arc<SerializedDepGraph>, WorkPr
         }
     }
 
-    let _prof_timer = prof.generic_activity("incr_comp_load_dep_graph");
+    let _prof_timer = sess.prof.generic_activity("incr_comp_load_dep_graph");
 
     match file_format::open_incremental_file(sess, &path) {
         Err(OpenFileError::NotFoundOrHeaderMismatch) => LoadResult::DataOutOfDate,
-        Err(OpenFileError::IoError { err }) => LoadResult::LoadDepGraph(path.to_owned(), err),
+        Err(OpenFileError::IoError { err }) => LoadResult::IoError { path: path.to_owned(), err },
         Ok(OpenFile { mmap, start_pos }) => {
             let Ok(mut decoder) = MemDecoder::new(&mmap, start_pos) else {
                 sess.dcx().emit_warn(errors::CorruptFile { path: &path });
@@ -143,9 +112,9 @@ fn load_dep_graph(sess: &Session) -> LoadResult<(Arc<SerializedDepGraph>, WorkPr
                 return LoadResult::DataOutOfDate;
             }
 
-            let dep_graph = SerializedDepGraph::decode(&mut decoder);
+            let prev_graph = SerializedDepGraph::decode(&mut decoder);
 
-            LoadResult::Ok { data: (dep_graph, prev_work_products) }
+            LoadResult::Ok { prev_graph, prev_work_products }
         }
     }
 }
@@ -179,14 +148,14 @@ pub fn load_query_result_cache(sess: &Session) -> Option<OnDiskCache> {
 
 /// Emits a fatal error if the assertion in `-Zassert-incr-state` doesn't match
 /// the outcome of trying to load previous-session state.
-fn maybe_assert_incr_state(sess: &Session, load_result: &LoadResult<impl Sized>) {
+fn maybe_assert_incr_state(sess: &Session, load_result: &LoadResult) {
     // Return immediately if there's nothing to assert.
     let Some(assertion) = sess.opts.unstable_opts.assert_incr_state else { return };
 
     // Match exhaustively to make sure we don't miss any cases.
     let loaded = match load_result {
         LoadResult::Ok { .. } => true,
-        LoadResult::DataOutOfDate | LoadResult::LoadDepGraph(..) => false,
+        LoadResult::DataOutOfDate | LoadResult::IoError { .. } => false,
     };
 
     match assertion {
@@ -203,33 +172,64 @@ fn maybe_assert_incr_state(sess: &Session, load_result: &LoadResult<impl Sized>)
     }
 }
 
-/// Setups the dependency graph by loading an existing graph from disk and set up streaming of a
-/// new graph to an incremental session directory.
+/// Loads the previous session's dependency graph from disk if possible, and
+/// sets up streaming output for the current session's dep graph data into an
+/// incremental session directory.
+///
+/// In non-incremental mode, a dummy dep graph is returned immediately.
 pub fn setup_dep_graph(
     sess: &Session,
     crate_name: Symbol,
     stable_crate_id: StableCrateId,
 ) -> DepGraph {
+    if sess.opts.incremental.is_none() {
+        return DepGraph::new_disabled();
+    }
+
     // `load_dep_graph` can only be called after `prepare_session_directory`.
     prepare_session_directory(sess, crate_name, stable_crate_id);
+    // Try to load the previous session's dep graph and work products.
+    let load_result = load_dep_graph(sess);
+
+    sess.time("incr_comp_garbage_collect_session_directories", || {
+        if let Err(e) = garbage_collect_session_directories(sess) {
+            warn!(
+                "Error while trying to garbage collect incremental compilation \
+                cache directory: {e}",
+            );
+        }
+    });
 
-    let res = sess.opts.build_dep_graph().then(|| load_dep_graph(sess));
+    // Emit a fatal error if `-Zassert-incr-state` is present and unsatisfied.
+    maybe_assert_incr_state(sess, &load_result);
 
-    if sess.opts.incremental.is_some() {
-        sess.time("incr_comp_garbage_collect_session_directories", || {
-            if let Err(e) = garbage_collect_session_directories(sess) {
-                warn!(
-                    "Error while trying to garbage collect incremental \
-                     compilation cache directory: {}",
-                    e
-                );
+    let (prev_graph, prev_work_products) = match load_result {
+        LoadResult::IoError { path, err } => {
+            sess.dcx().emit_warn(errors::LoadDepGraph { path, err });
+            Default::default()
+        }
+        LoadResult::DataOutOfDate => {
+            if let Err(err) = delete_all_session_dir_contents(sess) {
+                sess.dcx().emit_err(errors::DeleteIncompatible { path: dep_graph_path(sess), err });
             }
-        });
-    }
+            Default::default()
+        }
+        LoadResult::Ok { prev_graph, prev_work_products } => (prev_graph, prev_work_products),
+    };
+
+    // Stream the dep-graph to an alternate file, to avoid overwriting anything in case of errors.
+    let path_buf = staging_dep_graph_path(sess);
+
+    let mut encoder = FileEncoder::new(&path_buf).unwrap_or_else(|err| {
+        // We're in incremental mode but couldn't set up streaming output of the dep graph.
+        // Exit immediately instead of continuing in an inconsistent and untested state.
+        sess.dcx().emit_fatal(errors::CreateDepGraph { path: &path_buf, err })
+    });
+
+    file_format::write_file_header(&mut encoder, sess);
+
+    // First encode the commandline arguments hash
+    sess.opts.dep_tracking_hash(false).encode(&mut encoder);
 
-    res.and_then(|result| {
-        let (prev_graph, prev_work_products) = result.open(sess);
-        build_dep_graph(sess, prev_graph, prev_work_products)
-    })
-    .unwrap_or_else(DepGraph::new_disabled)
+    DepGraph::new(sess, prev_graph, prev_work_products, encoder)
 }

filename: compiler/rustc_incremental/src/persist/mod.rs
patch: @@ -11,7 +11,7 @@ mod save;
 mod work_product;
 
 pub use fs::{finalize_session_directory, in_incr_comp_dir, in_incr_comp_dir_sess};
-pub use load::{LoadResult, load_query_result_cache, setup_dep_graph};
+pub use load::{load_query_result_cache, setup_dep_graph};
 pub(crate) use save::save_dep_graph;
 pub use save::save_work_product_index;
 pub use work_product::copy_cgu_workproduct_to_incr_comp_cache_dir;

filename: compiler/rustc_incremental/src/persist/save.rs
patch: @@ -1,11 +1,8 @@
 use std::fs;
-use std::sync::Arc;
 
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_data_structures::sync::par_join;
-use rustc_middle::dep_graph::{
-    DepGraph, SerializedDepGraph, WorkProduct, WorkProductId, WorkProductMap,
-};
+use rustc_middle::dep_graph::{DepGraph, WorkProduct, WorkProductId};
 use rustc_middle::query::on_disk_cache;
 use rustc_middle::ty::TyCtxt;
 use rustc_serialize::Encodable as RustcEncodable;
@@ -149,38 +146,3 @@ fn encode_work_product_index(
 
     serialized_products.encode(encoder)
 }
-
-/// Builds the dependency graph.
-///
-/// This function creates the *staging dep-graph*. When the dep-graph is modified by a query
-/// execution, the new dependency information is not kept in memory but directly
-/// output to this file. `save_dep_graph` then finalizes the staging dep-graph
-/// and moves it to the permanent dep-graph path
-pub(crate) fn build_dep_graph(
-    sess: &Session,
-    prev_graph: Arc<SerializedDepGraph>,
-    prev_work_products: WorkProductMap,
-) -> Option<DepGraph> {
-    if sess.opts.incremental.is_none() {
-        // No incremental compilation.
-        return None;
-    }
-
-    // Stream the dep-graph to an alternate file, to avoid overwriting anything in case of errors.
-    let path_buf = staging_dep_graph_path(sess);
-
-    let mut encoder = match FileEncoder::new(&path_buf) {
-        Ok(encoder) => encoder,
-        Err(err) => {
-            sess.dcx().emit_err(errors::CreateDepGraph { path: &path_buf, err });
-            return None;
-        }
-    };
-
-    file_format::write_file_header(&mut encoder, sess);
-
-    // First encode the commandline arguments hash
-    sess.opts.dep_tracking_hash(false).encode(&mut encoder);
-
-    Some(DepGraph::new(sess, prev_graph, prev_work_products, encoder))
-}