RFC: proposed rustfmt settings for the project

[bunnie]

Proposing a "flag day" where rustfmt gets run on the entire repo. It will make diffs against stuff older than the "flag day" harder, but will hopefully make patches easier in the future as we start to take on more contributors & PRs.

Some background -- both @xobs and I have Opinions about formatting and they are different. For me at least, for code that I maintain, I'm particular about rustfmt's tendency to squish things into a small number of columns and consume lots of vertical space, which directly impacts my ability to read the code (see example below).

The proposed middle-ground solution is to adopt +nightly rustfmt, which finally has native support in vscode. I took the template proposed in its comment thread and modified it.

The primary modification is expanding the column width to 110 characters. This allows users to continue to contribute code with tall/narrow orientation if that is their preference, but doesn't eliminate wider code styles that allow better use of vertical real estate.

The rationale is that in the 90's I had SVGA computers with 132 column displays and 1/6th the screen area, so 110 characters seems like splitting the difference between 80 cols and 132 cols, allowing me to take better use of large, modern 4k displays, but potentially putting users coding primarily on mobile or small laptop screens at a disadvantage.

The other possibly controversial decision is the flag to automatically split imports into std/core/crate sets (but not forcing alphabetical ordering), under the reasoning that I tend to format my imports in logical groups and in order of attention significance, but I'm open to hearing arguments against that rationale.

Anyways, I ran these settings on the most "challenging" cases from my perspective and can live with the results.

Would love to hear opinions from all contributors (looking through commit logs -- @gsora @nworbnhoj @kotval @tmarble @rowr111 @samblenny @eupn @jeandudey, @Hoverbear maybe others I'm missing...) since the idea is we would make a rustfmt pass mandatory going forward, to make less formatting differences on patchsets going forward as we grow our base of contributors.

I'll give a week or so for this to proposal to gain consensus, and if there aren't any major objections will move ahead; if we can't achieve consensus, then I would abandon this PR and keep with the status quo.

---

A direct example of the narrow-formatting problem is something like this. I would prefer code that read like this:

    pub fn vibe(&self, pattern: VibePattern) -> Result<(), xous::Error> {
        send_message(self.conn,
            Message::new_scalar(Opcode::Vibe.to_usize().unwrap(), pattern.into(), 0, 0, 0)
        ).map(|_|())
    }

with default settings, rustfmt would produce this:

    pub fn vibe(&self, pattern: VibePattern) -> Result<(), xous::Error> {
        send_message(
            self.conn,
            Message::new_scalar(
                Opcode::Vibe.to_usize().unwrap(),
                pattern.into(),
                0,
                0,
                0,
            ),
        )
        .map(|_| ())
    }

The original line is 91 columns wide, so rustfmt takes every arg including the 0's and sticks them on their own line. Unfortunately a series of 0 arguments is a common idiom in Xous, and in some libraries like llio it causes the vertical space to explode and bury more significant lines of code.

Note that the rustfmt setting would allow users to still write code that puts 0s on lines by themselves if that is their preference; it simply doesn't force it until the line width exceeds 110 characters.

I will note that the default width of this text box I'm using to enter this github comment is 135 columns.

[bunnie]

One small thing I found out while reading the rustfmt docs is that if @generated appears within the first five lines of a file, it's excluded from formatting.

If this PR is accepted, we should also add to our code generators (blitstr font tables, xtask for timestamps and app menus) to avoid commits that fight with the formatter.

[gsora]

The primary modification is expanding the column width to 110 characters.

LFTM, less than that and it becomes an hamburger made of few chars.

The other possibly controversial decision is the flag to automatically split imports into std/core/crate sets

My dayjob involves writing and reviewing lots of Go code, in which this is routine: in my opinion it makes navigating a big code file quite simpler, and I'm all for it.

It also looks neat!

---

Having project-wide, universally accepted (not necessarily loved) code formatting settings is the way to go for a huge codebase like Xous.

I find code that looks uniform less hard to navigate and contribute to - this is probably due to some hints of ADHD on my side, but hey this is the world we live in :^) - so I strongly believe we should move forward with this proposal.

Although, I'm not sure we should allow for any exceptions, like the 110 chars one: a project's code should all look the same if there are well-known settings.

But! I'm fine even if we keep it, I just love having everything rustfmt'd.

---

On Matrix I alluded to some strong opinions of mine, so here we go: no PR should go through if it isn't formatted correctly.

One can achieve this with a combo made of pre-commit hooks and a GitHub Action.

The development workflow looks like this:

1. the developer installs pre-commit on their machine and runs pre-commit install from within the xous-core directory: it'll set up the necessary dependencies, and sets up a local git pre-commit hook to run them
2. development flows as expected
3. at commit time, one of two things can happen
   a. code is formatted correctly already, commit gets written
   b. code is not formatted correctly, commit is aborted, code gets formatted automatically

In case 3.b, developer is alerted through a git error message, and they just need to commit again for it to go through: pre-commit formatted code for them.

[nworbnhoj]

I am relatively new to rust, and the transition into this language has been more challenging than any since I learned to code back in the dark ages. We all develop a person style over time, and mine was untenable in the context of Rust Ownership and verbose match blocks. So I have been focused on "how things are done around here" (in both Rust and xous). My Rust style is most certainly still under active development as I slowly move beyond simply surviving Rust, and start stretching my legs a little.

I do have a clear preference for a consistent style within a project. Policy backed by automated formatting are the only way to go IMO. But the question is: which policy?

Short answer: I think 110 columns is a sensible width for today.

Musing follows ....

Transparency and practical audits is a (the) central tenant of Precursor/xous. It is very important to be able to comprehend what is going on in the code, and to be able to easily review the places where unwelcome code might be hiding, and unexpected code-paths that might be taken. I find rusts match blocks both a blessing and a curse in this regard. Mixing up "normal" code with "error-handling" tends tends to result in a vertical explosion containing a minority of "normal code". On the other hand, the match blocks make it easy to appreciate all possible code-paths. All of this musing to agree that vertical is something of a problem/frustration.

The diffs either side of "flag-day" do not concern me so much, because I am late to the party. So the sooner we have the "flag-day" the better, so more code is post "flag-day"

Personally, I would split imports into std/core/crate sets AND force alphabetical ordering. But this is not a biggie IMO.

The vertical rustfmt example (above) of the call to send_message() makes it easier while actively working with the fn call, and trying to figure out just which value is getting passed to which argument. The vertical 0,0,0,0 are annoying the rest of the time.

There are other examples in the xous code-base where the match blocks are perhaps just a bit too deep, and cause a vertical explosion with 80 col rustfmt. One answer is to add more columns; another is to break out the deeper blocks into fn calls. I have been experimenting with a "parallel" match pattern that may be another approach to the "vertical" problem (or maybe not).

match (
    fn a(),
    fn b(),
    fn c(),
) {
    (
        Ok(a),
        Ok(b),
        Ok(c),
    ) => a + b + c,
    (Err(e), _, _) => log::warn!("a problem"),
    (_, Err(e), _) => log::warn!("b problem"),
    (_, _, Err(e)) => log::warn!("c problem"),
    (_, _, _) => panic!(),
}

Here is an extreme case - which would traditionally result in a match blocks 20 deep!

[kotval]

I agree with pretty much everything above, including a "flag-day" sooner rather than later. I don't much care what the preferred style is as long as it is there and completely automated. I use "format on save" in Emacs so I can just focus on typing as fast as possible and disregard the code layout. When I finish something logical, I immediately save and let it format.

I do like a longer line width than the default, and think 110 is fine to avoid the Stack of Blankness. I really just want code density. A line too long that I can't read it is annoying, but if there are only a few such lines in a file, I can focus on the important bits better. A line with almost no characters on it is worse because it waste screen real estate.

I will note that sometimes breaking the rules makes sense, but there is #[rustfmt::skip] for that. We should permit use of that as long as it's applied with at least some decent reason. I would personally use this quite a lot in an effort to optimize code density in the 2d grid.

One style that I would like for us to adopt, though I'm not sure if this can be put in rustfmt is to disallow use something::*; My text editor config idiosyncrasies make me have an usually strong preference towards this (no lsp mode, no vscode), and I think it is pretty good style. I also am very much in favor of grouped imports in alphabetical order within each group.

One other things which is not specifically rustfmt, but is style related, is that I think that we should disallow rust code generation during build. Macros and whatnot are completely fine as is generating other file formats, but a build.rs which calls a command line protobuf compiler disgusts me. If you have to use such a tool, I think the appropriate thing is to commit the generated code directly, and mention how it was made. There are two reasons for this, one is auditability, the other is comprehensibility. I think currently I am the only one dealing with this, but trying to dig through other people's rust that uses such things wasted a fair bit of my time, so I'd like to not perpetuate that.

[samblenny]

Having a tool-supported standard code formatting policy sounds like a great idea. I don't have any strong opinions about what the details of that policy should be.

My main feedback is, for whatever policy might get decided on, it would be good to consider and document how developers can apply that policy using their preferred operating system and code editor.

For example, if you want to have a client-side pre-commit githook to run rustfmt --check using a shell script, it would be good to test that script against bash (Linux), zsh (macOS), and whatever shell Windows uses these days. If you do decide to implement an automated rustfmt check for pull requests or commits, perhaps doing those checks server-side on GitHub would be easier to test and maintain due to the more predictable shell environment.

For documenting how to invoke rustfmt, I'm guessing that once you commit a policy to rustfmt.toml, the normal rustfmt invocation methods will work fine. In that case, the main readme for rustfmt at https://github.com/rust-lang/rustfmt has subheadings titled "Running rustfmt directly" and "Running Rustfmt from your editor" that look relevant.

[bunnie]

Thank you everyone for the productive input!

Let me try to aggregate everything I've seen so far, and if I've missed a key point, please let me know.

• We have consensus around 110 columns.
• Alphabetical ordering of imports and modules is important. I am now convinced, the PR has been modified to reflect this.
• Consensus seems to be that formatting should be mandatory - I am on board with this.
• I'd also like to add that we should strip trailing whitespaces by default. This isn't a rustfmt option but if there's a way to automatically strip those from projects I'd love to hear suggestions.
• We need tooling to enforce & aid developers to format code

On that last point, I will need guidance & help on tooling. This is one of my weak points.

Tooling

TL;DR

Would it be possible to just have github actions run rustfmt and automatically reject a PR if it hasn't been formatted (and can github actions do things like strip trailing whitespaces)? And then back that up with a list of resources for making properly formatted code, starting with the simplest command-line invocation of formatting with nightly rust, and escalating into fancy automated tools and scripts you can add to your environment or editor?

If the answer to the first question is yes, then could someone either help to write such a github action (could even be a PR to this PR!) or point me to an example of how it is done?

Thoughts on automated tooling

I am trying to wrap my head around pre-commit. Is it the case that developers have to install pre-commit for these checks to run? Is there no built-in feature for e.g. command line github clients to trigger a pre-commit action? I'm confused if pre-commit is a wrapper around some existing git client action or if it's a whole separate subsystem that does fancy things.

I don't like the idea of mandating developers to run any third party program on their machines in order to contribute to the repo -- everyone has their reasons to trust or not trust a tool. If pre-commit can be a "recommended best practices" that makes your contributions easy and around 80% of developers would be OK with it, I'm OK with recommending the flow. But more ideal would be a baseline flow that requires minimal specialized tooling and then suggested flows on top of that to streamline for devs who are okay with trusting anything that comes off of pypi.

For context, I'm already a little leery of the fact that the existing rustfmt.toml only works if you have done a rustup install nightly, because basically any useful formatting option in rustfmt is strictly a nightly option (and will be forever - /me grumbles). This chews up over a gigabyte of disk space, slows down rustup update, and generally increases the bloat of the project for the sole purpose of formatting, but I think I might be an outlier in feeling uncomfortable about that. Obviously, I've gotten over that hump but these are the sorts of things that keep me up at night.

So my preference is that if users wrote proper code manually, as tedious as that might be, a PR could be accepted (no need to even run rustfmt if they're good at writing properly formatted code); but we should enforce proper coding standards at the PR level, and facilitate developers with a set of options that accommodate a range of tooling preferences. Whether a developer prefers to code in ed on a VT100 terminal over acoustic modem or they prefer to code using bleeding-edge npm and pypi-powered ML enhanced editors that barely keeps up on an M3 Pro with a fiber line, I will not judge (so long as they have equivalent code quality).

fwiw, my personal flow is a vscode setting to run rustfmt on save, with a plugin to strip trailing whitespaces, and a local llama instance. So I'm closer to the latter than the former developer.

Protecting Main

A final side-point is that @xobs has recommended we protect the main branch from direct commits and requiring a PR. I agree with this (even though it will make some chores a bit more annoying), so concurrent with merging this RFC we'll flip that bit.

Thanks again to everyone who took the time to add their thoughts to this thread. I appreciate your feedback and I am happy to see that we seem to be converging toward consensus.

[gsora]

Would it be possible to just have github actions run rustfmt and automatically reject a PR if it hasn't been formatted (and can github actions do things like strip trailing whitespaces)?

Yes! If we decide to go with pre-commitwe can run it in Actions and have it fail on non-zero exit status.

I am trying to wrap my head around pre-commit. Is it the case that developers have to install pre-commit for these checks to run? Is there no built-in feature for e.g. command line github clients to trigger a pre-commit action? I'm confused if pre-commit is a wrapper around some existing git client action or if it's a whole separate subsystem that does fancy things.

pre-commit is just a fancy tool to organize and commit in the repo all the tooling that need to run on the pre-commit git hook, because normally you can't commit that.

I've been using it at various workplaces, although I understand the threat model of Xous is quite different (but maybe not that far off?).

Consider also that developer can choose not to set up pre-commit and eventually see their PR blocked from merging.

and a local llama instance

Oh, imma need details on this 👀

we protect the main branch from direct commits and requiring a PR.

100% agree, protecting main is a must in any FOSS/public project!

[kotval]

I strongly agree that whatever automation should be simple enough that users on different setups don't have trouble using it, and not prescribe any particular setup, and I'm happy @bunnie feels the same. Ideally, we'd be able to explain what the tools did in such a way that you could accomplish the same things manually. As a principle example, personally hate pre-commit (1) it modifies the worktree and breaks git stash. 2) it transparently installs bloatware on my machine, 3) it usually fails to install said bloadware on my machine, breaking my ability to use any git command unless I disable it.) and refuse to run it. I'd be fine running the individual scripts it calls though as long as they aren't bloaty, though pre-commit is literally by it's own admission a "a multi-language package manager" and the epitome of DevOps' worst ideas. Because it transparently grabs tools from other language's package repos, that actually makes using it and letting users opt out rather tricky. I bring it up to say, if you are going to use it, please be very conservative, and ideally only use tools that one can either build from source for the system, install with cargo, or install from pypi. Since it installs random junk from who knows where, it usually negates the benefits of using if you want to make it easy for users to opt out. I'm totally fine with it running on Actions and telling me what I did wrong, so I can fix it, but often it is hard to articulate what the tool does.

System packages are up to a user, and we only rely on reasonable things, which is good. I love that almost everything just needs the rust toolchain and plain python, but I think our use of python could use some attention, especially if we're about to open the pre-commit mire.

At a minimum, I'd like for us to at least pin the deps in a requirements.txt (or pyproject.toml) with hashes, and we should be wary of introducing while supply chain security holes so that we can get nice formatting or what not. Since pip can execute arbitrary code during install, the more random packages you include, the worse. (So, can npm (and pre-commit loves npm junk) and the js developers have a habit of writing tiny packages that do next to nothing except operating as the perfect vector for randomware after they get abandoned. Google it and find numerous examples of such shenanigans.) Python being python tends to end up with a lot of transitive deps, and using plain pip doesn't mean you get pinned version, but often deps like "whatever > 1". This isn't that big of a deal for the code running on the github CI runners since it shouldn't have any actual secrets, but I like to be careful when installing python packages on my local system. If we wanted to get a little more modern with our python tooling, I highly recommend poetry. It's as close to Cargo as you can get in python, utilizing lock files with hashes, venv management, and just generally pretty good tooling, while also not inventing it's own package format (looking at you conda). If we used poetry to manage anything that we might want for something like pre-commit (as well as the stuff in our tools directory like the updater), then even users who don't want to use that can still pip install the deps that it puts in its lock file (or the subset they need to fix a particular issue). Poetry also gives you analogs of cargo tree which is useful for analyzing bloat. But if we start adding hooks for pre-commit aren't in pypi, crates.io, or something a user could reasonably be expected to install with the system package manager, then I'd be pretty sad.

As someone who is far closer to the developer using "ed on a VT100 terminal over acoustic modem", (which sadly only means I'm using Emacs on a Mac and often in remote areas where I get abhorrently slow internet) I really appreciate not being forced to either install whatever operating system is currently in vogue just to contribute or spend an unreasonable amount of time trying to get things installed to match the system the developers use.

a local llama instance
I am also curious. My experience with co-pilot was so bad, I ditched it and never went back, I'd be interested in using it for rust.

[samblenny]

Assuming things may go in the direction of a server-side status check that needs to pass before new PRs can be merged, here are some background links on a possible way to go about setting up such a thing. I haven't tried this, but, from my reading, it looks like git pre-commit stuff would not be required. I think this can all be done with GitHub stuff?

Stack Overflow Q&A:

1. "Can I use a GitHub Action to patch pull requests before running validating actions?": https://stackoverflow.com/questions/71756808/can-i-use-a-github-action-to-patch-pull-requests-before-running-validating-actio
2. "Github Actions: is there a way to make it mandatory for pull request to merge": https://stackoverflow.com/questions/60776412/github-actions-is-there-a-way-to-make-it-mandatory-for-pull-request-to-merge
3. "How to auto-reject a pull request if tests are failing (Github actions)":https://stackoverflow.com/questions/58654530/how-to-auto-reject-a-pull-request-if-tests-are-failing-github-actions

GitHub Documentation:

1. "Managing protected branches": https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches
2. "Require status checks before merging": https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging

[bunnie]

I think we are converging on server-side checks to enforce rules on formatting, with it up to the user to decide how to get past the checks. And ideally a well-curated and not overwhelming list of options for newcomers to navigate the formatting hurdle.

One other thing I'm going to put out there -- in terms of a "Flag Day" where we make patches/cherry picks/etc more difficult to revert due to formatting changes, my thought is to do it crate-by-crate, probably over a short period of time, rather than the whole repo all at once in a single mega-commit after running format on everything. The idea being that if we did encounter a crate that is broken by formatting changes, we can undo the formatting patch for that one crate easily.

And yah, I know of at least one crate that will be broken by alphabetical ordering of modules -- the graphics server's font list depends on the order of modules to lay them out in memory correctly (the order in the file directly corresponds to link order). They are currently ordered biggest font - to - smallest font order, in part to improve L2 cache re-use by clustering all the small fonts together, and in part to make it less painful to make fixes to small font sets that are frequently used. @xobs stepped on this grenade already and we have a work-around for it that is compatible with rustfmt (using @generated, putting a space in between each module line), but, it is one solid example where rustfmt broke something we didn't expect it to.

I'd love to hear any maintainer's wisdom about best practices conducting a flag day change like this.

[samblenny]

I made a demo repo with a .yml file that implements a nightly rustfmt check with max_width=110:
https://github.com/samblenny/autofmt-demo

• The yaml file defining the GitHub Action for the check:
  https://github.com/samblenny/autofmt-demo/blob/main/.github/workflows/rustfmt_check.yml
• Example PR with failing check (line is too long):
  Add 113 char line samblenny/autofmt-demo#4
• Example PR with passing check:
  Add 108 char line samblenny/autofmt-demo#7

Misc Notes

1. The ubuntu-latest runner image already has rustup installed, so it's possible to use run: rustup ... shell commands without taking any special actions to install rustup.
2. If you just do the basic code checkout of the PR feature branch with uses: actions/checkout@v4 (see https://github.com/marketplace/actions/checkout), then the rustfmt.toml file that gets used will be the one from the feature branch. It might make sense to take extra steps to overwrite the feature branch's rustfmt.toml with the one from main.
3. Deciding what trigger to use for running the check is a little tricky. I settled on on: push because I'm guessing that will work reasonably for re-running the check after commits to feature branches that are intended to fix the failing check. If the on: pull_request trigger was used, I'm not sure if fix commits would cause the check to be re-run. I'm unsure of the usage quota implications of running the check for every commit. There are a variety of other trigger options, some of which might result in significantly fewer CI runner hours over time: https://docs.github.com/en/actions/using-workflows/triggering-a-workflow

[samblenny]

Here is a shell command that could be used with run: ... to recursively detect rust source files with trailing whitespace:

find . -name '*.rs' -print0 | xargs -0 grep -c --with-filename '[ \t]$' \
 | grep -v ':0$' | perl -e 'while(<>){print;$n+=1};exit ($n>0)?0:1;'

The algorithm is:

1. find makes a list of *.rs files by recursively searching from the current directory
2. xargs feeds the filenames from find to grep
3. grep generates a count of lines in each file that match the ends-with-whitespace pattern
4. the second grep filters out lines with a match-count of 0
5. the perl script counts the number of non-zero-count files while echoing their names to the console, then returns a status code of 0 for zero files with whitespace lines or 1 for one or more files with whitespace lines

Since the perl script prints the filenames to the console, those names should end up in the status check log for the GitHub Action. So, theoretically, people could check that log to figure out which files had extra whitespace. It would work, but it wouldn't be super easy to use. Some other tool might be nice to have.

[samblenny]

I made another GitHub Action status check file that uses find, xargs, grep, and perl to look for .rs files that have trailing spaces. It seems like maybe rustfmt also picks up on the trailing spaces, but its output format is less than helpful (just a mysterious diff).

• The yaml file for the trailing space status check:
  https://github.com/samblenny/autofmt-demo/blob/main/.github/workflows/trailing_whitespace_check.yml
• Example PR with failing check:
  Add line with trailing whitespace to .rs file samblenny/autofmt-demo#10
• Example PR with passing check (note markdown allows trailing spaces):
  Add line with trailing spaces to markdown file samblenny/autofmt-demo#14

Feel free to copy the yaml stuff from my demo repo. That's what I wrote it for.

[bunnie]

OK, sorry for all the commits. The only way I know to test a github action is to...push it.

This amends the PR to absorb the trailing whitespace and format checks (thanks @samblenny). As we can see, the status of this PR is a failure, because the automation is in the PR and we haven't cleaned up the repo to meet the standard yet.

I'll probably try to merge this and move the repo over to meeting its own code criteria with a series of commits over the weekend...during the process we'll have a lot of automation failures, but I presume that's OK and I can still merge even if the automation fails.

[bunnie]

• Added a formatting guideline to help new users get started.
• Modified .vscode/settings.json to have the default settings recommended for users
• updated release notes

I think that does it?

[bunnie]

Just to be fully transparent - I plan to merge the PR this weekend and format Xous to meet the checks within it.

It will take some time for the full set of changes to roll out, a lot of iterations through CI to make sure nothing breaks. I'll try to remember to drop a note in chat once that's done.

Thanks to all for your help!

[eupn]

@bunnie Somehow I missed the entire discussion but I'm happy to see this. We're using a rustfmt config in our fork that excludes kernel and loader files to avoid unrelated diffs due to formatting but with formatting finally introduced over the whole codebase this problem will go away. I don't see any downsides tbh.