Skip to content

Rustfmt Maximalism #3749

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Rustfmt Maximalism #3749

wants to merge 2 commits into from

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Apr 25, 2025 •
edited
Loading

Just enable it globally. We can take care of format refactors later or as we update the code, and this way we immediately eliminate the ongoing formatting micro-penalties.

To auto-format in-flight PRs without manual conflict resolution:

Let's say commit X is the commit where all of main is rustfmt'ed. Nothing other dan cargo fmt should have been done in this commit.

  • First rebase the branch normally onto the predecessor of X (still without formatting) to resolve any outstanding conflicts.
  • Create and checkout a new branch for the rewrite off of X.
  • Run the following script, replace BRANCH with the branch to replay:
#!/bin/bash

echo Start

BRANCH="2025-04-rustfmt-tests-1"
for commit in $(git rev-list --reverse HEAD..$BRANCH); do
    echo "Commit: $commit"
    git restore --source=$commit .

    cargo fmt

    git add .
    git commit -c $commit --no-edit --allow-empty
done
  • Now every commit in the new branch is rustfmted and exists on top of formatted main.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Apr 25, 2025 •
edited
Loading

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@joostjager joostjager marked this pull request as ready for review April 25, 2025 13:15
@joostjager joostjager removed the request for review from wpaulino April 25, 2025 13:16
@tnull tnull requested a review from TheBlueMatt April 25, 2025 16:02
tnull
tnull approved these changes Apr 25, 2025
View reviewed changes
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed I get the same output by running cargo fmt

valentinewallace
valentinewallace approved these changes Apr 25, 2025
View reviewed changes
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK if you review any follow-ups to fix things up after this :)

lightning/src/ln/peer_handler.rs
Comment on lines +879 to +897
'a,
'b,
'c,
'd,
'e,
'f,
'logger,
'h,
'i,
'j,
'graph,
'k,
'mr,
SD,
M,
T,
F,
C,
L,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sheesh

benthecarman
benthecarman approved these changes Apr 25, 2025
View reviewed changes
Copy link
Contributor

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're so back

@joostjager joostjager requested review from jkczyz and wpaulino April 25, 2025 21:47
jkczyz
jkczyz reviewed Apr 25, 2025
View reviewed changes
lightning/src/ln/channel.rs
Comment on lines +3729 to +3730
flags & FundedStateFlags::ALL
== FundedStateFlags::LOCAL_SHUTDOWN_SENT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fits on one line. Is there something about our config that would cause it to split it over two lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that rustfmt wants to put the == on the same line, pushing the total == FundedStateFlags::LOCAL_SHUTDOWN_SENT | FundedStateFlags::REMOTE_SHUTDOWN_SENT to column 101. So expected behavior afaik.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it's bitwise not logical OR.

@joostjager joostjager requested a review from jkczyz April 26, 2025 00:09
@joostjager joostjager added the weekly goal Someone wants to land this this week label Apr 26, 2025
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

2 similar comments
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

carlaKC
carlaKC approved these changes Apr 28, 2025
View reviewed changes
Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, matches local cargo fmt

ci/rustfmt.sh
for file in $(comm -23 "$TMP_FILE" rustfmt_excluded_files); do
echo "Checking formatting of $file"
rustfmt $VERS --edition 2021 --check "$file"
done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONTRIBUTING.md needs update to remove reference to contrib/run-rustfmt.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one, will fix after rebase

@joostjager joostjager mentioned this pull request Apr 29, 2025
Open
@ldk-reviews-bot
Copy link

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

1 similar comment
@ldk-reviews-bot
Copy link

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

dunxen
dunxen approved these changes Apr 30, 2025
View reviewed changes
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually leaning toward this PR's approach and biting the bullet. I don't believe it's that cumbersome to existing PRs with your suggested solution.

Running cargo +1.63 fmt (MSRV-shipped rustfmt), I get the same output.

@joostjager
Copy link
Contributor Author

To be sure we wouldn't get any problems with in-flight prs, I tried the filter-branch command from the PR description on #3751: joostjager/rust-lightning@main-fmt...joostjager:rust-lightning:rustfmt-tests-rewritten

@joostjager
Copy link
Contributor Author

Another option that could be used is #[rustfmt::skip] on the method level, for code that turns out unreadable beyond personal preference.

@TheBlueMatt
Copy link
Collaborator

After rebasing my own rustfmt stuff I realized that the main rebase logic won't actually work in some cases around indentation changes. Because git sometimes mis-identifies hunks as non-conflict it'll end up duplicating some lines.

@joostjager
Copy link
Contributor Author

joostjager commented May 2, 2025 •
edited
Loading

After rebasing my own rustfmt stuff I realized that the main rebase logic won't actually work in some cases around indentation changes. Because git sometimes mis-identifies hunks as non-conflict it'll end up duplicating some lines.

I didn't run into this yet, but had identified the potential problem too. Updated the PR description with a new approach that strictly avoids any type of merge logic / automatic conflict resolution.

@ldk-reviews-bot
Copy link

🔔 3rd Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 3rd Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager joostjager mentioned this pull request May 3, 2025
Merged
@ldk-reviews-bot
Copy link

🔔 4th Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

1 similar comment
@ldk-reviews-bot
Copy link

🔔 4th Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager joostjager mentioned this pull request May 5, 2025
Open
@ldk-reviews-bot
Copy link

🔔 5th Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

1 similar comment
@ldk-reviews-bot
Copy link

🔔 5th Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnull tnull

@dunxen dunxen

@valentinewallace valentinewallace

@benthecarman benthecarman

@carlaKC carlaKC

@TheBlueMatt TheBlueMatt

@wpaulino wpaulino

@jkczyz jkczyz

Assignees
No one assigned
Labels
weekly goal Someone wants to land this this week
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@joostjager @ldk-reviews-bot @TheBlueMatt @tnull @dunxen @jkczyz @valentinewallace @benthecarman @carlaKC