--remap-path-prefix no longer works for compiler messages

[nicholasbishop]

Code

Create a directory and put a Rust file with bad syntax in it:

mkdir src
echo bad > src/test.rs

Then compile it with:

rustc --remap-path-prefix=src=MODIFIED src/test.rs

I expected to see output like this:

error: expected one of `!` or `::`, found `<eof>`
 --> MODIFIED/test.rs:1:1
[snip]

Instead, the path is not modified and I get:

error: expected one of `!` or `::`, found `<eof>`
 --> src/test.rs:1:1
[snip]

Version it worked on

rustc 1.53.0 (53cb7b0 2021-06-17)

Version with regression

rustc 1.54.0 (a178d03 2021-07-26)

[nicholasbishop]

Bisected:

searched nightlies: from nightly-2021-03-17 to nightly-2021-08-03
regressed nightly: nightly-2021-05-13
searched commits: from 5c02926 to 21e92b9
regressed commit: e1ff91f

bisected with cargo-bisect-rustc v0.6.0

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start=2021-03-17 --preserve --script=./bisect-check.py 

[apiraino]

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-low

[inquisitivecrystal]

After further discussion, there appears to be a consensus in the Prioritization Working Group that this should be P-medium for the moment. Sorry for the noise!

[dtolnay]

FYI @cbeuw @michaelwoerister, since this bisects to #83813. Would one of you be able to take a look?

[cbeuw]

I thought this is the intended behaviour? The local path is not embedded into a compilation output. Compiler diagnostics should give you the local path

[dtolnay]

For Buck's use case of --remap-path-prefix, I believe we want the remapped path in the diagnostic, rather than original.

Before:

  --> eden/mononoke/commit_rewriting/megarepo/src/history_fixup_delete.rs:55:13

After regression:

  --> buck-out/dev/bin/aab7ed39/eden/mononoke/commit_rewriting/megarepo/megarepolib#platform009-clang,rlib-pic/eden/mononoke/commit_rewriting/megarepo/src/history_fixup_delete.rs:55:13

Context: in a monorepo codebase, all the source files have a canonical path relative to the root of the monorepo. Those are the files that the engineer edits as part of day-to-day development. For rustc invocations though, the paths fed into the compiler by Buck are nested somewhere inside a Buck-managed directory. This is necessary for example in order to overlay generated sources into that directory side by side with the handwritten sources.

[inquisitivecrystal]

I thought this is the intended behaviour? The local path is not embedded into a compilation output. Compiler diagnostics should give you the local path

wg-prioritization got confused about this too, which is part of why we had so much trouble prioritizing this correctly. @nicholasbishop pointed out to us that the old behavior was intentional. It's actually part of the documentation for the flag:

--remap-path-prefix FROM=TO
                        Remap source names in all output (compiler messages
                        and output files)

[jsgf]

I thought this is the intended behaviour? The local path is not embedded into a compilation output. Compiler diagnostics should give you the local path

No it's a regression. The original intended behavior is to remap diagnostic paths as well.

I saw #83813 go by but I didn't realize it changed diagnostic behaviour or I would have flagged it. Were there tests for this?

[michaelwoerister]

I also thought that the intended behavior has changed after #70642 and the consensus was that diagnostics should not be remapped, so that one gets "real" file system paths in error messages that one can click on / copy & paste, etc.

I'm wondering if we need to make this behavior configurable. It looks like there is no solution that is correct in all use cases. @cbeuw, does your RFC cover this already maybe?

[jsgf]

@michaelwoerister In our use case, the one @dtolnay presented above, we're using --remap-path-prefix to map from temporary build locations to the real location that users can cut'n'paste, edit, open, etc, and also normalize out the cwd (which #87320 would also address).

I believe the idea to remap diagnostic paths came from me in #38322 (comment) and you implemented this in #41508 specifically noting that it also covers error messages.

So as far as this issue goes, not mapping error paths is a regression from the designed behaviour of --remap-path-prefix, and maybe we need to add some other mechanism to deal with #73167 though I admit I haven't fully grokked the details of the issue yet. My initial impression is that it's the question of whether --remap-path-prefix should remap paths embedded in other crate's metadata as part of diagnostics? Or does that already happen? Or is it that we should have some well-defined meta-path-prefixes to logically refer to sources which are "elsewhere" (which @infinity0 somewhat hinted at in #38322 (comment)).

(I'd really like to avoid having two distinct subtly-interacting features covering the same area.)

[cbeuw]

@michaelwoerister My original RFC doesn't cover this (as it was solely to do with Cargo). In response to the RFC feedback, my rust-lang/rfcs#3127 (comment) suggested that we have two extra remapping flags for rustc, one controlling only debuginfo and one controlling only std::file!() expansion (for panic messages). This is basically the set up in Clang and GCC. I still haven't finished the amended RFC with those additions because writing is hard :(

For now, if we are set that printing remapped path in diagnostics is the right behaviour for --remap-path-prefix, we should be able to do it just by replacing calls to prefer_local to prefer_remapped in here https://github.com/rust-lang/rust/blob/master/compiler/rustc_errors/src/emitter.rs. As a longer term solution I suppose we could add a third extra flag that controls the remapping of only compiler diagnostics?

@jsgf --remap-path-prefix does not affect paths coming from an external crate. Nothing in here does remapping on the path of an imported file (except for some hacks on sysroot paths to make some tests consistent when different config was used for rustc bootstrapping)

rust/compiler/rustc_metadata/src/rmeta/decoder.rs

Lines 1767 to 1863 in 7960030

	external_source_map
	.map(|source_file_to_import| {
	// We can't reuse an existing SourceFile, so allocate a new one
	// containing the information we need.
	let rustc_span::SourceFile {
	mut name,
	src_hash,
	start_pos,
	end_pos,
	mut lines,
	mut multibyte_chars,
	mut non_narrow_chars,
	mut normalized_pos,
	name_hash,
	..
	} = source_file_to_import;
	// If this file is under $sysroot/lib/rustlib/src/ but has not been remapped
	// during rust bootstrapping by `remap-debuginfo = true`, and the user
	// wish to simulate that behaviour by -Z simulate-remapped-rust-src-base,
	// then we change `name` to a similar state as if the rust was bootstrapped
	// with `remap-debuginfo = true`.
	// This is useful for testing so that tests about the effects of
	// `try_to_translate_virtual_to_real` don't have to worry about how the
	// compiler is bootstrapped.
	if let Some(virtual_dir) =
	&sess.opts.debugging_opts.simulate_remapped_rust_src_base
	{
	if let Some(real_dir) = &sess.opts.real_rust_source_base_dir {
	if let rustc_span::FileName::Real(ref mut old_name) = name {
	if let rustc_span::RealFileName::LocalPath(local) = old_name {
	if let Ok(rest) = local.strip_prefix(real_dir) {
	*old_name = rustc_span::RealFileName::Remapped {
	local_path: None,
	virtual_name: virtual_dir.join(rest),
	};
	}
	}
	}
	}
	}
	// If this file's path has been remapped to `/rustc/$hash`,
	// we might be able to reverse that (also see comments above,
	// on `try_to_translate_virtual_to_real`).
	try_to_translate_virtual_to_real(&mut name);
	let source_length = (end_pos - start_pos).to_usize();
	// Translate line-start positions and multibyte character
	// position into frame of reference local to file.
	// `SourceMap::new_imported_source_file()` will then translate those
	// coordinates to their new global frame of reference when the
	// offset of the SourceFile is known.
	for pos in &mut lines {
	*pos = *pos - start_pos;
	}
	for mbc in &mut multibyte_chars {
	mbc.pos = mbc.pos - start_pos;
	}
	for swc in &mut non_narrow_chars {
	*swc = *swc - start_pos;
	}
	for np in &mut normalized_pos {
	np.pos = np.pos - start_pos;
	}
	let local_version = sess.source_map().new_imported_source_file(
	name,
	src_hash,
	name_hash,
	source_length,
	self.cnum,
	lines,
	multibyte_chars,
	non_narrow_chars,
	normalized_pos,
	start_pos,
	end_pos,
	);
	debug!(
	"CrateMetaData::imported_source_files alloc \
	source_file {:?} original (start_pos {:?} end_pos {:?}) \
	translated (start_pos {:?} end_pos {:?})",
	local_version.name,
	start_pos,
	end_pos,
	local_version.start_pos,
	local_version.end_pos
	);
	ImportedSourceFile {
	original_start_pos: start_pos,
	original_end_pos: end_pos,
	translated_source_file: local_version,
	}
	})

When rustc exports metadata for the current crate, paths not affected by --remap-path-prefix will have the actual local paths exported; paths affected by --remap-path-prefix will only have the remapped path exported. Those are what another rustc session will see if it imports it.

rust/compiler/rustc_span/src/lib.rs

Lines 200 to 204 in 7960030

	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());

[michaelwoerister]

@jsgf Yes, I think I have read too much into the changes in #70642. I think this is a regression and I would consider it P-high.

I am wondering what to do about the rust-src case though. Since #70642 the compiler will detect if the source code of the standard library is available and remap paths into from the "virtual path" (i.e. /rustc/$rustc_sha) to the actual file system path. If the rust-src rustup component is not installed (as I imagine it is not on your build machines) then you get the virtual paths -- so that case should be fine in practice. But I wonder if this rust-src remapping should be disabled as soon as any --remap-path-prefix is present for the given compiler invocation. Or if there should be a flag to explicitly control this behavior.

@cbeuw, would you be up for working with me on fixing the regression? If we are quick, we might still be able backport it to the current beta.

[michaelwoerister]

One problem I just noticed: if we default to emitting remapped paths in error messages again then we will effectively break the rust-src functionality implemented in #70642, because for libstd there's always a remapped path available :/ So we would trade one regression for another.

[cbeuw]

@michaelwoerister Regarding #70642, could we just drop the virtual path on sysroot files when rust-src is available, and emit the local path everywhere by default unless they have been remapped by an explicit --remap-path-prefix? This will address #85463 as well. I'm happy to open an MCP on this if you think this is a big enough change

[michaelwoerister]

Regarding #70642, could we just drop the virtual path on sysroot files when rust-src is available, and emit the local path everywhere by default unless they have been remapped by an explicit --remap-path-prefix?

@cbeuw, I'm afraid that that would effectively re-introduce #73167. Code instantiated from the standard library would again refer to the local path, even in debuginfo and panic messages, and there would be no way to opt-out of that short of uninstalling the rust-src component, right?

[michaelwoerister]

Let's pull in @rust-lang/wg-diagnostics for feedback on the intended behavior here. Here is a summary of the problem:

• Translate the virtual /rustc/$hash prefix back to a real directory. #70642 implemented a feature in the compiler that makes it emit file paths to the actual source files in the standard library if the rust-src rustup component is installed. So instead of emitting

  • /rustc/881c1ac408d93bb7adaa3a51dabab9266e82eee8/library/std/src/panicking.rs:493:5, the compiler would emit
  • /home/mw/.rustup/toolchains/nightly/lib/rustlib/src/rust/library/std/src/panicking.rs:493:5.

  This is convenient when working locally because the path points to the actual file on disk. But it broke the --remap-path-prefix feature (used e.g. for privacy, reproducible builds) because there we really want to avoid local file system paths ending up in the produced binary -- and there was no way to opt out of this behavior.

• For this reason @cbeuw implemented a fix in Fix --remap-path-prefix not correctly remapping rust-src component paths and unify handling of path mapping with virtualized paths #83813 that made sure we emit the remapped paths (/rustc/{sha}/library/...) into output artifacts, while still emitting local file system paths for diagnostic messages.

• However, it turns out that sometimes (as evidenced by this issue) it is preferable to emit remapped paths for diagnostic messages too.

To me it seems that both use cases make a valid claim for either remapped or local paths to be emitted in diagnostic messages. The question is: how do we support both?

RFC rust-lang/rfcs#3127 will probably make this behavior configurable via commandline flags like --remap-debuginfo-prefix, --remap-macro-prefix, and --remap-diagnostics-prefix. But we don't know when that RFC will be merged and in which form exactly. So for addressing this regression, I don't think we want to wait for that.

So my question is: What should we do here? The options I see are:

• Fix the regression by unconditionally emitting remapped paths in diagnostic messages (implemented in Print remapped path in compiler diagnostics #88191). This will essentially undo the rust-src functionality until the behavior has been made configurable via the RFC.
• Leave things as they are now until the behavior has been made configurable via the RFC.
• Try to automatically choose the right behavior by disabling the rust-src functionality if any --remap-path-prefix option (even one that does not target standard library paths) is provided to rustc. That way, one can create reproducible/privacy-enabled builds even in the presence of rust-src ; but would still get real file system paths in diagnostics for the regular local workflow. This would be the compiler's behavior until the RFC defines something explicit.

I'm personally in favor of the last option as it should provide a good temporary mitigation of the problem.

[cbeuw]

there would be no way to opt-out of that short of uninstalling the rust-src component

@michaelwoerister user would have to manually supply a --remap-path-prefix that remaps the sysroot to something of their preference.

We could provide a special codeword in --remap-path-prefix which expands to the sysroot path or a separate flag, similar to the CWD proposal here rust-lang/compiler-team#450

[michaelwoerister]

@cbeuw, with current semantics the compiler will not remap something that already has been remapped. We would need to implement special handling for remapping the sysroot path during the special import handling for it (together with the special alias you mention). For fixing this regression in the short term, I don't think that's viable and would need an MCP like rust-lang/compiler-team#450.

[michaelwoerister]

I've opened #88363 with a potential fix for this regression that does not break "sysroot de-mapping" (#70642). It still needs test case to be added and updated.

Unfortunately, I think we are already too close to the release for any fix to be backported to beta.

[michaelwoerister]

#88363 has landed and is on nightly. @nicholasbishop, @jsgf, @dtolnay, can you all take a look if it fixes your respective use cases?

[nicholasbishop]

Looks good to me, thanks for the fix!