Auto merge of rust-lang#119139 - michaelwoerister:cleanup-stable-source-file-id, r=cjgillot

Unify SourceFile::name_hash and StableSourceFileId

This PR adapts the existing `StableSourceFileId` type so that it can be used instead of the `name_hash` field of `SourceFile`. This simplifies a few things that were kind of duplicated before.

The PR should also fix issues rust-lang#112700 and rust-lang#115835, but I was not able to reproduce these issues in a regression test. As far as I can tell, the root cause of these issues is that the id of the originating crate is not hashed in the `HashStable` impl of `Span` and thus cache entries that should have been considered invalidated were loaded. After this PR, the `stable_id` field of `SourceFile` includes information about the originating crate, so that ICE should not occur anymore.

Author: bors

Diff for: compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs

@@ -534,7 +534,7 @@ fn hex_encode(data: &[u8]) -> String {
 }
 
 pub fn file_metadata<'ll>(cx: &CodegenCx<'ll, '_>, source_file: &SourceFile) -> &'ll DIFile {
-    let cache_key = Some((source_file.name_hash, source_file.src_hash));
+    let cache_key = Some((source_file.stable_id, source_file.src_hash));
     return debug_context(cx)
         .created_files
         .borrow_mut()

Diff for: compiler/rustc_codegen_llvm/src/debuginfo/mod.rs

@@ -1,6 +1,7 @@
 #![doc = include_str!("doc.md")]
 
 use rustc_codegen_ssa::mir::debuginfo::VariableKind::*;
+use rustc_data_structures::unord::UnordMap;
 
 use self::metadata::{file_metadata, type_di_node};
 use self::metadata::{UNKNOWN_COLUMN_NUMBER, UNKNOWN_LINE_NUMBER};
@@ -20,8 +21,6 @@ use crate::value::Value;
 use rustc_codegen_ssa::debuginfo::type_names;
 use rustc_codegen_ssa::mir::debuginfo::{DebugScope, FunctionDebugContext, VariableKind};
 use rustc_codegen_ssa::traits::*;
-use rustc_data_structures::fx::FxHashMap;
-use rustc_data_structures::stable_hasher::Hash128;
 use rustc_data_structures::sync::Lrc;
 use rustc_hir::def_id::{DefId, DefIdMap};
 use rustc_index::IndexVec;
@@ -32,7 +31,9 @@ use rustc_middle::ty::{self, Instance, ParamEnv, Ty, TypeVisitableExt};
 use rustc_session::config::{self, DebugInfo};
 use rustc_session::Session;
 use rustc_span::symbol::Symbol;
-use rustc_span::{BytePos, Pos, SourceFile, SourceFileAndLine, SourceFileHash, Span};
+use rustc_span::{
+    BytePos, Pos, SourceFile, SourceFileAndLine, SourceFileHash, Span, StableSourceFileId,
+};
 use rustc_target::abi::Size;
 
 use libc::c_uint;
@@ -61,7 +62,7 @@ pub struct CodegenUnitDebugContext<'ll, 'tcx> {
     llcontext: &'ll llvm::Context,
     llmod: &'ll llvm::Module,
     builder: &'ll mut DIBuilder<'ll>,
-    created_files: RefCell<FxHashMap<Option<(Hash128, SourceFileHash)>, &'ll DIFile>>,
+    created_files: RefCell<UnordMap<Option<(StableSourceFileId, SourceFileHash)>, &'ll DIFile>>,
 
     type_map: metadata::TypeMap<'ll, 'tcx>,
     namespace_map: RefCell<DefIdMap<&'ll DIScope>>,

Diff for: compiler/rustc_metadata/src/rmeta/decoder.rs

@@ -1671,7 +1671,7 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
                     multibyte_chars,
                     non_narrow_chars,
                     normalized_pos,
-                    name_hash,
+                    stable_id,
                     ..
                 } = source_file_to_import;
 
@@ -1716,7 +1716,7 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
                 let local_version = sess.source_map().new_imported_source_file(
                     name,
                     src_hash,
-                    name_hash,
+                    stable_id,
                     source_len.to_u32(),
                     self.cnum,
                     lines,

Diff for: compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -5,7 +5,7 @@ use rustc_ast::Attribute;
 use rustc_data_structures::fingerprint::Fingerprint;
 use rustc_data_structures::fx::FxIndexSet;
 use rustc_data_structures::memmap::{Mmap, MmapMut};
-use rustc_data_structures::stable_hasher::{Hash128, HashStable, StableHasher};
+use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_data_structures::sync::{join, par_for_each_in, Lrc};
 use rustc_data_structures::temp_dir::MaybeTempDir;
 use rustc_hir as hir;
@@ -26,11 +26,12 @@ use rustc_serialize::{opaque, Decodable, Decoder, Encodable, Encoder};
 use rustc_session::config::{CrateType, OptLevel};
 use rustc_span::hygiene::HygieneEncodeContext;
 use rustc_span::symbol::sym;
-use rustc_span::{ExternalSource, FileName, SourceFile, SpanData, SyntaxContext};
+use rustc_span::{
+    ExternalSource, FileName, SourceFile, SpanData, StableSourceFileId, SyntaxContext,
+};
 use std::borrow::Borrow;
 use std::collections::hash_map::Entry;
 use std::fs::File;
-use std::hash::Hash;
 use std::io::{Read, Seek, Write};
 use std::path::{Path, PathBuf};
 
@@ -495,6 +496,8 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
 
         let mut adapted = TableBuilder::default();
 
+        let local_crate_stable_id = self.tcx.stable_crate_id(LOCAL_CRATE);
+
         // Only serialize `SourceFile`s that were used during the encoding of a `Span`.
         //
         // The order in which we encode source files is important here: the on-disk format for
@@ -511,7 +514,9 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
             //
             // At this point we also erase the actual on-disk path and only keep
             // the remapped version -- as is necessary for reproducible builds.
-            let mut source_file = match source_file.name {
+            let mut adapted_source_file = (**source_file).clone();
+
+            match source_file.name {
                 FileName::Real(ref original_file_name) => {
                     let adapted_file_name = if self.tcx.sess.should_prefer_remapped_for_codegen() {
                         source_map.path_mapping().to_embeddable_absolute_path(
@@ -525,22 +530,11 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
                         )
                     };
 
-                    if adapted_file_name != *original_file_name {
-                        let mut adapted: SourceFile = (**source_file).clone();
-                        adapted.name = FileName::Real(adapted_file_name);
-                        adapted.name_hash = {
-                            let mut hasher: StableHasher = StableHasher::new();
-                            adapted.name.hash(&mut hasher);
-                            hasher.finish::<Hash128>()
-                        };
-                        Lrc::new(adapted)
-                    } else {
-                        // Nothing to adapt
-                        source_file.clone()
-                    }
+                    adapted_source_file.name = FileName::Real(adapted_file_name);
+                }
+                _ => {
+                    // expanded code, not from a file
                 }
-                // expanded code, not from a file
-                _ => source_file.clone(),
             };
 
             // We're serializing this `SourceFile` into our crate metadata,
@@ -550,12 +544,20 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
             // dependencies aren't loaded when we deserialize a proc-macro,
             // trying to remap the `CrateNum` would fail.
             if self.is_proc_macro {
-                Lrc::make_mut(&mut source_file).cnum = LOCAL_CRATE;
+                adapted_source_file.cnum = LOCAL_CRATE;
             }
 
+            // Update the `StableSourceFileId` to make sure it incorporates the
+            // id of the current crate. This way it will be unique within the
+            // crate graph during downstream compilation sessions.
+            adapted_source_file.stable_id = StableSourceFileId::from_filename_for_export(
+                &adapted_source_file.name,
+                local_crate_stable_id,
+            );
+
             let on_disk_index: u32 =
                 on_disk_index.try_into().expect("cannot export more than U32_MAX files");
-            adapted.set_some(on_disk_index, self.lazy(source_file));
+            adapted.set_some(on_disk_index, self.lazy(adapted_source_file));
         }
 
         adapted.encode(&mut self.opaque)

Diff for: compiler/rustc_middle/src/hir/map/mod.rs

@@ -1098,7 +1098,7 @@ pub(super) fn crate_hash(tcx: TyCtxt<'_>, _: LocalCrate) -> Svh {
         .files()
         .iter()
         .filter(|source_file| source_file.cnum == LOCAL_CRATE)
-        .map(|source_file| source_file.name_hash)
+        .map(|source_file| source_file.stable_id)
         .collect();
 
     source_file_names.sort_unstable();

Diff for: compiler/rustc_middle/src/query/on_disk_cache.rs

@@ -1,6 +1,5 @@
 use rustc_data_structures::fx::{FxHashMap, FxIndexSet};
 use rustc_data_structures::memmap::Mmap;
-use rustc_data_structures::stable_hasher::Hash64;
 use rustc_data_structures::sync::{HashMapExt, Lock, Lrc, RwLock};
 use rustc_data_structures::unhash::UnhashMap;
 use rustc_data_structures::unord::UnordSet;
@@ -21,8 +20,10 @@ use rustc_session::Session;
 use rustc_span::hygiene::{
     ExpnId, HygieneDecodeContext, HygieneEncodeContext, SyntaxContext, SyntaxContextData,
 };
-use rustc_span::source_map::{SourceMap, StableSourceFileId};
-use rustc_span::{BytePos, ExpnData, ExpnHash, Pos, RelativeBytePos, SourceFile, Span};
+use rustc_span::source_map::SourceMap;
+use rustc_span::{
+    BytePos, ExpnData, ExpnHash, Pos, RelativeBytePos, SourceFile, Span, StableSourceFileId,
+};
 use rustc_span::{CachingSourceMapView, Symbol};
 use std::collections::hash_map::Entry;
 use std::mem;
@@ -133,30 +134,18 @@ impl AbsoluteBytePos {
     }
 }
 
-/// An `EncodedSourceFileId` is the same as a `StableSourceFileId` except that
-/// the source crate is represented as a [StableCrateId] instead of as a
-/// `CrateNum`. This way `EncodedSourceFileId` can be encoded and decoded
-/// without any additional context, i.e. with a simple `opaque::Decoder` (which
-/// is the only thing available when decoding the cache's [Footer].
 #[derive(Encodable, Decodable, Clone, Debug)]
 struct EncodedSourceFileId {
-    file_name_hash: Hash64,
+    stable_source_file_id: StableSourceFileId,
     stable_crate_id: StableCrateId,
 }
 
 impl EncodedSourceFileId {
-    #[inline]
-    fn translate(&self, tcx: TyCtxt<'_>) -> StableSourceFileId {
-        let cnum = tcx.stable_crate_id_to_crate_num(self.stable_crate_id);
-        StableSourceFileId { file_name_hash: self.file_name_hash, cnum }
-    }
-
     #[inline]
     fn new(tcx: TyCtxt<'_>, file: &SourceFile) -> EncodedSourceFileId {
-        let source_file_id = StableSourceFileId::new(file);
         EncodedSourceFileId {
-            file_name_hash: source_file_id.file_name_hash,
-            stable_crate_id: tcx.stable_crate_id(source_file_id.cnum),
+            stable_source_file_id: file.stable_id,
+            stable_crate_id: tcx.stable_crate_id(file.cnum),
         }
     }
 }
@@ -488,7 +477,9 @@ impl<'a, 'tcx> CacheDecoder<'a, 'tcx> {
             .borrow_mut()
             .entry(index)
             .or_insert_with(|| {
-                let stable_id = file_index_to_stable_id[&index].translate(tcx);
+                let source_file_id = &file_index_to_stable_id[&index];
+                let source_file_cnum =
+                    tcx.stable_crate_id_to_crate_num(source_file_id.stable_crate_id);
 
                 // If this `SourceFile` is from a foreign crate, then make sure
                 // that we've imported all of the source files from that crate.
@@ -499,12 +490,14 @@ impl<'a, 'tcx> CacheDecoder<'a, 'tcx> {
                 // that we will load the source files from that crate during macro
                 // expansion, so we use `import_source_files` to ensure that the foreign
                 // source files are actually imported before we call `source_file_by_stable_id`.
-                if stable_id.cnum != LOCAL_CRATE {
-                    self.tcx.cstore_untracked().import_source_files(self.tcx.sess, stable_id.cnum);
+                if source_file_cnum != LOCAL_CRATE {
+                    self.tcx
+                        .cstore_untracked()
+                        .import_source_files(self.tcx.sess, source_file_cnum);
                 }
 
                 source_map
-                    .source_file_by_stable_id(stable_id)
+                    .source_file_by_stable_id(source_file_id.stable_source_file_id)
                     .expect("failed to lookup `SourceFile` in new context")
             })
             .clone()

Diff for: compiler/rustc_query_system/src/ich/impls_syntax.rs

@@ -60,8 +60,8 @@ impl<'ctx> rustc_ast::HashStableContext for StableHashingContext<'ctx> {
 impl<'a> HashStable<StableHashingContext<'a>> for SourceFile {
     fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
         let SourceFile {
-            name: _, // We hash the smaller name_hash instead of this
-            name_hash,
+            name: _, // We hash the smaller stable_id instead of this
+            stable_id,
             cnum,
             // Do not hash the source as it is not encoded
             src: _,
@@ -75,7 +75,7 @@ impl<'a> HashStable<StableHashingContext<'a>> for SourceFile {
             ref normalized_pos,
         } = *self;
 
-        name_hash.hash_stable(hcx, hasher);
+        stable_id.hash_stable(hcx, hasher);
 
         src_hash.hash_stable(hcx, hasher);