Backport: Do not touch module with #![rustfmt::skip] (4297) #5094

ytmimi · 2021-11-19T04:11:47Z

Although the implementation is slightly different than the original PR,
the general idea is the same. After collecting all modules we want to
exclude formatting those that contain the #![rustfmt::skip] attribute.

calebcartwright · 2021-11-23T22:15:06Z

Thanks! Code changes look fine, though I have two asks:

Could you explain the purpose of the empty src/macros_ast.rs file, or remove it if it's not necessary?
Could you extend/modify the tests to cover more than just a single entry point, notably, let's be sure to have a test for imported out of line mods that are discovered and would normally be formatted (non direct/entry point files provided to rustfmt explicitly)

On the testing front, I'd advise utilizing the model I've started adding for module resolution related testing (see https://github.com/rust-lang/rustfmt/blob/master/src/test/mod_resolver.rs) for more targeted and explicit validation. One of the challenges we saw historically with trying to use the system/idempotence based checks is that it was far too easy for the absence of a failure to be specious and our tests weren't catching mod resolution/skipping issues, especially with imported/nested files. That happened typically because the out of line file(s) had to be explicitly added to the skip list used for the system/idempotence tests, and there was no way of differentiating between the testing skip list vs. the ignore behavior.

ytmimi · 2021-11-23T22:41:13Z

Could you explain the purpose of the empty src/macros_ast.rs file, or remove it if it's not necessary?

whoops that definitely shouldn't have been included. will remove right away!

Could you extend/modify the tests to cover more than just a single entry point, notably, let's be sure to have a test for imported out of line mods that are discovered and would normally be formatted (non direct/entry point files provided to rustfmt explicitly)

Sure! I can definitely add some additional test, and thanks for explaining all the nuance of properly testing skipping. I'll let you know if I have any questions.

calebcartwright · 2021-11-23T22:58:00Z

src/formatting.rs

+    .into_iter()
+    .filter(|(_, module)| !contains_skip(module.attrs()));


Actually I think I changed my mind sorry. Instead of having filtering scattered about in multiple places, I'd like to consolidate it in one place (there's several other checks on lines 114-119). It would probably be best to add filtering iterations after line 102/105 so that the reported duration of parsing is as closely confined to the parsing pass as possible, but I don't feel too strongly about it.

Given the ever expanding list of conditional checks, it would probably be best to pull things out into some sort of function though so it's easy to see/add more checks in the future

ytmimi · 2021-11-25T03:32:44Z

I've pulled out all the filtering logic into its own function should_skip_module
When I was adding one additional tests something struck me as odd and I wanted to get your input. When adding
the #![rustfmt::skip] attribute to tests/mod-resolver/skip-files-issue-5065/foo.rs then
tests/mod-resolver/skip-files-issue-5065/foo/bar/baz.rs never gets found.

I found this strange because when I add the #![rustfmt::skip] attribute to tests/mod-resolver/skip-files-issue 5065/main.rs we're still able to find tests/mod-resolver/skip-files-issue-5065/foo.rs and tests/mod- resolver/skip-files-issue-5065/one.rs

I guess my expectation would be that we'd skip formatting tests/mod-resolver/skip-files-issue-5065/foo.rs, but
we'd still format tests/mod-resolver/skip-files-issue-5065/bar/baz.rs, but if this is the right behavior,
please let me know.

calebcartwright · 2021-11-26T23:32:40Z

When I was adding one additional tests something struck me as odd and I wanted to get your input

I suspect this is due to the attribute placed on the main input file which gets handled a bit differently. One of the things on my todo list is to enhance the resolution process to take more "ignore" type information into account up front, but there's still some pending discussion to be had around whether ignore means "parse and discover, but don't format" or "completely ignore me altogether and don't even attempt to parse me"

calebcartwright · 2021-11-27T00:10:27Z

src/formatting.rs

@@ -5,6 +5,7 @@ use std::io::{self, Write};
 use std::time::{Duration, Instant};

 use rustc_ast::ast;
+use rustc_ast::AstLike;


I gather this is indeed needed otherwise the deny warnings compiler lint would've caused build failures, though curious what we need this for?

Yeah we need the trait rustc_ast::AstLike to be in scope to write module.attrs(). Here's the error message you get if you remove the use statement and run cargo check

--> src\formatting.rs:70:29 | 70 | if contains_skip(module.attrs()) { | ^^^^^ method not found in `&Module<'_>` | = help: items from traits can only be used if the trait is in scope help: the following trait is implemented but not in scope; perhaps add a `use` for it: | 3 | use crate::rustc_ast::AstLike; |

calebcartwright · 2021-11-27T00:14:44Z

src/formatting.rs

+fn should_skip_module<T: FormatHandler>(
+    config: &Config,
+    context: &FormatContext<'_, T>,
+    input_is_stdin: bool,
+    main_file: &FileName,
+    path: &FileName,
+    module: &Module<'_>,
+) -> bool {
+    if contains_skip(module.attrs()) {
+        return true;
+    }
+
+    let source_file = context.parse_session.span_to_file_contents(module.span);
+    let src = source_file.src.as_ref().expect("SourceFile without src");
+
+    let should_ignore = (!input_is_stdin && context.ignore_file(&path))
+        || (!config.format_generated_files() && is_generated_file(src));
+
+    if (config.skip_children() && path != main_file) || should_ignore {
+        return true;
+    }
+
+    false
+}


Thanks for pulling this out, think it makes things much more clear. I realize what you've done here is identical to what was there before, though as I'm looking at this I feel like we could maybe clean it up a bit more?

For example, we could use the same type of early return pattern utilized with contains_skip in the function opening for a file based input that's ignored, and save the span_to_file_contents and src.as_ref() calls when those conditions are true

I reorganized the should_skip_module function so now we only call span_to_file_contents and src.as_ref() when they are needed.

calebcartwright · 2021-11-27T00:16:24Z

src/test/configuration_snippet.rs

@@ -251,6 +255,7 @@ fn configuration_snippet_tests() {
    let blocks = get_code_blocks();
    let failures = blocks
        .iter()
+        .filter(|block| !block.fmt_skip())


Was this change related/necessary for the fix? It seems reasonable enough but wasn't sure if it was just an add on

Yeah it was needed. The tests fail otherwise. The original PR (4297) ran into a similar issue, and here's how they chose to fix it. The test code has changed a bit since (4297) was opened, but the comment in the commit is insightful as to what's happening and why we need to skip checking these code blocks.

Let me know if you have another suggestions for fixing the test.

Although the implementation is slightly different than the original PR, the general idea is the same. After collecting all modules we want to exclude formatting those that contain the #![rustfmt::skip] attribute.

calebcartwright · 2021-12-08T00:44:51Z

Nicely done, thank you!

ytmimi added the pr-not-reviewed label Nov 19, 2021

calebcartwright removed the pr-not-reviewed label Nov 23, 2021

calebcartwright added the pr-waiting-on-author label Nov 23, 2021

calebcartwright reviewed Nov 23, 2021

View reviewed changes

ytmimi force-pushed the backport_issue_5065 branch 3 times, most recently from ad69578 to f82070c Compare November 25, 2021 03:23

ytmimi force-pushed the backport_issue_5065 branch from f82070c to 66b8fe1 Compare November 25, 2021 03:34

ytmimi requested a review from calebcartwright November 25, 2021 03:48

ytmimi added pr-follow-up-review-pending and removed pr-waiting-on-author labels Nov 25, 2021

calebcartwright reviewed Nov 27, 2021

View reviewed changes

calebcartwright added pr-waiting-on-author and removed pr-follow-up-review-pending labels Nov 27, 2021

Backport: Do not touch module with #![rustfmt::skip] (4297)

246cfa9

Although the implementation is slightly different than the original PR, the general idea is the same. After collecting all modules we want to exclude formatting those that contain the #![rustfmt::skip] attribute.

ytmimi force-pushed the backport_issue_5065 branch from 66b8fe1 to 246cfa9 Compare November 30, 2021 18:43

ytmimi added pr-follow-up-review-pending and removed pr-waiting-on-author labels Nov 30, 2021

calebcartwright removed the pr-follow-up-review-pending label Dec 8, 2021

calebcartwright merged commit f40b1d9 into rust-lang:master Dec 8, 2021

ytmimi mentioned this pull request Mar 29, 2022

Do not touch module with #![rustfmt::skip] #4297

Merged

This was referenced Jun 4, 2022

Files content get deleted when formatting with #![rustfmt::skip] attribute rust-lang/rust-analyzer#12458

Closed

regression: rustfmt using input from stdin emits empty output when using #![rustfmt::skip] #5368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport: Do not touch module with #![rustfmt::skip] (4297) #5094

Backport: Do not touch module with #![rustfmt::skip] (4297) #5094

ytmimi commented Nov 19, 2021 •

edited

Loading

calebcartwright commented Nov 23, 2021

ytmimi commented Nov 23, 2021

calebcartwright Nov 23, 2021 •

edited

Loading

ytmimi commented Nov 25, 2021 •

edited

Loading

calebcartwright commented Nov 26, 2021

calebcartwright Nov 27, 2021

ytmimi Nov 30, 2021

calebcartwright Nov 27, 2021

ytmimi Nov 30, 2021

calebcartwright Nov 27, 2021

ytmimi Nov 30, 2021

calebcartwright commented Dec 8, 2021

		.into_iter()
		.filter(\|(_, module)\| !contains_skip(module.attrs()));

Backport: Do not touch module with #![rustfmt::skip] (4297) #5094

Backport: Do not touch module with #![rustfmt::skip] (4297) #5094

Conversation

ytmimi commented Nov 19, 2021 • edited Loading

calebcartwright commented Nov 23, 2021

ytmimi commented Nov 23, 2021

calebcartwright Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

ytmimi commented Nov 25, 2021 • edited Loading

calebcartwright commented Nov 26, 2021

calebcartwright Nov 27, 2021

Choose a reason for hiding this comment

ytmimi Nov 30, 2021

Choose a reason for hiding this comment

calebcartwright Nov 27, 2021

Choose a reason for hiding this comment

ytmimi Nov 30, 2021

Choose a reason for hiding this comment

calebcartwright Nov 27, 2021

Choose a reason for hiding this comment

ytmimi Nov 30, 2021

Choose a reason for hiding this comment

calebcartwright commented Dec 8, 2021

ytmimi commented Nov 19, 2021 •

edited

Loading

calebcartwright Nov 23, 2021 •

edited

Loading

ytmimi commented Nov 25, 2021 •

edited

Loading