Useless blank lines added when removing extern crate statements
jimmycuadra opened this issue · 10 comments
Here is a diff created by running cargo fix --edition-idioms on a piece of code:
diff --git a/src/main.rs b/src/main.rs
index 900f3d2..3c06c49 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,10 +1,10 @@
-extern crate futures;
-extern crate hyper;
-extern crate hyper_tls;
-extern crate serde;
+
+
+
+The four blank lines it substituted for the extern crate statements it removed are useless and just create a bunch of odd looking space in the source code. It'd be better to just remove the lines without replacement.
This is semi-no-a-bug in the sense that we figured rustfmt would clean up after rustfix if necessary, but it would probably make sense to improve the diagnostics in rustc to just go ahead and remove the newline
If someone can point me to where this happens in rustc, I can submit a PR.
The span is inserted here. It is originally taken from the extern crate item here. We'd probably just want to expand the span during the reporting though, so we could do it in https://github.com/rust-lang/rust/blob/4a45578bc58ff262864f72680cc02e83f5d2f5b3/src/librustc_typeck/check_unused.rs#L84
Note that if this is pursued, the code complexity of that lint is increased significantly. You need to take care of all kinds of things like
- if you remove the newline before the
extern crate, you need to check that you are not in the first line of a file - you need to actually check for newlines/spaces, ... and not just remove a character
- you need to consider the effect of macros
I think we should just leave this to rustfmt. In clippy we have generally opted to do such fixes only if it is absolutely trivial.
Is there a way to signal to end users that rustfix intends for rustfmt to be run immediately afterward? This isn't intuitive to me at all, hence coming here with this issue. It never occurred to me that I was just expected to run rustfmt to fix ugliness left behind by rustfix. Just seems like a less than stellar UX as it stands.
We could add a way to "rustfmt after fix" to ease this workflow.
This isn't intuitive to me at all, hence coming here with this issue. It never occurred to me that I was just expected to run rustfmt to fix ugliness left behind by rustfix.
The thing is, while it would be totally preferable to have rustc and clippy produce perfect suggestions, they would end up replicating a lot of rustfmt code (there's basically no way to share that code, without just plain running rustfmt).
Some things are probably even impossible to catch without full rustfmt. E.g. when a line gets longer after being fixed and then suddenly is over the rustfmt length.
Just changing the UI of cargo fix might be a satisfactory approach. Maybe just adding something to the output of running it suggesting a subsequent rustfmt run, or at the least just part of rustfix's help text. Maybe even cargo fix --fmt that automatically shells out to rustfmt if the fix completed successfully.
Wondering if there's been any progress on this issue since it's been reported, since it's still an issue. While I do think that a fix --fmt option would be nice, I think that removing the newline in all cases would be helpful, since there are definitely situations where you would like to run cargo fix without reformatting an entire file.
Perhaps the reformatting could be scoped to the parts that have been fixed, somehow? Maybe that's something that would have to be implemented on rustfmt's end. For example, one thing that could be useful is a cargo fix affecting imports could just run the import reformatting rules, reordering the imports at the upper level of the file while leaving all other items intact.
Closing in preference to rust-lang/rust#51176, since this would need to be fixed in the compiler.