Make local_path in RealFileName::Remapped Option to be removed in exported metadata

cbeuw · cbeuw · commit 9e0426d7842c · 2021-05-05T15:10:57.000+01:00
diff --git a/compiler/rustc_expand/src/base.rs b/compiler/rustc_expand/src/base.rs
@@ -1073,7 +1073,9 @@ impl<'a> ExtCtxt<'a> {
         if !path.is_absolute() {
             let callsite = span.source_callsite();
             let mut result = match self.source_map().span_to_filename(callsite) {
-                FileName::Real(name) => name.into_local_path(),
+                FileName::Real(name) => name
+                    .into_local_path()
+                    .expect("attempting to resolve a file path in an external file"),
                 FileName::DocTest(path, _) => path,
                 other => {
                     return Err(self.struct_span_err(
diff --git a/compiler/rustc_expand/src/expand.rs b/compiler/rustc_expand/src/expand.rs
@@ -362,7 +362,9 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
     // make crate a first class expansion target instead.
     pub fn expand_crate(&mut self, mut krate: ast::Crate) -> ast::Crate {
         let file_path = match self.cx.source_map().span_to_filename(krate.span) {
-            FileName::Real(name) => name.into_local_path(),
+            FileName::Real(name) => name
+                .into_local_path()
+                .expect("attempting to resolve a file path in an external file"),
             other => PathBuf::from(other.to_string()),
         };
         let dir_path = file_path.parent().unwrap_or(&file_path).to_owned();
diff --git a/compiler/rustc_expand/src/proc_macro_server.rs b/compiler/rustc_expand/src/proc_macro_server.rs
@@ -623,6 +623,7 @@ impl server::SourceFile for Rustc<'_> {
         match file.name {
             FileName::Real(ref name) => name
                 .local_path()
+                .expect("attempting to get a file path in an imported file in `proc_macro::SourceFile::path`")
                 .to_str()
                 .expect("non-UTF8 file path in `proc_macro::SourceFile::path`")
                 .to_string(),
diff --git a/compiler/rustc_metadata/src/rmeta/decoder.rs b/compiler/rustc_metadata/src/rmeta/decoder.rs
@@ -1676,7 +1676,7 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
                                     new_path.display(),
                                 );
                                 let new_name = rustc_span::RealFileName::Remapped {
-                                    local_path: new_path,
+                                    local_path: Some(new_path),
                                     virtual_name,
                                 };
                                 *old_name = new_name;
diff --git a/compiler/rustc_metadata/src/rmeta/encoder.rs b/compiler/rustc_metadata/src/rmeta/encoder.rs
@@ -497,9 +497,10 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
                             RealFileName::LocalPath(local_path) => {
                                 Path::new(&working_dir).join(local_path).into()
                             }
-                            RealFileName::Remapped { local_path, virtual_name } => {
+                            RealFileName::Remapped { local_path: _, virtual_name } => {
                                 FileName::Real(RealFileName::Remapped {
-                                    local_path: Path::new(&working_dir).join(local_path),
+                                    // We do not want any local path to be exported into metadata
+                                    local_path: None,
                                     virtual_name: virtual_name.clone(),
                                 })
                             }
diff --git a/compiler/rustc_span/src/lib.rs b/compiler/rustc_span/src/lib.rs
@@ -115,38 +115,80 @@ scoped_tls::scoped_thread_local!(pub static SESSION_GLOBALS: SessionGlobals);
 
 // FIXME: We should use this enum or something like it to get rid of the
 // use of magic `/rust/1.x/...` paths across the board.
-#[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd, Hash)]
-#[derive(HashStable_Generic, Decodable, Encodable)]
+#[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd)]
+#[derive(HashStable_Generic, Decodable)]
 pub enum RealFileName {
     LocalPath(PathBuf),
     /// For remapped paths (namely paths into libstd that have been mapped
     /// to the appropriate spot on the local host's file system, and local file
     /// system paths that have been remapped with `FilePathMapping`),
     Remapped {
-        /// `local_path` is the (host-dependent) local path to the file.
-        local_path: PathBuf,
+        /// `local_path` is the (host-dependent) local path to the file. This is
+        /// None if the file was imported from another crate
+        local_path: Option<PathBuf>,
         /// `virtual_name` is the stable path rustc will store internally within
         /// build artifacts.
         virtual_name: PathBuf,
     },
 }
 
+impl Hash for RealFileName {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        // To prevent #70924 from happening again we should only hash the
+        // remapped (virtualized) path if that exists. This is because
+        // virtualized paths to sysroot crates (/rust/$hash or /rust/$version)
+        // remain stable even if the corresponding local_path changes
+        self.remapped_path_if_available().hash(state)
+    }
+}
+
+// This is functionally identical to #[derive(Encodable)], with the exception of
+// an added assert statement
+impl<S: Encoder> Encodable<S> for RealFileName {
+    fn encode(&self, encoder: &mut S) -> Result<(), S::Error> {
+        encoder.emit_enum("RealFileName", |encoder| match *self {
+            RealFileName::LocalPath(ref local_path) => {
+                encoder.emit_enum_variant("LocalPath", 0, 1, |encoder| {
+                    Ok({
+                        encoder.emit_enum_variant_arg(0, |encoder| local_path.encode(encoder))?;
+                    })
+                })
+            }
+
+            RealFileName::Remapped { ref local_path, ref virtual_name } => encoder
+                .emit_enum_variant("Remapped", 1, 2, |encoder| {
+                    // For privacy and build reproducibility, we must not embed host-dependant path in artifacts
+                    // if they have been remapped by --remap-path-prefix
+                    assert!(local_path.is_none());
+                    Ok({
+                        encoder.emit_enum_variant_arg(0, |encoder| local_path.encode(encoder))?;
+                        encoder.emit_enum_variant_arg(1, |encoder| virtual_name.encode(encoder))?;
+                    })
+                }),
+        })
+    }
+}
+
 impl RealFileName {
-    /// Returns the path suitable for reading from the file system on the local host.
+    /// Returns the path suitable for reading from the file system on the local host,
+    /// if this information exists.
     /// Avoid embedding this in build artifacts; see `stable_name()` for that.
-    pub fn local_path(&self) -> &Path {
+    pub fn local_path(&self) -> Option<&Path> {
         match self {
-            RealFileName::LocalPath(p)
-            | RealFileName::Remapped { local_path: p, virtual_name: _ } => &p,
+            RealFileName::LocalPath(p) => Some(p),
+            RealFileName::Remapped { local_path: p, virtual_name: _ } => {
+                p.as_ref().map(PathBuf::as_path)
+            }
         }
     }
 
-    /// Returns the path suitable for reading from the file system on the local host.
+    /// Returns the path suitable for reading from the file system on the local host,
+    /// if this information exists.
     /// Avoid embedding this in build artifacts; see `stable_name()` for that.
-    pub fn into_local_path(self) -> PathBuf {
+    pub fn into_local_path(self) -> Option<PathBuf> {
         match self {
-            RealFileName::LocalPath(p)
-            | RealFileName::Remapped { local_path: p, virtual_name: _ } => p,
+            RealFileName::LocalPath(p) => Some(p),
+            RealFileName::Remapped { local_path: p, virtual_name: _ } => p,
         }
     }
 
diff --git a/compiler/rustc_span/src/source_map.rs b/compiler/rustc_span/src/source_map.rs
@@ -133,9 +133,6 @@ impl StableSourceFileId {
     fn new_from_name(name: &FileName) -> StableSourceFileId {
         let mut hasher = StableHasher::new();
 
-        // If name was remapped, we need to take both the local path
-        // and stablised path into account, in case two different paths were
-        // mapped to the same
         name.hash(&mut hasher);
 
         StableSourceFileId(hasher.finish())
@@ -954,7 +951,13 @@ impl SourceMap {
     }
     pub fn ensure_source_file_source_present(&self, source_file: Lrc<SourceFile>) -> bool {
         source_file.add_external_src(|| match source_file.name {
-            FileName::Real(ref name) => self.file_loader.read_file(name.local_path()).ok(),
+            FileName::Real(ref name) => {
+                if let Some(local_path) = name.local_path() {
+                    self.file_loader.read_file(local_path).ok()
+                } else {
+                    None
+                }
+            }
             _ => None,
         })
     }
@@ -999,23 +1002,17 @@ impl FilePathMapping {
     fn map_filename_prefix(&self, file: &FileName) -> (FileName, bool) {
         match file {
             FileName::Real(realfile) => {
-                // If the file is the Name variant with only local_path, then clearly we want to map that
-                // to a virtual_name
-                // If the file is already remapped, then we want to map virtual_name further
-                // but we leave local_path alone
-                let path = realfile.stable_name();
-                let (mapped_path, mapped) = self.map_prefix(path.to_path_buf());
-                if mapped {
-                    let mapped_realfile = match realfile {
-                        RealFileName::LocalPath(local_path)
-                        | RealFileName::Remapped { local_path, virtual_name: _ } => {
-                            RealFileName::Remapped {
-                                local_path: local_path.clone(),
-                                virtual_name: mapped_path,
-                            }
+                if let RealFileName::LocalPath(local_path) = realfile {
+                    let (mapped_path, mapped) = self.map_prefix(local_path.to_path_buf());
+                    let realfile = if mapped {
+                        RealFileName::Remapped {
+                            local_path: Some(local_path.clone()),
+                            virtual_name: mapped_path,
                         }
+                    } else {
+                        realfile.clone()
                     };
-                    (FileName::Real(mapped_realfile), mapped)
+                    (FileName::Real(realfile), mapped)
                 } else {
                     unreachable!("attempted to remap an already remapped filename");
                 }
diff --git a/src/librustdoc/doctest.rs b/src/librustdoc/doctest.rs
@@ -852,8 +852,10 @@ impl Collector {
             let filename = source_map.span_to_filename(self.position);
             if let FileName::Real(ref filename) = filename {
                 if let Ok(cur_dir) = env::current_dir() {
-                    if let Ok(path) = filename.local_path().strip_prefix(&cur_dir) {
-                        return path.to_owned().into();
+                    if let Some(local_path) = filename.local_path() {
+                        if let Ok(path) = local_path.strip_prefix(&cur_dir) {
+                            return path.to_owned().into();
+                        }
                     }
                 }
             }
@@ -884,7 +886,14 @@ impl Tester for Collector {
         }
 
         let path = match &filename {
-            FileName::Real(path) => path.local_path().to_path_buf(),
+            FileName::Real(path) => {
+                if let Some(local_path) = path.local_path() {
+                    local_path.to_path_buf()
+                } else {
+                    // Somehow we got the filename from the metadata of another crate, should never happen
+                    PathBuf::from(r"doctest.rs")
+                }
+            }
             _ => PathBuf::from(r"doctest.rs"),
         };
 
diff --git a/src/librustdoc/html/render/context.rs b/src/librustdoc/html/render/context.rs
@@ -290,7 +290,7 @@ impl<'tcx> Context<'tcx> {
 
         // We can safely ignore synthetic `SourceFile`s.
         let file = match item.span(self.tcx()).filename(self.sess()) {
-            FileName::Real(ref path) => path.local_path().to_path_buf(),
+            FileName::Real(ref path) => path.local_path_if_available().to_path_buf(),
             _ => return None,
         };
         let file = &file;
@@ -376,10 +376,17 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
         } = options;
 
         let src_root = match krate.src {
-            FileName::Real(ref p) => match p.local_path().parent() {
-                Some(p) => p.to_path_buf(),
-                None => PathBuf::new(),
-            },
+            FileName::Real(ref p) => {
+                if let Some(local_path) = p.local_path() {
+                    match local_path.parent() {
+                        Some(p) => p.to_path_buf(),
+                        None => PathBuf::new(),
+                    }
+                } else {
+                    // Somehow we got the filename from the metadata of another crate, should never happen
+                    PathBuf::new()
+                }
+            }
             _ => PathBuf::new(),
         };
         // If user passed in `--playground-url` arg, we fill in crate name here
diff --git a/src/librustdoc/html/sources.rs b/src/librustdoc/html/sources.rs
@@ -76,7 +76,13 @@ impl SourceCollector<'_, 'tcx> {
     /// Renders the given filename into its corresponding HTML source file.
     fn emit_source(&mut self, filename: &FileName) -> Result<(), Error> {
         let p = match *filename {
-            FileName::Real(ref file) => file.local_path().to_path_buf(),
+            FileName::Real(ref file) => {
+                if let Some(local_path) = file.local_path() {
+                    local_path.to_path_buf()
+                } else {
+                    return Ok(());
+                }
+            }
             _ => return Ok(()),
         };
         if self.scx.local_sources.contains_key(&*p) {
diff --git a/src/librustdoc/json/conversions.rs b/src/librustdoc/json/conversions.rs
@@ -63,13 +63,17 @@ impl JsonRenderer<'_> {
     fn convert_span(&self, span: clean::Span) -> Option<Span> {
         match span.filename(self.sess()) {
             rustc_span::FileName::Real(name) => {
-                let hi = span.hi(self.sess());
-                let lo = span.lo(self.sess());
-                Some(Span {
-                    filename: name.into_local_path(),
-                    begin: (lo.line, lo.col.to_usize()),
-                    end: (hi.line, hi.col.to_usize()),
-                })
+                if let Some(local_path) = name.into_local_path() {
+                    let hi = span.hi(self.sess());
+                    let lo = span.lo(self.sess());
+                    Some(Span {
+                        filename: local_path,
+                        begin: (lo.line, lo.col.to_usize()),
+                        end: (hi.line, hi.col.to_usize()),
+                    })
+                } else {
+                    None
+                }
             }
             _ => None,
         }

Original file line number	Diff line number	Diff line change
`@@ -497,9 +497,10 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {`
`497`	`497`	`RealFileName::LocalPath(local_path) => {`
`498`	`498`	`Path::new(&working_dir).join(local_path).into()`
`499`	`499`	`}`
`500`		`- RealFileName::Remapped { local_path, virtual_name } => {`
	`500`	`+ RealFileName::Remapped { local_path: _, virtual_name } => {`
`501`	`501`	`FileName::Real(RealFileName::Remapped {`
`502`		`- local_path: Path::new(&working_dir).join(local_path),`
	`502`	`+ // We do not want any local path to be exported into metadata`
	`503`	`+ local_path: None,`
`503`	`504`	`virtual_name: virtual_name.clone(),`
`504`	`505`	`})`
`505`	`506`	`}`