From 905d738b1af874e248b9fe7fe4b115458975d401 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 29 May 2020 11:47:17 -0400 Subject: [PATCH 1/4] `StableSourceFileId::new_from_pieces` does not need to be public. (and I want to discourage further use of it if possible.) --- src/librustc_span/source_map.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_span/source_map.rs b/src/librustc_span/source_map.rs index 51f554176630..dc85f7990b92 100644 --- a/src/librustc_span/source_map.rs +++ b/src/librustc_span/source_map.rs @@ -95,7 +95,7 @@ impl StableSourceFileId { ) } - pub fn new_from_pieces( + fn new_from_pieces( name: &FileName, name_was_remapped: bool, unmapped_path: Option<&FileName>, From da09fd3db0c71680e16311140d07e8f1e079af82 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 29 May 2020 11:31:55 -0400 Subject: [PATCH 2/4] Split payload of FileName::Real to track both real and virutalized paths. Such splits arise from metadata refs into libstd. This way, we can (in a follow on commit) continue to emit the virtual name into things like the like the StableSourceFileId that ends up in incremetnal build artifacts, while still using the devirtualized file path when we want to access the file. Note that this commit is intended to be a refactoring; the actual fix to the bug in question is in a follow-on commit. --- src/librustc_expand/base.rs | 2 +- src/librustc_expand/expand.rs | 2 +- src/librustc_expand/module.rs | 5 ++- src/librustc_expand/proc_macro_server.rs | 3 +- src/librustc_interface/passes.rs | 11 ++++-- src/librustc_metadata/rmeta/decoder.rs | 25 +++++++----- src/librustc_metadata/rmeta/encoder.rs | 1 + src/librustc_save_analysis/span_utils.rs | 5 ++- src/librustc_span/lib.rs | 48 ++++++++++++++++++++++-- src/librustc_span/source_map.rs | 25 ++++++++---- src/librustdoc/html/render.rs | 5 ++- src/librustdoc/html/render/cache.rs | 2 +- src/librustdoc/html/sources.rs | 4 +- src/librustdoc/test.rs | 4 +- 14 files changed, 103 insertions(+), 39 deletions(-) diff --git a/src/librustc_expand/base.rs b/src/librustc_expand/base.rs index 649aac488fcb..3a06770e31bd 100644 --- a/src/librustc_expand/base.rs +++ b/src/librustc_expand/base.rs @@ -1095,7 +1095,7 @@ impl<'a> ExtCtxt<'a> { if !path.is_absolute() { let callsite = span.source_callsite(); let mut result = match self.source_map().span_to_unmapped_path(callsite) { - FileName::Real(path) => path, + FileName::Real(name) => name.into_local_path(), FileName::DocTest(path, _) => path, other => { return Err(self.struct_span_err( diff --git a/src/librustc_expand/expand.rs b/src/librustc_expand/expand.rs index b505302f6250..25b94ff2cbeb 100644 --- a/src/librustc_expand/expand.rs +++ b/src/librustc_expand/expand.rs @@ -340,7 +340,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> { let mut module = ModuleData { mod_path: vec![Ident::from_str(&self.cx.ecfg.crate_name)], directory: match self.cx.source_map().span_to_unmapped_path(krate.span) { - FileName::Real(path) => path, + FileName::Real(name) => name.into_local_path(), other => PathBuf::from(other.to_string()), }, }; diff --git a/src/librustc_expand/module.rs b/src/librustc_expand/module.rs index 9bb2b57b7f59..82215c7297ed 100644 --- a/src/librustc_expand/module.rs +++ b/src/librustc_expand/module.rs @@ -71,7 +71,7 @@ crate fn parse_external_mod( // Extract the directory path for submodules of `module`. let path = sess.source_map().span_to_unmapped_path(module.inner); let mut path = match path { - FileName::Real(path) => path, + FileName::Real(name) => name.into_local_path(), other => PathBuf::from(other.to_string()), }; path.pop(); @@ -189,7 +189,8 @@ fn error_cannot_declare_mod_here<'a, T>( let mut err = sess.span_diagnostic.struct_span_err(span, "cannot declare a new module at this location"); if !span.is_dummy() { - if let FileName::Real(src_path) = sess.source_map().span_to_filename(span) { + if let FileName::Real(src_name) = sess.source_map().span_to_filename(span) { + let src_path = src_name.into_local_path(); if let Some(stem) = src_path.file_stem() { let mut dest_path = src_path.clone(); dest_path.set_file_name(stem); diff --git a/src/librustc_expand/proc_macro_server.rs b/src/librustc_expand/proc_macro_server.rs index b9693c2c8627..36af83711f7e 100644 --- a/src/librustc_expand/proc_macro_server.rs +++ b/src/librustc_expand/proc_macro_server.rs @@ -602,7 +602,8 @@ impl server::SourceFile for Rustc<'_> { } fn path(&mut self, file: &Self::SourceFile) -> String { match file.name { - FileName::Real(ref path) => path + FileName::Real(ref name) => name + .local_path() .to_str() .expect("non-UTF8 file path in `proc_macro::SourceFile::path`") .to_string(), diff --git a/src/librustc_interface/passes.rs b/src/librustc_interface/passes.rs index 214701db724e..9f1f38efb484 100644 --- a/src/librustc_interface/passes.rs +++ b/src/librustc_interface/passes.rs @@ -33,7 +33,7 @@ use rustc_session::output::{filename_for_input, filename_for_metadata}; use rustc_session::search_paths::PathKind; use rustc_session::Session; use rustc_span::symbol::Symbol; -use rustc_span::FileName; +use rustc_span::{FileName, RealFileName}; use rustc_trait_selection::traits; use rustc_typeck as typeck; @@ -569,13 +569,16 @@ fn write_out_deps( for cnum in resolver.cstore().crates_untracked() { let source = resolver.cstore().crate_source_untracked(cnum); if let Some((path, _)) = source.dylib { - files.push(escape_dep_filename(&FileName::Real(path))); + let file_name = FileName::Real(RealFileName::Named(path)); + files.push(escape_dep_filename(&file_name)); } if let Some((path, _)) = source.rlib { - files.push(escape_dep_filename(&FileName::Real(path))); + let file_name = FileName::Real(RealFileName::Named(path)); + files.push(escape_dep_filename(&file_name)); } if let Some((path, _)) = source.rmeta { - files.push(escape_dep_filename(&FileName::Real(path))); + let file_name = FileName::Real(RealFileName::Named(path)); + files.push(escape_dep_filename(&file_name)); } } }); diff --git a/src/librustc_metadata/rmeta/decoder.rs b/src/librustc_metadata/rmeta/decoder.rs index 2b292b35c159..f5a9dceb7829 100644 --- a/src/librustc_metadata/rmeta/decoder.rs +++ b/src/librustc_metadata/rmeta/decoder.rs @@ -1471,15 +1471,22 @@ impl<'a, 'tcx> CrateMetadataRef<'a> { if let Some(virtual_dir) = virtual_rust_source_base_dir { if let Some(real_dir) = &sess.real_rust_source_base_dir { - if let rustc_span::FileName::Real(path) = name { - if let Ok(rest) = path.strip_prefix(virtual_dir) { - let new_path = real_dir.join(rest); - debug!( - "try_to_translate_virtual_to_real: `{}` -> `{}`", - path.display(), - new_path.display(), - ); - *path = new_path; + if let rustc_span::FileName::Real(old_name) = name { + if let rustc_span::RealFileName::Named(one_path) = old_name { + if let Ok(rest) = one_path.strip_prefix(virtual_dir) { + let virtual_name = one_path.clone(); + let new_path = real_dir.join(rest); + debug!( + "try_to_translate_virtual_to_real: `{}` -> `{}`", + virtual_name.display(), + new_path.display(), + ); + let new_name = rustc_span::RealFileName::Devirtualized { + local_path: new_path, + virtual_name, + }; + *old_name = new_name; + } } } } diff --git a/src/librustc_metadata/rmeta/encoder.rs b/src/librustc_metadata/rmeta/encoder.rs index 91fbfcc0133e..b47a166feb52 100644 --- a/src/librustc_metadata/rmeta/encoder.rs +++ b/src/librustc_metadata/rmeta/encoder.rs @@ -396,6 +396,7 @@ impl<'tcx> EncodeContext<'tcx> { // any relative paths are potentially relative to a // wrong directory. FileName::Real(ref name) => { + let name = name.local_path(); let mut adapted = (**source_file).clone(); adapted.name = Path::new(&working_dir).join(name).into(); adapted.name_hash = { diff --git a/src/librustc_save_analysis/span_utils.rs b/src/librustc_save_analysis/span_utils.rs index 6620941c4404..d5f992b0de05 100644 --- a/src/librustc_save_analysis/span_utils.rs +++ b/src/librustc_save_analysis/span_utils.rs @@ -16,12 +16,13 @@ impl<'a> SpanUtils<'a> { pub fn make_filename_string(&self, file: &SourceFile) -> String { match &file.name { - FileName::Real(path) if !file.name_was_remapped => { + FileName::Real(name) if !file.name_was_remapped => { + let path = name.local_path(); if path.is_absolute() { self.sess .source_map() .path_mapping() - .map_prefix(path.clone()) + .map_prefix(path.into()) .0 .display() .to_string() diff --git a/src/librustc_span/lib.rs b/src/librustc_span/lib.rs index 58cdb87158af..f18a863d7208 100644 --- a/src/librustc_span/lib.rs +++ b/src/librustc_span/lib.rs @@ -53,7 +53,7 @@ use std::cmp::{self, Ordering}; use std::fmt; use std::hash::Hash; use std::ops::{Add, Sub}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::str::FromStr; use md5::Md5; @@ -81,11 +81,45 @@ impl Globals { scoped_tls::scoped_thread_local!(pub static GLOBALS: Globals); +/// FIXME: Perhaps this should not implement Rustc{Decodable, Encodable} +#[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd, Hash, RustcDecodable, RustcEncodable)] +#[derive(HashStable_Generic)] +pub enum RealFileName { + Named(PathBuf), + /// For de-virtualized paths (namely paths into libstd that have been mapped + /// to the appropriate spot on the local host's file system), + Devirtualized { + /// `local_path` is the (host-dependent) local path to the file. + local_path: PathBuf, + /// `virtual_name` is the stable path rustc will store internally within + /// build artifacts. + virtual_name: PathBuf, + }, +} + +impl RealFileName { + /// Returns the path suitable for reading from the file system on the local host. + pub fn local_path(&self) -> &Path { + match self { + RealFileName::Named(p) + | RealFileName::Devirtualized { local_path: p, virtual_name: _ } => &p, + } + } + + /// Returns the path suitable for reading from the file system on the local host. + pub fn into_local_path(self) -> PathBuf { + match self { + RealFileName::Named(p) + | RealFileName::Devirtualized { local_path: p, virtual_name: _ } => p, + } + } +} + /// Differentiates between real files and common virtual files. #[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd, Hash, RustcDecodable, RustcEncodable)] #[derive(HashStable_Generic)] pub enum FileName { - Real(PathBuf), + Real(RealFileName), /// Call to `quote!`. QuoteExpansion(u64), /// Command line. @@ -107,7 +141,13 @@ impl std::fmt::Display for FileName { fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { use FileName::*; match *self { - Real(ref path) => write!(fmt, "{}", path.display()), + Real(RealFileName::Named(ref path)) => write!(fmt, "{}", path.display()), + // FIXME: might be nice to display both compoments of Devirtualized. + // But for now (to backport fix for issue #70924), best to not + // perturb diagnostics so its obvious test suite still works. + Real(RealFileName::Devirtualized { ref local_path, virtual_name: _ }) => { + write!(fmt, "{}", local_path.display()) + } QuoteExpansion(_) => write!(fmt, ""), MacroExpansion(_) => write!(fmt, ""), Anon(_) => write!(fmt, ""), @@ -123,7 +163,7 @@ impl std::fmt::Display for FileName { impl From for FileName { fn from(p: PathBuf) -> Self { assert!(!p.to_string_lossy().ends_with('>')); - FileName::Real(p) + FileName::Real(RealFileName::Named(p)) } } diff --git a/src/librustc_span/source_map.rs b/src/librustc_span/source_map.rs index dc85f7990b92..c432de3e847c 100644 --- a/src/librustc_span/source_map.rs +++ b/src/librustc_span/source_map.rs @@ -235,7 +235,7 @@ impl SourceMap { fn try_new_source_file( &self, - filename: FileName, + mut filename: FileName, src: String, ) -> Result, OffsetOverflowError> { // The path is used to determine the directory for loading submodules and @@ -245,13 +245,22 @@ impl SourceMap { // be empty, so the working directory will be used. let unmapped_path = filename.clone(); - let (filename, was_remapped) = match filename { - FileName::Real(filename) => { - let (filename, was_remapped) = self.path_mapping.map_prefix(filename); - (FileName::Real(filename), was_remapped) + let was_remapped; + if let FileName::Real(real_filename) = &mut filename { + match real_filename { + RealFileName::Named(path_to_be_remapped) + | RealFileName::Devirtualized { + local_path: path_to_be_remapped, + virtual_name: _, + } => { + let mapped = self.path_mapping.map_prefix(path_to_be_remapped.clone()); + was_remapped = mapped.1; + *path_to_be_remapped = mapped.0; + } } - other => (other, false), - }; + } else { + was_remapped = false; + } let file_id = StableSourceFileId::new_from_pieces(&filename, was_remapped, Some(&unmapped_path)); @@ -998,7 +1007,7 @@ impl SourceMap { } pub fn ensure_source_file_source_present(&self, source_file: Lrc) -> bool { source_file.add_external_src(|| match source_file.name { - FileName::Real(ref name) => self.file_loader.read_file(name).ok(), + FileName::Real(ref name) => self.file_loader.read_file(name.local_path()).ok(), _ => None, }) } diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index cd49da88d965..f6fb10b01809 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -473,7 +473,7 @@ pub fn run( } = options; let src_root = match krate.src { - FileName::Real(ref p) => match p.parent() { + FileName::Real(ref p) => match p.local_path().parent() { Some(p) => p.to_path_buf(), None => PathBuf::new(), }, @@ -1621,9 +1621,10 @@ impl Context { // We can safely ignore synthetic `SourceFile`s. let file = match item.source.filename { - FileName::Real(ref path) => path, + FileName::Real(ref path) => path.local_path().to_path_buf(), _ => return None, }; + let file = &file; let (krate, path) = if item.source.cnum == LOCAL_CRATE { if let Some(path) = self.shared.local_sources.get(file) { diff --git a/src/librustdoc/html/render/cache.rs b/src/librustdoc/html/render/cache.rs index 57d385de3209..225940773413 100644 --- a/src/librustdoc/html/render/cache.rs +++ b/src/librustdoc/html/render/cache.rs @@ -173,7 +173,7 @@ impl Cache { // Cache where all our extern crates are located for &(n, ref e) in &krate.externs { let src_root = match e.src { - FileName::Real(ref p) => match p.parent() { + FileName::Real(ref p) => match p.local_path().parent() { Some(p) => p.to_path_buf(), None => PathBuf::new(), }, diff --git a/src/librustdoc/html/sources.rs b/src/librustdoc/html/sources.rs index c5f44baced2e..018c0e82c456 100644 --- a/src/librustdoc/html/sources.rs +++ b/src/librustdoc/html/sources.rs @@ -67,10 +67,10 @@ impl<'a> SourceCollector<'a> { /// 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, + FileName::Real(ref file) => file.local_path().to_path_buf(), _ => return Ok(()), }; - if self.scx.local_sources.contains_key(&**p) { + if self.scx.local_sources.contains_key(&*p) { // We've already emitted this source return Ok(()); } diff --git a/src/librustdoc/test.rs b/src/librustdoc/test.rs index 284e6d421ee2..ad7f2c112c15 100644 --- a/src/librustdoc/test.rs +++ b/src/librustdoc/test.rs @@ -688,7 +688,7 @@ 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.strip_prefix(&cur_dir) { + if let Ok(path) = filename.local_path().strip_prefix(&cur_dir) { return path.to_owned().into(); } } @@ -718,7 +718,7 @@ impl Tester for Collector { // FIXME(#44940): if doctests ever support path remapping, then this filename // needs to be the result of `SourceMap::span_to_unmapped_path`. let path = match &filename { - FileName::Real(path) => path.clone(), + FileName::Real(path) => path.local_path().to_path_buf(), _ => PathBuf::from(r"doctest.rs"), }; From 5e5a3d58678d70ad49b87251a12d11dea52e69b0 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 29 May 2020 14:04:03 -0400 Subject: [PATCH 3/4] Use the virtual name for libstd files in StableSourceFileId and also in the encoded build artifacts. Fix #70924. --- src/librustc_metadata/rmeta/encoder.rs | 2 +- src/librustc_span/lib.rs | 13 +++++++++++++ src/librustc_span/source_map.rs | 11 ++++++++++- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/librustc_metadata/rmeta/encoder.rs b/src/librustc_metadata/rmeta/encoder.rs index b47a166feb52..9964c9c94c95 100644 --- a/src/librustc_metadata/rmeta/encoder.rs +++ b/src/librustc_metadata/rmeta/encoder.rs @@ -396,7 +396,7 @@ impl<'tcx> EncodeContext<'tcx> { // any relative paths are potentially relative to a // wrong directory. FileName::Real(ref name) => { - let name = name.local_path(); + let name = name.stable_name(); let mut adapted = (**source_file).clone(); adapted.name = Path::new(&working_dir).join(name).into(); adapted.name_hash = { diff --git a/src/librustc_span/lib.rs b/src/librustc_span/lib.rs index f18a863d7208..e17fe1fa782c 100644 --- a/src/librustc_span/lib.rs +++ b/src/librustc_span/lib.rs @@ -99,6 +99,7 @@ pub enum RealFileName { impl RealFileName { /// Returns the path suitable for reading from the file system on the local host. + /// Avoid embedding this in build artifacts; see `stable_name` for that. pub fn local_path(&self) -> &Path { match self { RealFileName::Named(p) @@ -107,12 +108,24 @@ impl RealFileName { } /// Returns the path suitable for reading from the file system on the local host. + /// Avoid embedding this in build artifacts; see `stable_name` for that. pub fn into_local_path(self) -> PathBuf { match self { RealFileName::Named(p) | RealFileName::Devirtualized { local_path: p, virtual_name: _ } => p, } } + + /// Returns the path suitable for embedding into build artifacts. Note that + /// a virtualized path will not correspond to a valid file system path; see + /// `local_path` for something that is more likely to return paths into the + /// local host file system. + pub fn stable_name(&self) -> &Path { + match self { + RealFileName::Named(p) + | RealFileName::Devirtualized { local_path: _, virtual_name: p } => &p, + } + } } /// Differentiates between real files and common virtual files. diff --git a/src/librustc_span/source_map.rs b/src/librustc_span/source_map.rs index c432de3e847c..c33c1dd29cb7 100644 --- a/src/librustc_span/source_map.rs +++ b/src/librustc_span/source_map.rs @@ -86,6 +86,8 @@ impl FileLoader for RealFileLoader { #[derive(Copy, Clone, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable, Debug)] pub struct StableSourceFileId(u128); +// FIXME: we need a more globally consistent approach to the problem solved by +// StableSourceFileId, perhaps built atop source_file.name_hash. impl StableSourceFileId { pub fn new(source_file: &SourceFile) -> StableSourceFileId { StableSourceFileId::new_from_pieces( @@ -102,7 +104,14 @@ impl StableSourceFileId { ) -> StableSourceFileId { let mut hasher = StableHasher::new(); - name.hash(&mut hasher); + if let FileName::Real(real_name) = name { + // rust-lang/rust#70924: Use the stable (virtualized) name when + // available. (We do not want artifacts from transient file system + // paths for libstd to leak into our build artifacts.) + real_name.stable_name().hash(&mut hasher) + } else { + name.hash(&mut hasher); + } name_was_remapped.hash(&mut hasher); unmapped_path.hash(&mut hasher); From a8e4236edc1e118ccb6c3f3a8d0139e4cd90b5b8 Mon Sep 17 00:00:00 2001 From: Felix S Klock II Date: Sat, 30 May 2020 08:19:35 -0400 Subject: [PATCH 4/4] add fixme suggested by eddyb --- src/librustc_span/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/librustc_span/lib.rs b/src/librustc_span/lib.rs index e17fe1fa782c..94795e635586 100644 --- a/src/librustc_span/lib.rs +++ b/src/librustc_span/lib.rs @@ -81,7 +81,10 @@ impl Globals { scoped_tls::scoped_thread_local!(pub static GLOBALS: Globals); -/// FIXME: Perhaps this should not implement Rustc{Decodable, Encodable} +// FIXME: Perhaps this should not implement Rustc{Decodable, Encodable} +// +// 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, RustcDecodable, RustcEncodable)] #[derive(HashStable_Generic)] pub enum RealFileName {