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.

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

Give rustfmt a hint about stdin input #5266

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Mar 14, 2022

When formatting text via stdin rustfmt didn't have a way to ignore stdin input.

Now, when passing input to rustfmt via stdin one can also provide the --stdin-file-hint option to inform rustfmt that the input is actually from the hinted at file. rustfmt now uses this hint to determine if it can ignore formatting stdin and immediately echo the input back to the user.

Note: This option can only be passed when formatting files via stdin, and is intended for text editor plugins that call rustfmt by passing input via stdin (e.g. rust-analyzer).

@ytmimi ytmimi force-pushed the ignore-stdin branch 2 times, most recently from bb4519e to 2a3ed2c Compare March 15, 2022 01:08
@ytmimi
Copy link
Contributor Author

ytmimi commented Mar 15, 2022

Looks like the "stable" tests are failing because the ignore list can't be set. I'll add the #[nightly_only_test] to the relevant tests.

Also looks like some issues with windows paths.
Edit: The issue does not seem to be related to windows paths see comment below

@ytmimi
Copy link
Contributor Author

ytmimi commented Mar 17, 2022

Looking at (i686-pc-windows-gnu, nightly) logs (but all windows logs should be similar) I added some println! macros to help me debug what's going on. I'm scratching my head because the strings should be the same but the comparison is failing on windows, but succeeding on linux/mac.

Heres' an example of one of the logs

---- rustfmt_stdin_formatting::changes_are_output_to_stdout stdout ----
expect: fn main() {
    println!("hello world!");
}

stdout: fn main() {
    println!("hello world!");
}

stderr: 
thread 'rustfmt_stdin_formatting::changes_are_output_to_stdout' panicked at 'assertion failed: stdout == expected_output', tests\rustfmt\main.rs:212:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Edit:

If you compare the expect value with the stdout value in a python REPL it shows that they're the same, so now I'm really confused 🙃

>>> expect = """
... fn main() {
...     println!("hello world!");
... }
... """
>>> stdout = """
... fn main() {
...     println!("hello world!");
... }
... """
>>> expect == stdout
True

@calebcartwright
Copy link
Member

I'm scratching my head because the strings should be the same but the comparison is failing on windows, but succeeding on linux/mac.

Haven't looked at this in detail but this type of thing often hints at line endings

@ytmimi
Copy link
Contributor Author

ytmimi commented Mar 18, 2022

Haven't looked at this in detail but this type of thing often hints at line endings

Ahh I didn't even consider that as a possible issue. I'll investigate that!

@ytmimi ytmimi force-pushed the ignore-stdin branch 3 times, most recently from cd23c85 to bcf190d Compare March 18, 2022 02:49
@ytmimi
Copy link
Contributor Author

ytmimi commented Mar 18, 2022

@calebcartwright you were right. It turns out it was a line ending issue. For the tests that were giving me trouble I decided to pass them the newline_style=Unix config option to not have to worry about the line endings.

No rush on the review!

@ytmimi ytmimi added p-low pr-merge-conflict This PR has merge conflicts that must be resolved labels Jul 27, 2022

// return early if there are issues with the file hint specified
if let Some(file_hint) = &options.stdin_file_hint {
if !file_hint.exists() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason that the file must exist on the file system?
For our use case #5792 (files in git history) there could be egde cases where the file is not present on disk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, it's been a very long time since I've looked at this PR, and I don't remember why. I definitely didn't have your use case in mind when I initially wrote this PR.

It's hard to say when I'll revisit this, but when I do, I'll double check if there are any issues with providing non-existent files as the file hint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another use case would be editing usaved files.

ytmimi added 2 commits June 27, 2024 01:42
When formatting files via stdin rustfmt didn't have a way to ignore
stdin input.

Now, when passing input to rustfmt via stdin one can also provide the
`--stdin-file-hint` option to inform rustfmt that the input is actually
from the hinted at file. rustfmt now uses this hint to determine if it
can ignore formatting stdin.

Note: This option is intended for text editor plugins that call rustfmt
by passing input via stdin (e.g. rust-analyzer).
@ytmimi ytmimi removed pr-merge-conflict This PR has merge conflicts that must be resolved p-low labels Jun 27, 2024
@@ -90,6 +90,22 @@ pub enum OperationError {
/// supported with standard input.
#[error("Emit mode {0} not supported with standard output.")]
StdinBadEmit(EmitMode),
/// Using `--std-file-hint` incorrectly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Using `--std-file-hint` incorrectly
/// Using `--stdin-file-hint` incorrectly

@@ -494,6 +523,13 @@ fn determine_operation(matches: &Matches) -> Result<Operation, OperationError> {
return Ok(Operation::Stdin { input: buffer });
}

// User's can only pass `--stdin-file-hint` when formating files via stdin.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// User's can only pass `--stdin-file-hint` when formating files via stdin.
// Users can only pass `--stdin-file-hint` when formating files via stdin.

}

if let Some(ext) = file_hint.extension() {
if ext != "rs" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does rustfmt reject files with other extensions when passed a path? We should probably match that behavior, someone could use a different extension for various reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants