Skip to content
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

rustfmt removes empty newlines between blocks of module-level attributes #4082

Open
RalfJung opened this issue Mar 14, 2020 · 5 comments · May be fixed by #5438
Open

rustfmt removes empty newlines between blocks of module-level attributes #4082

RalfJung opened this issue Mar 14, 2020 · 5 comments · May be fixed by #5438

Comments

@RalfJung
Copy link
Member

RalfJung commented Mar 14, 2020

In a situation like

#![feature(core_intrinsics)]
#![feature(lang_items)]
#![feature(libc)]
#![feature(nll)]
#![feature(panic_unwind)]
#![feature(staged_api)]
#![feature(std_internals)]
#![feature(unwind_attributes)]
#![feature(abi_thiscall)]
#![feature(rustc_attrs)]
#![feature(raw)]
#![panic_runtime]
#![feature(panic_runtime)]

// `real_imp` is unused with Miri, so silence warnings.
#![cfg_attr(miri, allow(dead_code))]

rustfmt removes that empty line. That's not great, as grouping these attributes by "paragraphs" conveys semantic meaning.

rustfmt preserves empty lines between statements, so it seems like the semantic value of that information is acknowledged, but it is not done consistently.

@calebcartwright
Copy link
Member

Some related conversation in #2954. Hopefully this can be supported via the blank_lines* config options too

@dhardy
Copy link

dhardy commented Jul 4, 2022

The linked issue appears to be about inserting empty lines, which is very different from removal of blank lines.

In my opinion, rustfmt may reduce multiple concurrent blank lines down to a single one (though even this is somewhat dubious), but should never completely remove empty lines separating non-empty lines in the default config.

@calebcartwright
Copy link
Member

I don't think there's any value in debating relevance, but to elaborate...

The overarching point is that different users want different behavior when it comes to empty lines, and the approach to address this in other context entails configuration options that allow users to set both a maximum and minimum line count which gives them control over whether rustfmt output will have lines maintained/added/removed relative to the input.

rustfmt doesn't directly process your input files and individual whitespace characters, so when we talk about potential solutions to requests we do so in the context of how rustfmt operates and not the more abstract user perception. "don't remove blank lines" is a desired outcome but not a tangible/actionable solution in and of itself. The blank_lines configuration option paradigm is likely the only viable approach here too, particularly given the constraints around changes to default formatting behavior. However, it still has unresolved bugs and questions, hence the applicability of what we've seen/discussed/etc. of that paradigm in other contexts.

@calebcartwright calebcartwright removed this from the 3.0.0 milestone Jul 4, 2022
@dhardy
Copy link

dhardy commented Jul 4, 2022

particularly given the constraints around changes to default formatting behavior

I don't see how that's an issue, when we're talking about stopping removal of existing lines.

Surely rustfmt does read existing whitespace tokens? I know it can determine when imports are separated by empty lines.

@calebcartwright
Copy link
Member

I'm not keen on getting into a debate on how rustfmt operates; it works with the AST as produced by rustc_parse which does not explicitly represent whitespace. The fact that the AST node spans contain a respective representation from the input files (albeit with some whitespace homogenization) doesn't change the fact.

No one has said that the requested behavior is impossible, and in fact most of the discussion has been about the ways through which rustmt could support the desired end formatting.

You're welcome to submit a PR if you want, but we're not going to change the default behavior and any resolution needs to take into account the breadth of user needs/desires so we don't end up in yet another scenario of satiating one camp while taking away from another.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants