Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Ensure that fn config_path returns canonicalized paths. #6272

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Ensure that fn config_path returns canonicalized paths. #6272

merged 1 commit into from
Aug 14, 2024

Conversation

anforowicz
Copy link
Contributor

This commit ensures that fn config_path in rustfmt/src/config/mod.rs always returns canonicalized paths. This is achieved by canonicalizing both: 1) paths received via CliOptions (after checking if the path exists) as well as 2) paths returned via get_toml_path (canonicalizing within fn get_toml_path).

Fixes #6178

@ytmimi
Copy link
Contributor

ytmimi commented Aug 12, 2024

Thanks for the PR. Would you mind double checking if there's any difference in the output when running rustfmt -v path/to/file.rs on windows before and after this change if the paths are canonicalized. I think we might already be canonicalizing them before we display the paths, but I want to double check.

Output should look something like this:

Using rustfmt config file /path/to/rustfmt.toml for /path/to/file.rs
Formatting /path/to/file.rs
Spent 0.000 secs in the parsing phase, and 0.001 secs in the formatting phase

@anforowicz
Copy link
Contributor Author

Would you mind double checking if there's any difference in the output when running rustfmt -v path/to/file.rs on windows before and after this change if the paths are canonicalized. I think we might already be canonicalizing them before we display the paths, but I want to double check.

There are no output changes in this scenario.

Before the changes:

C:\src\rustfmt>git checkout origin/master
C:\src\rustfmt>cargo run --bin rustfmt -- -v path/to/file.rs
   Compiling rustfmt-nightly v1.7.1 (C:\src\rustfmt)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 9.50s
     Running `target\debug\rustfmt.exe -v path/to/file.rs`
Using rustfmt config file \\?\C:\src\rustfmt\rustfmt.toml for \\?\C:\src\rustfmt\path\to\file.rs
Formatting \\?\C:\src\rustfmt\path\to\file.rs
Spent 0.012 secs in the parsing phase, and 0.002 secs in the formatting phase

After the changes:

C:\src\rustfmt>git checkout path-canon
C:\src\rustfmt>cargo run --bin rustfmt -- -v path/to/file.rs
   Compiling rustfmt-nightly v1.7.1 (C:\src\rustfmt)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 8.84s
     Running `target\debug\rustfmt.exe -v path/to/file.rs`
Using rustfmt config file \\?\C:\src\rustfmt\rustfmt.toml for \\?\C:\src\rustfmt\path\to\file.rs
Formatting \\?\C:\src\rustfmt\path\to\file.rs
Spent 0.012 secs in the parsing phase, and 0.001 secs in the formatting phase

As expected the path passed to --config-path changes (gets canonicalized) after the changes.

Before the changes:

C:\src\rustfmt>git checkout origin/master
C:\src\rustfmt>cargo run --bin rustfmt -- -v --config-path=c:\test\rustfmt.toml c:\test\bar.rs
   Compiling rustfmt-nightly v1.7.1 (C:\src\rustfmt)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 8.52s
     Running `target\debug\rustfmt.exe -v --config-path=c:\test\rustfmt.toml c:\test\bar.rs`
Using rustfmt config file c:\test\rustfmt.toml
thread 'main' panicked at C:\Users\lukasza\.cargo\registry\src\index.crates.io-6f17d22bba15001f\ignore-0.4.18\src\gitignore.rs:227:9:
path is expected to be under the root
...

After the changes:

C:\src\rustfmt>git checkout path-canon
C:\src\rustfmt>cargo run --bin rustfmt -- -v --config-path=c:\test\rustfmt.toml c:\test\bar.rs
   Compiling rustfmt-nightly v1.7.1 (C:\src\rustfmt)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 8.44s
     Running `target\debug\rustfmt.exe -v --config-path=c:\test\rustfmt.toml c:\test\bar.rs`
Using rustfmt config file \\?\C:\test\rustfmt.toml
Formatting \\?\C:\test\bar.rs
Spent 0.010 secs in the parsing phase, and 0.005 secs in the formatting phase

@ytmimi
Copy link
Contributor

ytmimi commented Aug 14, 2024 •
edited
Loading

Thanks for double checking. I figured that the behavior would change slightly and wanted to confirm that.

Edit: Diff Check Job passed ✅

@anforowicz @ytmimi
Ensure that fn config_path returns canonicalized paths.
97a5426 
This commit ensures that `fn config_path` in `rustfmt/src/config/mod.rs`
always returns canonicalized paths.  This is achieved by canonicalizing
both: 1) paths received via `CliOptions` (after checking if the path
exists) as well as 2) paths returned via `get_toml_path` (canonicalizing
within `fn get_toml_path`).

Fixes rust-lang#6178
@ytmimi ytmimi added pr-ready-to-merge release-notes Needs an associated changelog entry and removed pr-not-reviewed labels Aug 14, 2024
@ytmimi ytmimi merged commit caaa612 into rust-lang:master Aug 14, 2024
26 checks passed
@anforowicz anforowicz deleted the path-canon branch August 14, 2024 19:35
@ytmimi ytmimi removed the release-notes Needs an associated changelog entry label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ytmimi ytmimi ytmimi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rustfmt crashes with ignore on Windows
3 participants
@anforowicz @ytmimi @rustbot