Isn’t the no-brainer/minimalist and presumably straightforward approach something like “if a file was fully cargo-fmt formatted before then the file should be formatted after else behave like currently”? Could there be any downsides to this approach?
This could be combined with warning that fires if the file in question was not found to be correctly formatted yet.
Further opt-in or settings aren’t really required IMO anyway. In cases where people would want the file completely cargo fmt-formatted, but it hasn’t been in a well-cargo fmt-ed, I don’t see any benefit in prompting the user to “please cargo fmt first” if they could just as well run cargo fmt afterwards and be done already, with the same end result.
Maybe the suggestion could even include the command that would format just the files in question – would have been my next sentence, but looking into this some more, is it really true that stable rustfmt does not support single-file formatting (i.e. without other files reachable via mod foo;) yet!?
A mode to “always run cargo fmt, too” seems unnecessary as well, because cargo fix isn’t supposed to replace cargo fmt anyway, right? So all proper users would also be using cargo fmt manually, anyway, (or equivalent functionality through their IDE).
Thinking of possible downsides anyway…
…the only remaining somewhat unsupported use-case is when users want to e.g. use stable cargo fix in a project where nightly rustfmt is used and configured in a place that the cargo fix-invoked rustfmt would pick up, that motivates simply also emitting the same warning that the resulting changes might not be ideally formatter; perhaps with an added help or note that describes the failure case.
The only false behavior would be in the rare case where
rustfmt (or alternative Rust formatting infrastructure) that the user actually uses is configured somewhere else – or a different stable version – but happens to consider the file in question well-formatted before the fix, too, as does the actual tool the user uses
- then after the fix, formats it into a state that no longer matches the formatting tooling that user actually uses
- so there was no warning, but the result wasn’t the “desired” formatted result the user would have wanted
Though this is not worse than the status quo, anyway.
Aaaand I’m noticing a second possible case of misbehavior:
rustfmt sometimes “gives up” on sections of code leaving them untouched as a result
- this means that if the fix happens in such a section, the result could be not-properly-formatted (needing to be looked at by the user) but
rustfmt behavior itself gives no indication of this happening
- this means that if the
cargo fix change happens to remove the issue that rustfmt got stuck on, then subsequent formatting of the file can affect a larger section of code, in principle
- this means that if the
cargo fix change happens to introduce a code pattern that rustfmt “gives up” on, the result could be not-properly-formatted without warning
Only the second case would be an actual possible regression; but I also think this is rarely all that problematic because rustfmt “giving up” is often well-restricted to small sections of code anyway.
Ideally, rustfmt should have a mode where it simply tells use the set of ranges of file positions that it “gave up on” and if the cargo fix diffs overlap with those, a warning could be generated.
Probably, the behavior should still be the same “for each file separately: if a per-file cargo fmt --check operation blessed the file’s previous state, then run a per-file formatting after the fix”. The main case the warning addresses is where the fix applies to an non-formattable section, and thus manual review by the user is might be required for a good-looking result.
I do think all of this can be left as a future possibility though.
Just like a range-based mode can be a future possibility. (Looks like rustfmt already has unstable functionality for formatting ranges within a file; so in the future with this, it might be possible to transition the per-file based logic to a per-diff based logic, but maybe that should wait for the relevant rustfmt feature to be stabilized.)
Edit: Another example I’m just thinking of is rustfmt::skip on a code section; though that would probably have all the same implications & relevant mechanisms… Realistically, the only reason why we would want to warn here is “hey, you should look at this code, it might not be formatted do your desires”, but realistically reviewing of cargo fix changes “look good” is the user’s job anyway, and the main concern that we’re instead solving is to avoid cases where the result does look like (and is) perfectly valid and well-formatted code, but happens not to be cargo fmt’s exact choice of formatting. This main concern does not apply with rustfmt::skip sections or sections rustfmt “gives up” on.
A last concern is that that occasionally rustfmt itself can break code. In theory there could be a case where cargo fix would change code correctly and only subsequent formatting breaks it. It’s usually the opposite, i.e. cargo fix seems more likely to break code itself in the first place, and there’s already counter measures in place that check vcs so that no state is lost unless the user opts in to it. This should be rare and the user still has the compiler warning anyway, so between [case 1:] a slightly incorrect suggestion from cargo check where cargo fix breaks your code, and this new case [case 2:] of the suggestion from cargo fix being 100% correct and only the conditional automatic subsequent formatting making the code slightly wrong… the UX should be quite similar. They can always try acting on the warning manually; and might not even notice the difference between these cases.