Lint suggestion: Path construction using format!

[jplatte]

What it does

Suggest to create PathBufs using Path::join (or maybe PathBuf::push in some circumstances) instead of string formatting.

Lint Name

path_format

Category

suspicious

Example

PathBuf::from(format!("{}/foo/bar", base_path)); // or
PathBuf::from(format!("{}\\foo\\bar", base_ath));

could be written as

Path::new(base_path).join("foo").join("bar")

[merelymyself]

I would like to give this a shot. Could I confirm that an approach like this will work?

@rustbot claim

[xFrednet]

Welcome to clippy 👋

Yes, the basic idea is the same. You have to find a format macro where the output is used to create a Path. Depending on your implementation it could be good to first search for the path creation and than figure out where the string comes from :).

[xFrednet]

Hey @jplatte, I've started in the review for the lint and found several examples of paths like r"C:\windows\system32.dll" being used in the documentation of PathBuf. Based on this I wonder if using format for paths is really so wrong. Could you expand a bit why .join() is considered better than format!.

The main reason I could think of would be the support for OSStrings 🤔

[jplatte]

I find it less readable and for portability it could also be better to use the platform default separator. The join solution could also be more efficient (but the opposite could also be true for many successive joins).

For absolute paths like the example you showed I agree that there isn't much of a point as those are already almost certainly platform-specific. Maybe the lint shouldn't fire if the path is known to be absolute?

[xFrednet]

IDK, it seems like Path and PathBuf are designed to handle separators the same across all platforms. The PR includes some rewrites of format macros with join, and I actually find them less readable than the original format version. If there is no technical benefit, I'd prefer the formatted version. This is not to say we can't have the lint, but in that case, I'd see it more in the restriction category.

[Jarcho]

Raw paths on windows \\?\ require backslashes, so if you don't know how the path is rooted then format! can break. Also any use of PathBuf::from(format!("{}/{}", x, y.display())) should be warned on anyways.

[xFrednet]

@Jarcho I'm still not 100% sure I understand all the edge cases correctly. Could you maybe explain why it makes a difference if the path is rooted?

There is another thing, which I'm unsure about. In the PR there is a check to allow cases, where the format is only used for the filename extension like this PathBuf::from(format!("/x/folder{}/textfile.txt", folder_number)). Is this a valid use, or would you say this should also be linted? (Based on the context, I'd guess that this should be linted, correct?)

I feel like I'm lacking some knowledge, since I haven't worked with paths too much in this regard 😅

[Jarcho]

Paths on windows can take a few forms. As a partial list you have:

• Relative paths. e.g. .\foo\bar or foo\bar
• Regular rooted paths. e.g. C:\foo\bar
• UNC paths. e.g. \\domain\share\foo\bar
• Verbatim paths. e.g. \\?\C:\foo\bar

There's a few others as well, but they aren't important here. For the first three forms slashes and backslashes are treated the same way. Verbatim paths are sent to the filesystem driver without being interpreted. In general, this results in only backslashes working as a separator with slashes being interpreted as part of the component's name. This results in \\?\C:\foo/bar accessing a file named foo/bar, not the file bar inside the folder foo.

[xFrednet]

The PR #8833 is a good start for this lint. It was just closed due to inactivity.