Don't use remapped path when loading modules and include files #44940

philipc · 2017-09-30T06:50:38Z

rust-highfive · 2017-09-30T06:50:53Z

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.

philipc · 2017-10-01T03:03:19Z

./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?

michaelwoerister · 2017-10-02T09:13:33Z

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

michaelwoerister · 2017-10-02T12:42:27Z

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!

philipc · 2017-10-02T12:50:55Z

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.

michaelwoerister · 2017-10-02T13:21:02Z

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.

carols10cents · 2017-10-02T16:25:31Z

r? @michaelwoerister

philipc · 2017-10-03T12:56:56Z

I've updated FileMap::path as requested.

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

michaelwoerister · 2017-10-04T08:55:10Z

@bors r+

Excellent, thanks a lot, @philipc!

bors · 2017-10-04T08:55:11Z

📌 Commit 9bbd7a3 has been approved by michaelwoerister

bors · 2017-10-05T05:16:47Z

⌛ Testing commit 9bbd7a3 with merge a0db04b...

@michaelwoerister

Don't use remapped path when loading modules and include files Fixes bug reported in #41555 (comment). cc @michaelwoerister

bors · 2017-10-05T08:53:36Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing a0db04b to master...

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

Don't use remapped path when loading modules and include files

c27a82f

rust-highfive assigned nikomatsakis Sep 30, 2017

philipc mentioned this pull request Oct 1, 2017

Improvements to proc_macro::Span API #43604

Merged

2 tasks

rust-highfive assigned michaelwoerister and unassigned nikomatsakis Oct 2, 2017

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2017

philipc added 2 commits October 3, 2017 19:47

Rename FileMap::path and change to an Option

3a225c7

Add fixme regarding remapping paths for doctests

9bbd7a3

bors added a commit that referenced this pull request Oct 5, 2017

Auto merge of #44940 - philipc:remap-path, r=michaelwoerister

a0db04b

Don't use remapped path when loading modules and include files Fixes bug reported in #41555 (comment). cc @michaelwoerister

bors merged commit 9bbd7a3 into rust-lang:master Oct 5, 2017

philipc deleted the remap-path branch October 5, 2017 08:56

johnthagen mentioned this pull request Oct 7, 2017

Tracking issue for Path Prefix Remapping #41555

Closed

cbeuw mentioned this pull request Apr 3, 2021

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use remapped path when loading modules and include files #44940

Don't use remapped path when loading modules and include files #44940

philipc commented Sep 30, 2017

rust-highfive commented Sep 30, 2017

philipc commented Oct 1, 2017

michaelwoerister commented Oct 2, 2017

michaelwoerister commented Oct 2, 2017

philipc commented Oct 2, 2017

michaelwoerister commented Oct 2, 2017

carols10cents commented Oct 2, 2017

philipc commented Oct 3, 2017

michaelwoerister commented Oct 4, 2017

bors commented Oct 4, 2017

bors commented Oct 5, 2017

bors commented Oct 5, 2017

Don't use remapped path when loading modules and include files #44940

Don't use remapped path when loading modules and include files #44940

Conversation

philipc commented Sep 30, 2017

rust-highfive commented Sep 30, 2017

philipc commented Oct 1, 2017

michaelwoerister commented Oct 2, 2017

michaelwoerister commented Oct 2, 2017

philipc commented Oct 2, 2017

michaelwoerister commented Oct 2, 2017

carols10cents commented Oct 2, 2017

philipc commented Oct 3, 2017

michaelwoerister commented Oct 4, 2017

bors commented Oct 4, 2017

bors commented Oct 5, 2017

bors commented Oct 5, 2017