From 9e0426d7842c4a603237789b59e6c491d2dd3b4a Mon Sep 17 00:00:00 2001 From: Andy Wang Date: Fri, 9 Apr 2021 00:54:51 +0100 Subject: [PATCH] Make local_path in RealFileName::Remapped Option to be removed in exported metadata --- compiler/rustc_expand/src/base.rs | 4 +- compiler/rustc_expand/src/expand.rs | 4 +- .../rustc_expand/src/proc_macro_server.rs | 1 + compiler/rustc_metadata/src/rmeta/decoder.rs | 2 +- compiler/rustc_metadata/src/rmeta/encoder.rs | 5 +- compiler/rustc_span/src/lib.rs | 66 +++++++++++++++---- compiler/rustc_span/src/source_map.rs | 35 +++++----- src/librustdoc/doctest.rs | 15 ++++- src/librustdoc/html/render/context.rs | 17 +++-- src/librustdoc/html/sources.rs | 8 ++- src/librustdoc/json/conversions.rs | 18 +++-- 11 files changed, 123 insertions(+), 52 deletions(-) diff --git a/compiler/rustc_expand/src/base.rs b/compiler/rustc_expand/src/base.rs index f88b05e6c8f..5c83d6c7ad5 100644 --- 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 index 7bb29d20e4a..03910f4e18d 100644 --- 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 index 4089dc88b7b..f91c0d83138 100644 --- 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 index 74325c62ed3..4cb90d5a6d6 100644 --- 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 index fbae2278f1e..9a398d902a7 100644 --- 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 index 44617b5aa00..e5e6f31886c 100644 --- 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, /// `virtual_name` is the stable path rustc will store internally within /// build artifacts. virtual_name: PathBuf, }, } +impl Hash for RealFileName { + fn hash(&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 Encodable 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 { 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 index 03be11f828b..4e60d071c68 100644 --- 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) -> 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 index 9b945cd42cf..576f27f8f5c 100644 --- 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 index 293c0a40fa7..95f3a32e41f 100644 --- 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 index 3d4d8df0a71..9753179b6e1 100644 --- 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 index 88e369e1bab..e0009bf264a 100644 --- 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 { 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, }