--check silently ignored when formatting from stdin

[pyrrho]

When I run,

echo '
fn   foo (   ) {
    fake_fn("this is fine");
    fake_fn   ( "This not so much"    )    ;
}
' | rustfmt --check

the output is equivalent to rustfmt --emit stdout. Ideally, I would like to see the above call emit something like,

Diff in 'stdin' at line 1:
-fn   foo (   ) {
+fn foo() {
     fake_fn("this is fine");
-    fake_fn   ( "This not so much"    )    ;
+    fake_fn("This not so much");
 }

If that's not possible, a warning or error message noting the ignored option would make this behavior less surprising, and may help nudge users towards valid usage.

It might also be worth noting that rustfmt +nightly --emit json is silently ignored in the same situations.

$ rustfmt --version
rustfmt 1.4.4-stable (0462008d 2019-08-06)

$ rustfmt +nightly --version
rustfmt 1.4.9-nightly (33e36670 2019-10-07)

[calebcartwright]

When using stdin input, rustfmt will always use stdout emit mode (by design). However, as you noted, because of this rustfmt is silently ignoring the --check and --emit options.

I don't think it'll be viable to support the various file emit modes with stdin (include the diff emit mode that comes with --check when formatting files), but would it be helpful for this to be "less silent"?

I suppose we could have rustfmt err if you include --check or --emit with stdin (just like it does if you try to use both --check and --emit) or print a warning message, and/or we could note the stdin = stdout only in the readme

[topecongiro]

When using stdin input, rustfmt will always use stdout emit mode (by design).

Well, we can support --check on stdin input it is feasible. If not, then yes, rustfmt should exit with an error.

[pyrrho]

I agree, if it's possible to emit diffs (with --check) or JSON (with +nightly --emit json), that would be the best solution.

If not, an error (as occurs with --check --emit stdout) would be better than silently ignoring options.

[jugglerchris]

I'd be interested in working on this - making it an error as a first step, and then looking into making --check and --emit work.

[calebcartwright]

Currently, the checkstyle and json emitters would not handle stdin as they are expecting a real file path:

rustfmt/src/emitter/json.rs

Lines 40 to 60 in 69c7dbc

	fn emit_formatted_file(
	&mut self,
	output: &mut dyn Write,
	FormattedFile {
	filename,
	original_text,
	formatted_text,
	}: FormattedFile<'_>,
	) -> Result<EmitterResult, io::Error> {
	const CONTEXT_SIZE: usize = 0;
	let filename = ensure_real_path(filename);
	let diff = make_diff(original_text, formatted_text, CONTEXT_SIZE);
	let has_diff = !diff.is_empty();
	if has_diff {
	output_json_file(output, filename, diff, self.num_files)?;
	self.num_files += 1;
	}
	Ok(EmitterResult { has_diff })
	}

https://github.com/rust-lang/rustfmt/blob/master/src/emitter/checkstyle.rs#L23-L37

I included the file path check when I implemented the json emitter because it was there in the checkstyle emitter, though in hindsight I'm not sure if that's really required

I don't see any reason why name couldn't be stdin in the json output

[  
  {  
    "name":"tests/writemode/source/json.rs",
    "mismatches":[  
      {  
        "original_begin_line":5,
        "original_end_line":7,
        "expected_begin_line":5,
        "expected_end_line":5,
        "original":"fn foo_expr() {\n    1\n}",
        "expected":"fn foo_expr() { 1 }"
      }
]

The diff emitter (which is used with --check), would need to be tweaked as well to not use ensure_real_path if the file name is stdin, which it does when either the user includes the -l option (which lists file names) or if there is a newline style diff.

[jugglerchris]

I've taken the liberty of speculatively doing the first step (returning errors) in #3875; I'll probably speculatively go ahead and try to make them work too. :-)

[jugglerchris]

I got some time to look at making the other emit modes work on this branch. I haven't done any tests so far for the changes though. Looking through, tests/rustfmt/main.rs looks like the most likely place to add tests of the rustfmt executable - is that right?

[calebcartwright]

Looking through, tests/rustfmt/main.rs looks like the most likely place to add tests of the rustfmt executable - is that right?

I'd take a look at src/test/mod.rs, for example

rustfmt/src/test/mod.rs

Lines 409 to 429 in 69c7dbc

	fn stdin_works_with_modified_lines() {
	init_log();
	let input = "\nfn\n some( )\n{\n}\nfn main () {}\n";
	let output = "1 6 2\nfn some() {}\nfn main() {}\n";
	let input = Input::Text(input.to_owned());
	let mut config = Config::default();
	config.set().newline_style(NewlineStyle::Unix);
	config.set().emit_mode(EmitMode::ModifiedLines);
	let mut buf: Vec<u8> = vec![];
	{
	let mut session = Session::new(config, Some(&mut buf));
	session.format(input).unwrap();
	let errors = ReportedErrors {
	has_diff: true,
	..Default::default()
	};
	assert_eq!(session.errors, errors);
	}
	assert_eq!(buf, output.as_bytes());
	}

and the in-file unit tests for the relevant emitters src/emitter/..
https://github.com/rust-lang/rustfmt/tree/master/src/emitter

[jugglerchris]

Thanks @calebcartwright , that was very useful. I've raised a new PR with the JSON and checkstyle emitters now working with stdin (not yet --check). I'm not happy about adding a #[rustfmt::skip] in the tests - is the right fix to put the outputs in text files and load them as with some other tests?

[calebcartwright]

Thanks @jugglerchris! Will respond to your question over in #3894

[jugglerchris]

I think #3894 and #3896 (which includes the changes from the other - perhaps it wasn't worth breaking them into two PRs... I can close one if that's easier) finish resolving this ticket.

[jugglerchris]

Have I missed any parts to this or do we think this can be closed?

[calebcartwright]

Was the diff emitter updated? That's the emitter used with --check (the checkstyle emitter is for the xml report)

[jugglerchris]

You can't select it explicitly with --emit, but --check with input from stdin works for me:

$ cargo +nightly run --bin rustfmt -- --check < ./foo.rs
Diff in stdin at line 1:
-fn main
-()
-
-{
-}
+fn main() {}
+

[calebcartwright]

You can't select it explicitly with --emit, but --check with input from stdin works for me:

$ cargo +nightly run --bin rustfmt -- --check < ./foo.rs
Diff in stdin at line 1:
-fn main
-()
-
-{
-}
+fn main() {}
+

I know, but if the only formatting issue is line endings and/or the user also includes the -l option there would still be an issue when using --check as the assocaited emitter is still expecting the file name to be an actual path, no?

Obviously that'd be a pretty weird user behavior, but I do think it'd be worth making the same updates to avoid such a scenario/error. Whether that's updating the emitter or just checking for the presence of the -l option up front with stdin input

[jugglerchris]

Ok, I do get a panic with --check -l, and the "Incorrect newline style" message just doesn't appear with stdin. Thanks! I'll take a look at those.

[jugglerchris]

I think the missing "Incorrect newline style" is because the SourceMap doesn't remember the original line endings, so by the time it gets to DiffEmitter::emit_formatted_file() it's identical to the fixed output. But I've hopefully fixed the panics now.

[calebcartwright]

👍 I believe your updates to the diff emitter will cover both cases.

It's true that internally the parser normalizes line endings to unix style, so the original_text value will always have unix style endings when using stdin (as there is no original source file rustfmt can check to determine the actual original line endings). However, I do think there were some scenarios where formatted_text could have windows style endings (namely with newline_style = "Windows" or newline_style = "Native" when running on windows) which may have resulted in panics with the previous behavior.

[topecongiro]

Closed via #3910.