Add `rustfmt` support #2648

tnull · 2023-10-05T19:35:43Z

Another attempt to address #410.

TODO:

Currently more or less blocked on resolution of allow optional programmer discretion on chain length/function arguments per line rust-lang/rustfmt#4306
- rustfmt devs plan to add support (see allow optional programmer discretion on chain length/function arguments per line rust-lang/rustfmt#4306 (comment))
- Could also use #[rustfmt::skip] blocks as an interim solution until that happens.
Add a CI check that ensures touched files are formatted.

codecov-commenter · 2023-10-05T19:49:16Z

Codecov Report

Attention: 795 lines in your changes are missing coverage. Please review.

Comparison is base (5bf58f0) 89.14% compared to head (5271c89) 89.97%.

❗ Current head 5271c89 differs from pull request most recent head 45c7439. Consider uploading reports for the commit 45c7439 to get more accurate results

Files	Patch %	Lines
lightning/src/ln/features.rs	30.98%	147 Missing ⚠️
lightning-net-tokio/src/lib.rs	62.50%	88 Missing and 2 partials ⚠️
lightning-invoice/src/utils.rs	92.81%	60 Missing and 1 partial ⚠️
lightning/src/events/mod.rs	61.71%	49 Missing ⚠️
lightning-background-processor/src/lib.rs	92.46%	27 Missing and 11 partials ⚠️
lightning/src/chain/chainmonitor.rs	89.21%	21 Missing and 16 partials ⚠️
lightning-persister/src/fs_store.rs	51.38%	33 Missing and 2 partials ⚠️
lightning-block-sync/src/gossip.rs	0.00%	34 Missing ⚠️
lightning-invoice/src/de.rs	85.63%	14 Missing and 11 partials ⚠️
lightning/src/chain/onchaintx.rs	91.43%	14 Missing and 11 partials ⚠️
... and 21 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2648      +/-   ##
==========================================
+ Coverage   89.14%   89.97%   +0.83%     
==========================================
  Files         116      112       -4     
  Lines       93205   116864   +23659     
  Branches    93205   116864   +23659     
==========================================
+ Hits        83089   105151   +22062     
- Misses       7583     9622    +2039     
+ Partials     2533     2091     -442

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

benthecarman · 2023-10-06T19:15:40Z

Huge concept ack

tnull · 2023-10-18T07:54:29Z

Now included a commit reformatting the project for preview purposes. Please anybody let me know if something in particular looks off and if we should change any of the config parameters. I'll try to accommodate wishes as far as possible, at least as long as nobody voices a conflicting opinion. When in doubt, I'll go with the default setting.

jkczyz · 2023-10-18T16:04:13Z

Another attempt to address #410.

TODO:

Currently more or less blocked on resolution of allow optional programmer discretion on chain length/function arguments per line rust-lang/rustfmt#4306

rustfmt devs plan to add support (see allow optional programmer discretion on chain length/function arguments per line rust-lang/rustfmt#4306 (comment))

Could also use #[rustfmt::skip] blocks as an interim solution until that happens.

This is a big one for me, though I'm not sure how practical adding #[rustfmt::skip] would be given how many places it would need to be added.

Add a CI check that ensures touched files are formatted.

jkczyz

Just scanned a few files and comments anything that jumped out as strange.

jkczyz · 2023-10-18T15:48:48Z

lightning-block-sync/src/gossip.rs

 impl<
-	A: Future<Output=Result<(BlockHash, Option<u32>), BlockSourceError>> + Unpin,
-	B: Future<Output=Result<BlockHash, BlockSourceError>> + Unpin,
-> Joiner<A, B> {
+		A: Future<Output = Result<(BlockHash, Option<u32>), BlockSourceError>> + Unpin,
+		B: Future<Output = Result<BlockHash, BlockSourceError>> + Unpin,
+	> Joiner<A, B>


Wondering why this gets an extra indentation? Is there a way to configure this?

lightning-block-sync/src/gossip.rs

lightning-block-sync/src/http.rs

jkczyz · 2023-10-18T15:58:10Z

lightning-block-sync/src/lib.rs

-
 #![deny(missing_docs)]
 #![deny(unsafe_code)]
-


Not great that it removes these given the comment may not apply to all the lines below.

Yeah, I'm not sure if there is a config option to leave this alone.

Btw. we could consider setting blank_lines_lower_bound/blank_lines_upper_bound to 1/2?

Eh, that would be more disruptive, IMO. It would put a bank line before the opening line of a method body and between statements, IIUC.

Oh my, totally missed that this also applies to each line inside bodies, thought this would only reformat lines between blocks. Forget what I wrote.

jkczyz · 2023-10-18T16:00:24Z

lightning-block-sync/src/lib.rs

-		let mut notifier = ChainNotifier {
-			header_cache: &mut chain.header_cache(0..=1),
-			chain_listener,
-		};
+		let mut notifier =
+			ChainNotifier { header_cache: &mut chain.header_cache(0..=1), chain_listener };


Would be preferable if it deferred to the author for how to format a struct initializer, IMHO.

jkczyz · 2023-10-18T16:06:37Z

lightning-background-processor/src/lib.rs

+						&Init {
+							features: nodes[j].node.init_features(),
+							networks: None,
+							remote_network_address: None,
+						},


Some of these would probably be better formatted if we extract something like this into a local variable. Makes me wonder if we should do a file-by-file transition to catch these.

Yeah, also happy to go this way if it would be preferred. We could go file-by-file, do some doc cleanups in the process (intended to do that for a while) until we have 80% or so of the codebase covered, at which point we could just bite the bullet and run fmt project-wide (and enforce it fully in CI).

rustfmt.toml

valentinewallace · 2023-11-27T19:17:33Z

rustfmt.toml

@@ -6,4 +6,3 @@ use_small_heuristics = "Max"
 fn_params_layout = "Compressed"
 match_block_trailing_comma = true
 overflow_delimited_expr = true
-format_strings = true


I personally like this option... it seems worth it to me even if we don't add custom skips, though that would be nice too.

rustfmt.toml

valentinewallace · 2023-11-27T21:20:13Z

lightning/src/util/chacha20.rs

-			let lens = [
-				a1,a2,a3,a4,
-				b1,b2,b3,b4,
-				c1,c2,c3,c4,
-				d1,d2,d3,d4
-			];
+			let lens = [a1, a2, a3, a4, b1, b2, b3, b4, c1, c2, c3, c4, d1, d2, d3, d4];


Could use a skip here.

valentinewallace · 2023-11-27T21:23:33Z

lightning/src/util/poly1305.rs

-	r         : [u32; 5],
-	h         : [u32; 5],
-	pad       : [u32; 4],
-	leftover  : usize,
-	buffer    : [u8; 16],
-	finalized : bool,
+	r: [u32; 5],
+	h: [u32; 5],
+	pad: [u32; 4],
+	leftover: usize,
+	buffer: [u8; 16],
+	finalized: bool,


Candidate for a skip, and several other custom whitespaces used for alignment in this file.

valentinewallace · 2023-11-30T21:27:02Z

lightning/src/ln/channel.rs

+	fn get_closing_transaction_weight(
+		&self, a_scriptpubkey: Option<&Script>, b_scriptpubkey: Option<&Script>,
+	) -> u64 {
+		let mut ret = (4 +                                                   // version


Some of the custom alignment is off in this method now.

domZippilli · 2024-01-10T22:33:51Z

Currently more or less blocked on resolution of allow optional programmer discretion on chain length/function arguments per line rust-lang/rustfmt#4306

rustfmt devs plan to add support (see allow optional programmer discretion on chain length/function arguments per line rust-lang/rustfmt#4306 (comment))

Could also use #[rustfmt::skip] blocks as an interim solution until that happens.

Reading that comment, it seems like this isn't a priority?

I can't and won't attempt to provide an estimate for when this new option might land on nightly. It's likely going to have to be accompanied by some other internal chain-related refactoring, and we've got various other priorities as well. However, portions of the changes needed to achieve this have actually already been implemented and merged (as they were independently beneficial), and I anticipate we'll be able to spend some bandwidth on this before the end of the year.

Finally, while I certainly appreciate the offers to help, I don't think the development of this new option is something that others can readily assist with; it's a complex part of the codebase that has a number of outstanding code-level design questions which also need to be considered alongside other needed changes. As such I don't think it would be practical for us to delegate nor mentor the work.

How onerous would it be to use #[rustfmt::skip]? Is it a lot of cases?

edit: I see Jeff's comment now that it is a lot of places. Any idea how many though? Or, could we live with too many lines in some places?

tnull · 2024-01-11T08:15:24Z

Currently more or less blocked on resolution of allow optional programmer discretion on chain length/function arguments per line rust-lang/rustfmt#4306

rustfmt devs plan to add support (see allow optional programmer discretion on chain length/function arguments per line rust-lang/rustfmt#4306 (comment))

Could also use #[rustfmt::skip] blocks as an interim solution until that happens.

Reading that comment, it seems like this isn't a priority?

I can't and won't attempt to provide an estimate for when this new option might land on nightly. It's likely going to have to be accompanied by some other internal chain-related refactoring, and we've got various other priorities as well. However, portions of the changes needed to achieve this have actually already been implemented and merged (as they were independently beneficial), and I anticipate we'll be able to spend some bandwidth on this before the end of the year.

Finally, while I certainly appreciate the offers to help, I don't think the development of this new option is something that others can readily assist with; it's a complex part of the codebase that has a number of outstanding code-level design questions which also need to be considered alongside other needed changes. As such I don't think it would be practical for us to delegate nor mentor the work.

How onerous would it be to use #[rustfmt::skip]? Is it a lot of cases?

edit: I see Jeff's comment now that it is a lot of places. Any idea how many though? Or, could we live with too many lines in some places?

The current thinking seems to be that when we do this we'll migrate the codebase file-by-file and clean up particularly bad cases, e.g., by pulling out more into variables before matching on them., so that the formatting of the actual match statements won't be as bad. Whatever is left then can be handled by a #[rustfmt::skip]. Given that 0.0.120 is around the corner it probably doesn't make sense to pick this up before, but I'd like to go for this approach after the release.

@jkczyz @valentinewallace @TheBlueMatt Does this sound like a reasonable plan to you?

TheBlueMatt · 2024-01-11T21:28:55Z

I'd be happy to see a PR for just one file (maybe a small one that will demonstrate some of the worst cases in our code, like onion_utils or so?) that uses the current rustfmt and moves things into variables to at least get the best we can to see how bad it is. Not sure if we want to commit to something before we at least have a sample?

jkczyz · 2024-01-11T23:51:09Z

Yeah, I'd also prefer a file-by-file approach.

We add some unstable featues that might be nice to have, but are currently unstable. So we probably don't want to use them right away.

coderabbitai · 2024-01-31T13:32:14Z

Important

Auto Review Skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit-tests for this file.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository from git and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

tnull · 2024-01-31T13:46:13Z

I'd be happy to see a PR for just one file (maybe a small one that will demonstrate some of the worst cases in our code, like onion_utils or so?) that uses the current rustfmt and moves things into variables to at least get the best we can to see how bad it is. Not sure if we want to commit to something before we at least have a sample?

Yeah, generally makes sense to go file-by-file. Given that we'll have to commit to one rustfmt.toml for the whole project I think it might make sense to go with a decently sized sample first, i.e., to increase the chances we'd be seeing the worst offenders in the first PR while we can easily make adjustments on-the-go without reformatting everything we already covered before.

So, last call for general comments here. Start of next week I'll close this general PR and open the first per-file PR. Might go with onion_utils.rs or even peer_handler.rs if nobody feels strongly.

tnull · 2024-02-06T15:33:23Z

I now opened the first rustfmt PR: #2877. Closing this.

tnull marked this pull request as draft October 5, 2023 19:35

tnull force-pushed the 2023-10-rustfmt branch from 0548f9f to 4281566 Compare October 18, 2023 07:53

jkczyz reviewed Oct 18, 2023

View reviewed changes

jbesraa mentioned this pull request Oct 22, 2023

Add CandidateRouteHop to channel_penalty_msat inputs #2551

Merged

tnull force-pushed the 2023-10-rustfmt branch 5 times, most recently from 59badbd to 5271c89 Compare October 30, 2023 08:28

valentinewallace reviewed Nov 27, 2023

View reviewed changes

rustfmt.toml Outdated Show resolved Hide resolved

valentinewallace reviewed Nov 27, 2023

View reviewed changes

jkczyz reviewed Nov 27, 2023

View reviewed changes

rustfmt.toml Outdated Show resolved Hide resolved

rustfmt.toml Outdated Show resolved Hide resolved

valentinewallace reviewed Nov 27, 2023

View reviewed changes

valentinewallace reviewed Nov 30, 2023

View reviewed changes

tnull added 7 commits January 31, 2024 14:30

Add rustfmt.toml

596963c

f Remove fn_single_line

afe5fba

f Remove match_arm_blocks

cb3a618

f Remove format_strings

3d25828

f Add UNSTABLE nice-to-haves

c5fedfc

We add some unstable featues that might be nice to have, but are currently unstable. So we probably don't want to use them right away.

f Drop chain_width

74997cb

PREVIEW

45c7439

tnull force-pushed the 2023-10-rustfmt branch from 5271c89 to 45c7439 Compare January 31, 2024 13:31

dunxen mentioned this pull request Feb 1, 2024

Handle self payments #2573

Open

tnull mentioned this pull request Feb 6, 2024

rustfmt: Add CI scripts and format onion_utils.rs #2877

Merged

tnull closed this Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `rustfmt` support #2648

Add `rustfmt` support #2648

tnull commented Oct 5, 2023 •

edited

Loading

codecov-commenter commented Oct 5, 2023 •

edited

Loading

benthecarman commented Oct 6, 2023

tnull commented Oct 18, 2023 •

edited

Loading

jkczyz commented Oct 18, 2023

jkczyz left a comment

jkczyz Oct 18, 2023

jkczyz Oct 18, 2023

tnull Oct 28, 2023

jkczyz Oct 30, 2023

tnull Oct 30, 2023

jkczyz Oct 18, 2023

jkczyz Oct 18, 2023

tnull Oct 28, 2023

valentinewallace Nov 27, 2023

valentinewallace Nov 27, 2023

valentinewallace Nov 27, 2023

valentinewallace Nov 30, 2023

domZippilli commented Jan 10, 2024 •

edited

Loading

tnull commented Jan 11, 2024

TheBlueMatt commented Jan 11, 2024 •

edited

Loading

jkczyz commented Jan 11, 2024

coderabbitai bot commented Jan 31, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

tnull commented Jan 31, 2024

tnull commented Feb 6, 2024

Add rustfmt support #2648

Add rustfmt support #2648

Conversation

tnull commented Oct 5, 2023 • edited Loading

codecov-commenter commented Oct 5, 2023 • edited Loading

Codecov Report

benthecarman commented Oct 6, 2023

tnull commented Oct 18, 2023 • edited Loading

jkczyz commented Oct 18, 2023

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domZippilli commented Jan 10, 2024 • edited Loading

tnull commented Jan 11, 2024

TheBlueMatt commented Jan 11, 2024 • edited Loading

jkczyz commented Jan 11, 2024

coderabbitai bot commented Jan 31, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

tnull commented Jan 31, 2024

tnull commented Feb 6, 2024

Add `rustfmt` support #2648

Add `rustfmt` support #2648

tnull commented Oct 5, 2023 •

edited

Loading

codecov-commenter commented Oct 5, 2023 •

edited

Loading

tnull commented Oct 18, 2023 •

edited

Loading

domZippilli commented Jan 10, 2024 •

edited

Loading

TheBlueMatt commented Jan 11, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)