Repository: rust-lang/rust
Pull request: Clean up the API for opening/checking incremental-compilation files 

Returning dedicated structs and enums makes the meaning of each return value more obvious, and provides a more natural home for documentation.

This part of the codebase is fairly crufty, and I doubt anyone feels fully confident about reviewing changes to it, but hopefully these particular cleanups should be fairly straightforward.

I have more changes planned in this area (e.g. cleaning up `LoadResult`), but I stopped here to make review a bit easier.

Changed files:
filename: compiler/rustc_incremental/src/persist/file_format.rs
patch: @@ -12,7 +12,7 @@
 use std::borrow::Cow;
 use std::io::{self, Read};
 use std::path::{Path, PathBuf};
-use std::{env, fs};
+use std::{array, env, fs};
 
 use rustc_data_structures::memmap::Mmap;
 use rustc_serialize::Encoder;
@@ -30,12 +30,12 @@ const HEADER_FORMAT_VERSION: u16 = 0;
 
 pub(crate) fn write_file_header(stream: &mut FileEncoder, sess: &Session) {
     stream.emit_raw_bytes(FILE_MAGIC);
-    stream
-        .emit_raw_bytes(&[(HEADER_FORMAT_VERSION >> 0) as u8, (HEADER_FORMAT_VERSION >> 8) as u8]);
+    stream.emit_raw_bytes(&u16::to_le_bytes(HEADER_FORMAT_VERSION));
 
-    let rustc_version = rustc_version(sess.is_nightly_build(), sess.cfg_version);
-    assert_eq!(rustc_version.len(), (rustc_version.len() as u8) as usize);
-    stream.emit_raw_bytes(&[rustc_version.len() as u8]);
+    let rustc_version = rustc_version(sess);
+    let rustc_version_len =
+        u8::try_from(rustc_version.len()).expect("version string should not exceed 255 bytes");
+    stream.emit_raw_bytes(&[rustc_version_len]);
     stream.emit_raw_bytes(rustc_version.as_bytes());
 }
 
@@ -80,26 +80,47 @@ where
     }
 }
 
-/// Reads the contents of a file with a file header as defined in this module.
-///
-/// - Returns `Ok(Some(data, pos))` if the file existed and was generated by a
-///   compatible compiler version. `data` is the entire contents of the file
-///   and `pos` points to the first byte after the header.
-/// - Returns `Ok(None)` if the file did not exist or was generated by an
-///   incompatible version of the compiler.
-/// - Returns `Err(..)` if some kind of IO error occurred while reading the
-///   file.
-pub(crate) fn read_file(
+pub(crate) struct OpenFile {
+    /// A read-only mmap view of the file contents.
+    pub(crate) mmap: Mmap,
+    /// File position to start reading normal data from, just after the end of the file header.
+    pub(crate) start_pos: usize,
+}
+
+pub(crate) enum OpenFileError {
+    /// Either the file was not found, or one of the header checks failed.
+    ///
+    /// These conditions prevent us from reading the file contents, but should
+    /// not trigger an error or even a warning, because they routinely happen
+    /// during normal operation:
+    /// - File-not-found occurs in a fresh build, or after clearing the build directory.
+    /// - Header-mismatch occurs after upgrading or switching compiler versions.
+    NotFoundOrHeaderMismatch,
+
+    /// An unexpected I/O error occurred while opening or checking the file.
+    IoError { err: io::Error },
+}
+
+impl From<io::Error> for OpenFileError {
+    fn from(err: io::Error) -> Self {
+        OpenFileError::IoError { err }
+    }
+}
+
+/// Tries to open a file that was written by the previous incremental-compilation
+/// session, and checks that it was produced by a matching compiler version.
+pub(crate) fn open_incremental_file(
+    sess: &Session,
     path: &Path,
-    report_incremental_info: bool,
-    is_nightly_build: bool,
-    cfg_version: &'static str,
-) -> io::Result<Option<(Mmap, usize)>> {
-    let file = match fs::File::open(path) {
-        Ok(file) => file,
-        Err(err) if err.kind() == io::ErrorKind::NotFound => return Ok(None),
-        Err(err) => return Err(err),
-    };
+) -> Result<OpenFile, OpenFileError> {
+    let file = fs::File::open(path).map_err(|err| {
+        if err.kind() == io::ErrorKind::NotFound {
+            OpenFileError::NotFoundOrHeaderMismatch
+        } else {
+            OpenFileError::IoError { err }
+        }
+    })?;
+
     // SAFETY: This process must not modify nor remove the backing file while the memory map lives.
     // For the dep-graph and the work product index, it is as soon as the decoding is done.
     // For the query result cache, the memory map is dropped in save_dep_graph before calling
@@ -116,8 +137,8 @@ pub(crate) fn read_file(
         let mut file_magic = [0u8; 4];
         file.read_exact(&mut file_magic)?;
         if file_magic != FILE_MAGIC {
-            report_format_mismatch(report_incremental_info, path, "Wrong FILE_MAGIC");
-            return Ok(None);
+            report_format_mismatch(sess, path, "Wrong FILE_MAGIC");
+            return Err(OpenFileError::NotFoundOrHeaderMismatch);
         }
     }
 
@@ -126,37 +147,35 @@ pub(crate) fn read_file(
         debug_assert!(size_of_val(&HEADER_FORMAT_VERSION) == 2);
         let mut header_format_version = [0u8; 2];
         file.read_exact(&mut header_format_version)?;
-        let header_format_version =
-            (header_format_version[0] as u16) | ((header_format_version[1] as u16) << 8);
+        let header_format_version = u16::from_le_bytes(header_format_version);
 
         if header_format_version != HEADER_FORMAT_VERSION {
-            report_format_mismatch(report_incremental_info, path, "Wrong HEADER_FORMAT_VERSION");
-            return Ok(None);
+            report_format_mismatch(sess, path, "Wrong HEADER_FORMAT_VERSION");
+            return Err(OpenFileError::NotFoundOrHeaderMismatch);
         }
     }
 
     // Check RUSTC_VERSION
     {
-        let mut rustc_version_str_len = [0u8; 1];
-        file.read_exact(&mut rustc_version_str_len)?;
-        let rustc_version_str_len = rustc_version_str_len[0] as usize;
-        let mut buffer = vec![0; rustc_version_str_len];
+        let mut rustc_version_str_len = 0u8;
+        file.read_exact(array::from_mut(&mut rustc_version_str_len))?;
+        let mut buffer = vec![0; usize::from(rustc_version_str_len)];
         file.read_exact(&mut buffer)?;
 
-        if buffer != rustc_version(is_nightly_build, cfg_version).as_bytes() {
-            report_format_mismatch(report_incremental_info, path, "Different compiler version");
-            return Ok(None);
+        if buffer != rustc_version(sess).as_bytes() {
+            report_format_mismatch(sess, path, "Different compiler version");
+            return Err(OpenFileError::NotFoundOrHeaderMismatch);
         }
     }
 
-    let post_header_start_pos = file.position() as usize;
-    Ok(Some((mmap, post_header_start_pos)))
+    let start_pos = file.position() as usize;
+    Ok(OpenFile { mmap, start_pos })
 }
 
-fn report_format_mismatch(report_incremental_info: bool, file: &Path, message: &str) {
+fn report_format_mismatch(sess: &Session, file: &Path, message: &str) {
     debug!("read_file: {}", message);
 
-    if report_incremental_info {
+    if sess.opts.unstable_opts.incremental_info {
         eprintln!(
             "[incremental] ignoring cache artifact `{}`: {}",
             file.file_name().unwrap().to_string_lossy(),
@@ -168,12 +187,13 @@ fn report_format_mismatch(report_incremental_info: bool, file: &Path, message: &
 /// A version string that hopefully is always different for compiler versions
 /// with different encodings of incremental compilation artifacts. Contains
 /// the Git commit hash.
-fn rustc_version(nightly_build: bool, cfg_version: &'static str) -> Cow<'static, str> {
-    if nightly_build {
-        if let Ok(val) = env::var("RUSTC_FORCE_RUSTC_VERSION") {
-            return val.into();
-        }
+fn rustc_version(sess: &Session) -> Cow<'static, str> {
+    // Allow version string overrides so that tests can produce a header-mismatch on demand.
+    if sess.is_nightly_build()
+        && let Ok(env_version) = env::var("RUSTC_FORCE_RUSTC_VERSION")
+    {
+        Cow::Owned(env_version)
+    } else {
+        Cow::Borrowed(sess.cfg_version)
     }
-
-    cfg_version.into()
 }

filename: compiler/rustc_incremental/src/persist/load.rs
patch: @@ -1,9 +1,8 @@
 //! Code to load the dep-graph from files.
 
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use std::sync::Arc;
 
-use rustc_data_structures::memmap::Mmap;
 use rustc_data_structures::unord::UnordMap;
 use rustc_hashes::Hash64;
 use rustc_middle::dep_graph::{DepGraph, SerializedDepGraph, WorkProductMap};
@@ -20,6 +19,7 @@ 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.
@@ -69,23 +69,6 @@ impl<T: Default> LoadResult<T> {
     }
 }
 
-fn load_data(path: &Path, sess: &Session) -> LoadResult<(Mmap, usize)> {
-    match file_format::read_file(
-        path,
-        sess.opts.unstable_opts.incremental_info,
-        sess.is_nightly_build(),
-        sess.cfg_version,
-    ) {
-        Ok(Some(data_and_pos)) => LoadResult::Ok { data: data_and_pos },
-        Ok(None) => {
-            // The file either didn't exist or was produced by an incompatible
-            // compiler version. Neither is an error.
-            LoadResult::DataOutOfDate
-        }
-        Err(err) => LoadResult::LoadDepGraph(path.to_path_buf(), err),
-    }
-}
-
 fn delete_dirty_work_product(sess: &Session, swp: SerializedWorkProduct) {
     debug!("delete_dirty_work_product({:?})", swp);
     work_product::delete_workproduct_files(sess, &swp.work_product);
@@ -113,12 +96,12 @@ fn load_dep_graph(sess: &Session) -> LoadResult<(Arc<SerializedDepGraph>, WorkPr
     // when trying to load work products.
     if sess.incr_comp_session_dir_opt().is_some() {
         let work_products_path = work_products_path(sess);
-        let load_result = load_data(&work_products_path, sess);
 
-        if let LoadResult::Ok { data: (work_products_data, start_pos) } = load_result {
+        if let Ok(OpenFile { mmap, start_pos }) =
+            file_format::open_incremental_file(sess, &work_products_path)
+        {
             // Decode the list of work_products
-            let Ok(mut work_product_decoder) = MemDecoder::new(&work_products_data[..], start_pos)
-            else {
+            let Ok(mut work_product_decoder) = MemDecoder::new(&mmap[..], start_pos) else {
                 sess.dcx().emit_warn(errors::CorruptFile { path: &work_products_path });
                 return LoadResult::DataOutOfDate;
             };
@@ -147,11 +130,11 @@ fn load_dep_graph(sess: &Session) -> LoadResult<(Arc<SerializedDepGraph>, WorkPr
 
     let _prof_timer = prof.generic_activity("incr_comp_load_dep_graph");
 
-    match load_data(&path, sess) {
-        LoadResult::DataOutOfDate => LoadResult::DataOutOfDate,
-        LoadResult::LoadDepGraph(path, err) => LoadResult::LoadDepGraph(path, err),
-        LoadResult::Ok { data: (bytes, start_pos) } => {
-            let Ok(mut decoder) = MemDecoder::new(&bytes, start_pos) else {
+    match file_format::open_incremental_file(sess, &path) {
+        Err(OpenFileError::NotFoundOrHeaderMismatch) => LoadResult::DataOutOfDate,
+        Err(OpenFileError::IoError { err }) => LoadResult::LoadDepGraph(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 });
                 return LoadResult::DataOutOfDate;
             };
@@ -191,15 +174,17 @@ pub fn load_query_result_cache(sess: &Session) -> Option<OnDiskCache> {
     let _prof_timer = sess.prof.generic_activity("incr_comp_load_query_result_cache");
 
     let path = query_cache_path(sess);
-    match load_data(&path, sess) {
-        LoadResult::Ok { data: (bytes, start_pos) } => {
-            let cache = OnDiskCache::new(sess, bytes, start_pos).unwrap_or_else(|()| {
+    match file_format::open_incremental_file(sess, &path) {
+        Ok(OpenFile { mmap, start_pos }) => {
+            let cache = OnDiskCache::new(sess, mmap, start_pos).unwrap_or_else(|()| {
                 sess.dcx().emit_warn(errors::CorruptFile { path: &path });
                 OnDiskCache::new_empty()
             });
             Some(cache)
         }
-        _ => Some(OnDiskCache::new_empty()),
+        Err(OpenFileError::NotFoundOrHeaderMismatch | OpenFileError::IoError { .. }) => {
+            Some(OnDiskCache::new_empty())
+        }
     }
 }