Theseus icon indicating copy to clipboard operation
Theseus copied to clipboard

Run `rustfmt` / `cargo fmt` on source code

Open xa888s opened this issue 2 years ago • 5 comments

Many files have inconsistent formatting which can make it harder to understand the code. Running rustfmt on all files would ensure that the formatting is consistent.

xa888s avatar Jun 11 '22 02:06 xa888s

Do you want this, Kevin? I've noticed you've got a lot of careful manual formatting, vertically aligning declarations and match blocks and whatnot. I'm a huge fan of rustfmt but it will sometimes make code look worse.

An in-between option would be to add #[rustfmt::skip] to preserve the existing formatting in places rustfmt really mangles. Of course, doing that would take a good bit of manual scrutiny.

Also, are you happy with the default Rust style guide formatting? If you want to customize anything in rustfmt.toml best to do that upfront...

jkugelman avatar Jun 17 '22 01:06 jkugelman

I mentioned some of my thoughts here: https://github.com/theseus-os/Theseus/pull/530#issuecomment-1154547570

I'm not opposed to it, but we would indeed need to do some heavy customization of rustfmt before letting it loose on the entire code base. Some of our older crates would definitely benefit from formatting fixes. Unfortunately I don't necessarily have the time to devote to studying and customizing rustfmt, so in #530 I've proposed a similar strategy to satisfying clippy: incrementally apply it to groups of crates until we're satisfied with its behavior.

kevinaboos avatar Jun 17 '22 03:06 kevinaboos

I have tried getting vscode's settings.json "editor.formatOnSave": true to work with the suggestion in https://github.com/theseus-os/Theseus/pull/530#issuecomment-1155969831, but that might have to wait for a while: https://github.com/rust-lang/rust-analyzer/issues/10826. I also tried

"rust-analyzer.rustfmt.extraArgs": [ "--config-path ." ],

so you don't have to ;)

For now I did this: #637

hecatia-elegua avatar Sep 13 '22 17:09 hecatia-elegua

@hecatia-elegua ah, I completely missed your comment above, hence my confusion in #637.

Disclaimer: i haven't looked at rustfmt in a while, so my understanding of its features may be out of date.

I'd vote to start with the crates in applications/ and kernel/, since those are the main first-party components of Theseus.

The first and easiest thing we need to enforce is a maximum line width (column count). Let's start with a soft limit of 100 columns and a hard limit of 110 columns.

Another big concern with rustfmt is that it seems to eagerly split statements across multiple lines, e.g.,

let result = me.iter().filter(...).find().next();

becomes

let result = me
    .iter()
    .filter(...)
    .find()
    .next();

This is okay when each statement is long, but when the statements are short (let result = me.iter().next();) it becomes tedious and less easy to read. That's the main issue I have, and it seems like chain_width is one of the relevant settings there.

Another thing I've noticed in PRs that have run rustfmt is that it destroys manual vertical alignment of variable/const/static definitions. Hopefully a setting is available to preserve that.

Finally, in general I prefer many small PRs over one big PR as it's easier to review that way (and easier to handle merge conflicts). So, perhaps a good way to go about this is to submit a PR that adds one rustfmt setting and includes changes to one file as an example, and then once we've settled on the specifics of that file's changed formatting, then we can apply that one change to other files. Then we'll incrementally add more rustfmt settings in future PRs, and also set up a CI action to ensure new code changes abide by those formatting rules. We've done that with clippy and it has been relatively painless so far.

kevinaboos avatar Oct 03 '22 18:10 kevinaboos

Formatting imports would be an improvement too, as they're kind of messy and inconsistent. I like the following options:

  • imports_granularity: Crate
  • group_imports: StdExternalCrate

Also it looks like the use_small_heuristics = Max would address most of my complaints about verbose line splitting.

kevinaboos avatar Oct 03 '22 19:10 kevinaboos