add support for formatting files in subdirectories of tests

[calebcartwright]

Resolves #1820

The idea of the approach is that if at least one of the packages that's going to be formatted contains a test target, then look for files within subdirectories of the tests directory for that package. If there were any found, then those are passed (along with the typical src_path for each of the package targets) to rustfmt.

I've tested it with a mix of repositories, format strategies, etc., including on clippy to validate. AFAICT it is working as desired.

Opening this in draft to see whether folks think this might be a viable approach, if so I'll add some tests and do a bit of cleanup. It's not perfect, but my hope is that it's better than not having any support for these files at all.

To test this with clippy (or any other repo that has nested files under tests as described in #1820), create a formatting error in one of those nested files (I butchered tests/ui-toml/bad_toml/conf_bad_toml.rs for my below examples, make sure you add tests/ui/crashes/ice-3891.rs to the rustfmt.toml ignore config)

Without the new flags, no checking of nested files and no isses

$ cargo +nightly fmt -- --check 

With the new flags:

$ cargo +nightly fmt --include-nested-test-files -- --check
Diff in rust-clippy/tests/ui-toml/bad_toml/conf_bad_toml.rs at line 1:
 // error-pattern: error reading Clippy's configuration file

-fn main()           {
-
-        }
+fn main() {}

I've made this an opt-in feature via a new flag for cargo fmt (--include-nested-test-files) given that this has the potential to introduce new formatting issues for folks (checking/formatting files that may have never been checked). We could potentially invert that (perhaps with 2.0?) to make this the default behavior with an opt-out flag as well.

I also added a cargo fmt option to allow for listed nested files under tests to be ignored. The ignore config option also works for excluding files in subdirs within tests, but I was having an issue with one of the ice files in the clippy repo that was still happening even with that file set in the rustfmt.toml ignore setting 🤷‍♂

Edit: #3782 eliminated the need for this duplicative ignore cli arg

[topecongiro]

Thank you for the PR! I think the overall strategy should work, though would prefer not to add ignored-nested-test-files command line option.

I also added a cargo fmt option to allow for listed nested files under tests to be ignored. The ignore config option also works for excluding files in subdirs within tests, but I was having an issue with one of the ice files in the clippy repo that was still happening even with that file set in the rustfmt.toml ignore setting 🤷‍♂

I do think that ignore config option should suffice in this case, and surprised that it's not working. Do you mean that formatting the file causes rustfmt to crash?

[calebcartwright]

Do you mean that formatting the file causes rustfmt to crash?

Correct (or it at least causes rustfmt to always returns a non-zero exit code). Without excluding that file from being passed to rustfmt in the first place, I always get the below error:

error: invalid suffix `x` for integer literal
 --> rust-clippy/tests/ui/crashes/ice-3891.rs:3:5
  |
3 |     1x;
  |     ^^ invalid suffix `x`
  |
  = help: the suffix must be one of the integral types (`u32`, `isize`, etc)

It looks like they were explicitly ignoring that file over in clippy too via their workaround

https://github.com/rust-lang/rust-clippy/blob/master/clippy_dev/src/fmt.rs#L51

[topecongiro]

Ok, looks like rustfmt is panicking inside the parser while trying to parse the file with invalid syntax.

The file is being parsed even though it is specified in ignore config option because we are filtering files with ignore after the module resolution, which requires parsing the file.

rustfmt/src/formatting.rs

Lines 101 to 109 in 1ded995

	let files = modules::ModResolver::new(
	&context.parse_session,
	directory_ownership.unwrap_or(parse::DirectoryOwnership::UnownedViaMod(true)),
	!(input_is_stdin || config.skip_children()),
	)
	.visit_crate(&krate)
	.map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
	for (path, module) in files {
	let should_ignore = !input_is_stdin && ignore_path_set.is_match(&path);

We should probably pass ignore_path_set to ModResolver and not parse the file that matches to the ignore pattern.

[calebcartwright]

The file is being parsed even though it is specified in ignore config option because we are filtering files with ignore after the module resolution, which requires parsing the file.

Ahh that makes sense, thank you so much! I spent a lot of time trying to figure out if I had a typo in the ignore setting 😄

We should probably pass ignore_path_set to ModResolver and not parse the file that matches to the ignore pattern.

👍 IMO that sounds like a useful change even independent of this PR. I can create a new issue to track that (feel free to assign to me), and will open up a separate PR for that change. In parallel I'll update this PR to remove the ignored-nested-test-files option over here (which will help simplify things) if that sounds good to you?

[topecongiro]

👍 IMO that sounds like a useful change even independent of this PR. I can create a new issue to track that (feel free to assign to me), and will open up a separate PR for that change. In parallel I'll update this PR to remove the ignored-nested-test-files option over here (which will help simplify things) if that sounds good to you?

SGTM. Thanks!

[calebcartwright]

CI failures seem unrelated AFAICT. Current master branch is also failing to compile against current nightly. Both this branch and master compile with passing tests against nightly-2019-10-13

[calebcartwright]

As this new behavior is on the cargo fmt side determining the correct set of files to pass to rustfmt, I wanted to ensure we had some automated tests. I decided to create some mini-sample projects under the tests directory and then added some tests here that use those samples against the functions that are determining the targets/files.

[calebcartwright]

I also made an udpate to fail if the user has included the --include-nested-test-files arg but we encounter an IO error while trying to discover those files.

I feel like this is the correct/desired behavior, as we wouldn't want to silently ignore such file discovery errors and give users the (potentially) incorrect impression that those files are formatted correctly.

[topecongiro]

LGTM, thank you for going through multiple updates!