-
Notifications
You must be signed in to change notification settings - Fork 899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Explain spans of the input file which were excluded from the formatter #5683
Comments
Thanks for reaching out. It may be subtle, but I think there's actually a lot of territory covered in a relatively short description. As for the first part, I think it's important for anyone interested in this type of information to be sure they are running rustfmt with the relevant existing options enabled, including error_on_line_overflow and error_on_unformatted. They're not perfect, but they can help surface some of the very same type of info. As you also noted, macros are also a common source of this, and that's intentional, even if not as widely known as we'd like (macro calls that use brace delims are ignored by design). For the second part around "why", I think that's significantly more tricky to do in practice (would be roughly analogous to rustc explaining the "why" behind ICEs), and IMO would not have as much long term utility as it may seem at first blush. For example, currently when rustfmt encounters a let-else statement it will simply re-emit the original contents of the span, and that's also intentional for reasons detailed elsewhere (tl;dr the formatting rules haven't been codified yet). I could see it being valuable for rustfmt to notify users of that fact on the first run, but I think it would become unwelcome noise from the second run onward that would quickly be ignored. Similarly, I'd suggest it would also quickly become unwelcome if every run of rustfmt incorporated a bunch of output about skipped macro calls. tl;dr - I'd agree there's some improvements that could be done on the first part, but I'm skeptical of both the feasibility and utility of the second |
I didnt know about those two settings, and doubt I would have found them if you hadnt mentioned them. Much appreciated. I enabled IMO it would be great if those settings were default to a third state of "warn but dont error", at least whilst on nightly. This provides discoverability of these settings without breaking peoples CI, and would provide both "where and why" for a subset of the reasons, and I would be likely to dig into these warnings over time, and raise bugs for these. To minimise noise, it could emit only a few warnings of each type, and/or emit how many warnings are being squelched. Another approach would be to have a setting for "percentage of the source file that should be reformatted", and an warning is emitted if the reformatter has to skip more than that percentage of the source. If set to a reasonable default (50% at first?), it will surface when users had a reasonable expectation of formatting happening, and it clearly did not. I dont know if there are current sufficient mechanism for users to tell rustfmt they know and accept formatting isnt occurring. e.g. For the "why" aspect that you are skeptical about, I would be happy if the extra noise was all hidden behind |
I think the challenge with doing something new/different that's still off-by-default would leave us in the same place where 👇
continues to be the status quo. I believe I forgot to mention this, but one of things we're actually already planning on doing is adapting those options to be enums with some set of silent/warn/error variants vs. their current boolean off/on nature. We'd also like to shift them to be warn by default instead of silent by default to help address the current state (the unknowns/incorrect presumption of successful formatting/etc.) I'd also like to be able to support the main thrust of your request (at least from my pov) around highlighting the portions of the code that weren't able to be formatted explicitly (vs. current state of silence unless they happen to have something like trailing whitespace in which case users get a somewhat vague "internal error" style message). This does pose some technical problems for us, both in terms of internal architecture constraints (we've ideas on how to proceed on this front but it will be a big haul) and the general challenge of trying to map the span from the original input to the corresponding span in the output (in cases where tl;dr, A lot of this is doable and on our roadmap, but will take time. The "why" part is probably one that will be punted on still, and we'll likely have to rely on users coming to us with questions like "why is rustfmt saying it couldn't format this Foo in my code", and we'll just have to deal with answering/pointing to the respective issue. |
Often I see sections of source code which do not get reformatted, and afaics there is no way to know a) it was skipped, and b) why it was skipped.
I know of macros as a common source of this, c.f. #8 , but sometimes that doesn't explain why a section of the source code was not formatted.
I think it would be helpful if rustfmt was able to inform the user why it skipped spans of the input file while checking, or spans of the output file if reformatting. This will increase awareness in the Rust community of the current limitations of rustfmt, so they know when not to rely on it, likely also steering developers to using syntax which is compatible with rustfmt if they want to rely on rustfmt more, and possibly bringing in more contributors to rustfmt.
The text was updated successfully, but these errors were encountered: