Don't use remapped path when loading modules and include files by philipc · Pull Request #44940 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation12 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

./x.py test src/test/run-make passes for me locally, so I don't know why travis failed.

There's at least one span_to_filename usage in librustdoc that is still wrong. The problem with it is that the result is used for both input and display purposes, so the change to fix it will be more extensive, probably requiring passing around both the original path and the remapped name. I can work on that if this sounds like the right approach. There's a couple more span_to_filename uses in librustdoc that I haven't investigated fully.

Also, anywhere that converts strings to PathBuf seems error prone, since we don't know if the string is a real path, or if it was lossily converted from a path, or if it isn't a path at all. In this PR I've moved an occurrence of this from the callers of span_to_filename into new_filemap. I initially tried to avoid this completely, but that was going to be a much larger change to all the callers of new_filemap. Is this worth pursuing?

Thanks a lot for the PR, @philipc! I'll take a closer look shortly.

Looks like an excellent start! Some comments:

./x.py test src/test/run-make passes for me locally, so I don't know why travis failed.

This seems to be a spurious failure probably not related to your PR: #43402

There's at least one span_to_filename usage in librustdoc that is still wrong. The problem with it is that the result is used for both input and display purposes, so the change to fix it will be more extensive, probably requiring passing around both the original path and the remapped name. I can work on that if this sounds like the right approach. There's a couple more span_to_filename uses in librustdoc that I haven't investigated fully.

Could you just add a FIXME comment to those and link to this PR? I don't think it's possible to invoke rustdoc with path remapping anyway.

Also, anywhere that converts strings to PathBuf seems error prone, since we don't know if the string is a real path, or if it was lossily converted from a path, or if it isn't a path at all. In this PR I've moved an occurrence of this from the callers of span_to_filename into new_filemap. I initially tried to avoid this completely, but that was going to be a much larger change to all the callers of new_filemap. Is this worth pursuing?

Let's keep this PR focussed, I'd say. The other changes can still be done later.

Some minor comments regarding the implementation:

• I would prefer a different name for the path field, for example fs_path or unmapped_path -- something blunt :) (and CodeMap::span_to_path() should then also be named accordingly)
• Intended or not, this PR does the right thing for reproducible builds in that it does not serialize the path field into crate metadata. However, I'd prefer if the field was an Option<Path> to make this more explicit. span_to_path() can call .expect("CodeMap::span_to_path() called for imported file-map?") to make things easy.

Otherwise this looks great already!

I don't think it's possible to invoke rustdoc with path remapping anyway.

Is this something that should be possible though? Do distributions want to package the generated doc, and have the src links point to the remapped location?

I'll address the other comments, thanks for reviewing.

Is this something that should be possible though? Do distributions want to package the generated doc, and have the src links point to the remapped location?

I could imagine so. But since the current rustdoc implementation will be replaced by one that coupled less tightly to the compiler internals, I don't think we need to solve this here.

I've updated FileMap::path as requested.

Also added a FIXME for doctests, but not sure how useful it is.

📌 Commit 9bbd7a3 has been approved by michaelwoerister

bors added a commit that referenced this pull request

Oct 5, 2017

cbeuw mentioned this pull request

Apr 3, 2021

bors added a commit to rust-lang-ci/rust that referenced this pull request

May 12, 2021

Fix --remap-path-prefix not correctly remapping rust-src component paths and unify handling of path mapping with virtualized paths

This PR fixes rust-lang#73167 ("Binaries end up containing path to the rust-src component despite --remap-path-prefix") by preventing real local filesystem paths from reaching compilation output if the path is supposed to be remapped.

RealFileName::Named introduced in rust-lang#72767 is now renamed as LocalPath, because this variant wraps a (most likely) valid local filesystem path.

RealFileName::Devirtualized is renamed as Remapped to be used for remapped path from a real path via --remap-path-prefix argument, as well as real path inferred from a virtualized (during compiler bootstrapping) /rustc/... path. The local_path field is now an Option<PathBuf>, as it will be set to None before serialisation, so it never reaches any build output. Attempting to serialise a non-None local_path will cause an assertion faliure.

When a path is remapped, a RealFileName::Remapped variant is created. The original path is preserved in local_path field and the remapped path is saved in virtual_name field. Previously, the local_path is directly modified which goes against its purpose of "suitable for reading from the file system on the local host".

rustc_span::SourceFile's fields unmapped_path (introduced by rust-lang#44940) and name_was_remapped (introduced by rust-lang#41508 when --remap-path-prefix feature originally added) are removed, as these two pieces of information can be inferred from the name field: if it's anything other than a FileName::Real(_), or if it is a FileName::Real(RealFileName::LocalPath(_)), then clearly name_was_remapped would've been false and unmapped_path would've been None. If it is a FileName::Real(RealFileName::Remapped{local_path, virtual_name}), then name_was_remapped would've been true and unmapped_path would've been Some(local_path).

cc @eddyb who implemented /rustc/... path devirtualisation

flip1995 pushed a commit to flip1995/rust that referenced this pull request

May 20, 2021

Fix --remap-path-prefix not correctly remapping rust-src component paths and unify handling of path mapping with virtualized paths

This PR fixes rust-lang#73167 ("Binaries end up containing path to the rust-src component despite --remap-path-prefix") by preventing real local filesystem paths from reaching compilation output if the path is supposed to be remapped.

RealFileName::Named introduced in rust-lang#72767 is now renamed as LocalPath, because this variant wraps a (most likely) valid local filesystem path.

RealFileName::Devirtualized is renamed as Remapped to be used for remapped path from a real path via --remap-path-prefix argument, as well as real path inferred from a virtualized (during compiler bootstrapping) /rustc/... path. The local_path field is now an Option<PathBuf>, as it will be set to None before serialisation, so it never reaches any build output. Attempting to serialise a non-None local_path will cause an assertion faliure.

When a path is remapped, a RealFileName::Remapped variant is created. The original path is preserved in local_path field and the remapped path is saved in virtual_name field. Previously, the local_path is directly modified which goes against its purpose of "suitable for reading from the file system on the local host".

rustc_span::SourceFile's fields unmapped_path (introduced by rust-lang#44940) and name_was_remapped (introduced by rust-lang#41508 when --remap-path-prefix feature originally added) are removed, as these two pieces of information can be inferred from the name field: if it's anything other than a FileName::Real(_), or if it is a FileName::Real(RealFileName::LocalPath(_)), then clearly name_was_remapped would've been false and unmapped_path would've been None. If it is a FileName::Real(RealFileName::Remapped{local_path, virtual_name}), then name_was_remapped would've been true and unmapped_path would've been Some(local_path).

cc @eddyb who implemented /rustc/... path devirtualisation

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request

May 27, 2021

Fix --remap-path-prefix not correctly remapping rust-src component paths and unify handling of path mapping with virtualized paths

This PR fixes rust-lang#73167 ("Binaries end up containing path to the rust-src component despite --remap-path-prefix") by preventing real local filesystem paths from reaching compilation output if the path is supposed to be remapped.

RealFileName::Named introduced in rust-lang#72767 is now renamed as LocalPath, because this variant wraps a (most likely) valid local filesystem path.

RealFileName::Devirtualized is renamed as Remapped to be used for remapped path from a real path via --remap-path-prefix argument, as well as real path inferred from a virtualized (during compiler bootstrapping) /rustc/... path. The local_path field is now an Option<PathBuf>, as it will be set to None before serialisation, so it never reaches any build output. Attempting to serialise a non-None local_path will cause an assertion faliure.

When a path is remapped, a RealFileName::Remapped variant is created. The original path is preserved in local_path field and the remapped path is saved in virtual_name field. Previously, the local_path is directly modified which goes against its purpose of "suitable for reading from the file system on the local host".

rustc_span::SourceFile's fields unmapped_path (introduced by rust-lang#44940) and name_was_remapped (introduced by rust-lang#41508 when --remap-path-prefix feature originally added) are removed, as these two pieces of information can be inferred from the name field: if it's anything other than a FileName::Real(_), or if it is a FileName::Real(RealFileName::LocalPath(_)), then clearly name_was_remapped would've been false and unmapped_path would've been None. If it is a FileName::Real(RealFileName::Remapped{local_path, virtual_name}), then name_was_remapped would've been true and unmapped_path would've been Some(local_path).

cc @eddyb who implemented /rustc/... path devirtualisation