FR: make disabling `format_generated_files` work with stdin input

[ilyagr]

Currently, rustfmt formats generated files if they are passed via stdin, even if format_generated_files=false. I think it's nice to have an issue tracking this problem specifically.

See #5080 (comment) for an overview with a few more details, and #5172 for the reason for the current behavior.

IMO, if format_generated_files=false, rustfmt should act like cat on generated files passed via stdin. I'm unsure whether there are any issues with such a plan. This is already what happens when disable_all_formatting=true.

---

I'll link to #5137 which I think is spiritually related; if that were fixed, it would be a workaround for this issue as well (and vice-versa, as far as generated files are concerned, which is currently my main use-case for #5137).

[ilyagr]

I tried to fix this, but ran into the problem that the test

rustfmt/src/test/mod.rs

Lines 617 to 636 in 0332da0

	#[test]
	fn stdin_generated_files_issue_5172() {
	init_log();
	let input = Input::Text("//@generated\nfn main() {}".to_owned());
	let mut config = Config::default();
	config.set().emit_mode(EmitMode::Stdout);
	config.set().format_generated_files(false);
	config.set().newline_style(NewlineStyle::Unix);
	let mut buf: Vec<u8> = vec![];
	{
	let mut session = Session::new(config, Some(&mut buf));
	session.format(input).unwrap();
	assert!(session.has_no_errors());
	}
	// N.B. this should be changed once `format_generated_files` is supported with stdin
	assert_eq!(
	String::from_utf8(buf).unwrap(),
	"<stdin>:\n\n//@generated\nfn main() {}\n",
	);
	}

doesn't actually pass input via stdin like the tests above and below it. I think it'd work fine if I rewrote it to match the surrounding tests (see inside Details), but I'm not sure whether there's some way the user or some API could call rustfmt in the same way the test does.

For reference, here's my naive patch:

diff --git a/src/formatting.rs b/src/formatting.rs
index 1e1e329f62..503046ac94 100644
--- a/src/formatting.rs
+++ b/src/formatting.rs
@@ -77,9 +77,7 @@
         return true;
     }
 
-    // FIXME(calebcartwright) - we need to determine how we'll handle the
-    // `format_generated_files` option with stdin based input.
-    if !input_is_stdin && !config.format_generated_files() {
+    if !config.format_generated_files() {
         let source_file = context.psess.span_to_file_contents(module.span);
         let src = source_file.src.as_ref().expect("SourceFile without src");
 
@@ -140,10 +138,6 @@
     )
     .visit_crate(&krate)?
     .into_iter()
-    .filter(|(path, module)| {
-        input_is_stdin
-            || !should_skip_module(config, &context, input_is_stdin, &main_file, path, module)
-    })
     .collect::<Vec<_>>();
 
     timer = timer.done_parsing();
@@ -152,8 +146,12 @@
     context.psess.set_silent_emitter();
 
     for (path, module) in files {
-        if input_is_stdin && contains_skip(module.attrs()) {
+        let should_skip =
+            should_skip_module(config, &context, input_is_stdin, &main_file, &path, &module);
+        if input_is_stdin && (should_skip || contains_skip(module.attrs())) {
             return echo_back_stdin(context.psess.snippet_provider(module.span).entire_snippet());
+        } else if should_skip {
+            continue;
         }
         should_emit_verbose(input_is_stdin, config, || println!("Formatting {}", path));
         context.format_file(path, &module, is_macro_def)?;

With this patch, buf inside the test ends up empty.

(Aside: I'm also not sure whether it's necessary to check for contains_skip(module.attrs()) both inside should_skip_module and in the loop above, but that's a separate and minor issue)

Here's a test that passes with this patch:

#[test]
fn stdin_disable_generated_formatting_test() {
    init_log();
    let input =
        String::from("//@generated\nfn main() { println!(\"This should not be formatted.\"); }");
    let mut child = Command::new(rustfmt().to_str().unwrap())
        .stdin(Stdio::piped())
        .stdout(Stdio::piped())
        .arg("--config=format_generated_files=false")
        .spawn()
        .expect("failed to execute child");

    {
        let stdin = child.stdin.as_mut().expect("failed to get stdin");
        stdin
            .write_all(input.as_bytes())
            .expect("failed to write stdin");
    }

    let output = child.wait_with_output().expect("failed to wait on child");
    assert!(output.status.success());
    assert!(output.stderr.is_empty());
    assert_eq!(input, String::from_utf8(output.stdout).unwrap());
}

(Aside: Perhaps there should be a helper for running stdin tests like these)

[ilyagr]

Ah, maybe the problem is that echo_back_stdin does not take into account what the configured "emitter" is or was, and instead prints directly to stdout. It seems like it could easily output to an emitter, but I'm not sure what else that would affect.