Fixup Windows verbatim paths when used with the include! macro

[ChrisDenton]

On Windows, the following code can fail if the OUT_DIR environment variable is a verbatim path (i.e. begins with \\?\):

include!(concat!(env!("OUT_DIR"), "/src/repro.rs"));

This is because verbatim paths treat / literally, as if it were just another character in the file name.

The good news is that the standard library already has code to fix this. We can simply use components to normalize the path so it works as intended.

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in run-make tests.

cc @jieyouxu

[ChrisDenton]

While I'd very much welcome a review from jieyouxu, this will need lang approval. I think it could just be considered a bug fix but it might also be considered a change in language.

cc @joshtriplett maybe?

[Noratrieb]

Given that I see most projects already using / as the separator, calling it a bug fix seems fair. Still, I agree that lang should see it.

[jieyouxu]

Changes look reasonable from compiler side. Rolling a lang reviewer to decide if this behavior change is acceptable.

r? lang

[bors]

☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts.

[joshtriplett]

It looks like this is limited to verbatim paths, which seems completely safe. This wouldn't be safe to do for arbitrary Windows driver-interpreted paths, but it seems entirely safe for Windows verbatim paths.

[ChrisDenton]

Thanks! So I'm going to go out on a limb and say jieyouxu's positive review still stands.

@bors r=jieyouxu

[bors]

📌 Commit edc97a0 has been approved by jieyouxu

It is now in the queue for this repository.

[ChrisDenton]

What happens if you use a bad verbatim path in a path attribute today? Should probably be consistent?

I mean you can't use concat in path, right?

[traviscross]

@rustbot labels -I-lang-easy-decision

This one turned out to not be an easy call for us. One question that came up is:

@ChrisDenton: Is there any way that someone could have used the behavior that exists today correctly with forward slashes in the string? I.e., is there any plausibly valid use case that we might be breaking?

(h/t @tmandry)

[ChrisDenton]

Is there any way that someone could have used the behavior that exists today correctly with forward slashes in the string? I.e., is there any plausibly valid use case that we might be breaking?

I cannot think of one. I can think of an implausible scenario. Like someone could create a named pipe, containing / in the name (which is technically allowed but very hard to do normally) then they intentionally use a verbatim prefix so they can access it.

[ChrisDenton]

But I would note that if they're doing something that's not only platform specific but requires a fair amount of hacks (and would break when using normal \\.\PIPE paths).

The scenario for supporting this is a lot more plausible: someone sets an environment variable in a build script using Path::canonicalize (which makes verbatim paths on Windows). Then they do the common concat thing expecting it to work cross-platform.

[traviscross]

@rfcbot reviewed

Based on the responses here, what's proposed seems like at least the best worst option.

[nikomatsakis]

@rfcbot reviewed

[tmandry]

@rfcbot reviewed

[scottmcm]

I mean you can't use concat in path, right?

This isn't concat-specific though, because it'll change if you put a string literal in the include too.

I just think that we should do it for all compiler paths if we're doing it here.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[traviscross]

@rustbot labels -I-lang-nominated

We discussed this in the meeting today... and it's now in FCP.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[ChrisDenton]

I mean you can't use concat in path, right?

This isn't concat-specific though, because it'll change if you put a string literal in the include too.

I just think that we should do it for all compiler paths if we're doing it here.

Oh, it turns out we do something kind of similar for path already:

rust/compiler/rustc_expand/src/module.rs

Lines 195 to 200 in f225713

	// On windows, the base path might have the form
	// `\\?\foo\bar` in which case it does not tolerate
	// mixed `/` and `\` separators, so canonicalize
	// `/` to `\`.
	#[cfg(windows)]
	let path_str = path_str.replace("/", "\\");

[ChrisDenton]

This has passed lang FCP and was earlier approved by @jieyouxu so I'll merge this now. I'll look more at #[path] but it already looks like it does the equivalent by virtue of using Path::join (which handles verbatim paths).

rust/compiler/rustc_expand/src/module.rs

Line 202 in f225713

Some(dir_path.join(path_str))

@bors r=jieyouxu

[bors]

📌 Commit edc97a0 has been approved by jieyouxu

It is now in the queue for this repository.