Integrate rustfmt #425

valentinewallace · 2019-12-19T22:35:11Z

Closes #410. @jkczyz let me know any initial feedback!

~~Some notes:~~
~~1) I stuck with only "stable" features of rustfmt, of which there are only 14~~
~~2) I went with 80-character lines because I like being able to have many vim panes side-by-side, but it looks a bit weird at times. 100 might be better.~~
~~3) Travis will fail if we run check the formatting w/ multiple rust versions, so I chose stable for the lightning crate and 1.34.2 for the fuzz crate.~~

Problem: the build fails on 1.22.0 -- rustfmt only works on 1.24.0 and beyond. Are there any plans to deprecate support for 1.22.0, and/or could we possibly bump the minimum supported version of RL to 1.24.0?

TODO:
[ ] add a bunch of #[rustfmt::skip]s to places that benefit from being aligned in a specific way

TheBlueMatt · 2019-12-20T20:14:38Z

Nice. 80 char-wide is really really awkward for some of our long-names types and for many of our many-parameter functions. If anything I'd vote for something more like 150, even.
How stable is rustfmt across recent versions? Does 1.40 produce something different from 1.39 from 1.38? If it does, I'm not sure its really a candidate for everyday use, no?

valentinewallace · 2019-12-20T22:24:31Z

How stable is rustfmt across recent versions? Does 1.40 produce something different from 1.39 from 1.38?

1.40 the same as 1.39, but slightly different from 1.38. So, it's not the most stable. But, can't we all just decide on a canonical version up-front?

Nice. 80 char-wide is really really awkward for some of our long-names types

Hm, true. Upped to 120, and it seems much improved, lmk if you still prefer >=150.

TheBlueMatt

There's a few awkward results here, but generally looks good.

TheBlueMatt · 2019-12-23T20:48:03Z

lightning-net-tokio/src/lib.rs

-				Ok(pause_read) => {
-					if pause_read {
+		tokio::spawn(
+			reader


This is super awkward. Anything we can do with the defaults so it doesn't make a mess like this?

I think changing this default might help: https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#function-calls

Sure, lets try that.

Should be better with this new change!

TheBlueMatt · 2019-12-23T20:49:45Z

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -65,7 +89,16 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
 	if disconnect {
 		nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 		nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
-		reconnect_nodes(&nodes[0], &nodes[1], (true, true), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));


This also feels a bit awkward, no? Can we make it just put a line per thing if the total line is too long, not just all the time?

I'm used to this look from previous formatters but fine with trying something different.

Can we make it just put a line per thing if the total line is too long, not just all the time?

IIUC that is what it is: https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#tall-default
We could change it to this instead: https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#compressed

Hmm, my question was more about when rustfmt decides to put it on new lines...I kinda assumed it was "too long" by char count, but looks like this one shouldn't hit that metric?

I think the formatter fudges the character count a bit such that this line is barely over the limit: https://i.imgur.com/EWGbOBk.png
We could increase the character count again from 120 to 150. that does seem a bit long to me, but not a hill i'm willing to die on 😛

Hmm...So my smallest terminal is 140-wide, though I presume I'm a minority here. Maybe bump again and see where we end up.

Functions with large number of arguments may signal a need to refactor the code. Here, the arguments consisting of long sequences of literals aren't very meaningful to the reader.

Generally, I'd prefer if lines aren't longer than 100-120 characters. @valentinewallace Does fn_args_layout apply to function definition or calls (or both)?

@jkczyz looks like it applies to both. I also prefer 100-120 character max lines.

jkczyz · 2020-01-02T19:17:12Z

fuzz/src/chanmon_consistency.rs

-			monitor)
-		} }
+			(ChannelManager::new(Network::Bitcoin, fee_est.clone(), monitor.clone(), broadcast.clone(), Arc::clone(&logger), keys_manager.clone(), config, 0).unwrap(), monitor)
+			}};


These look like instances of rust-lang/rustfmt#3068 or rust-lang/rustfmt#3858. We'll likely get churn until that is fixed. The first issue seems to imply that it is only a problem when using tabs for indentation.

Does disabling format_macro_bodies prevent this? We could do that until the issue is fixed.

Done! good idea.

Oh, note that adding unstable features means we can only run the formatter on the rust nightly channel though, which requires adding another travis build and also potentially extra code churn over time. I'm fine with all that, but worth pointing out.

jkczyz · 2020-01-02T19:36:52Z

fuzz/src/chanmon_consistency.rs

 						events::MessageSendEvent::UpdateHTLCs { ref node_id, .. } => {
-							if *node_id != drop_node_id { true } else { false }
-						},
+							if *node_id != drop_node_id {
+								true
+							} else {
+								false
+							}
+						}


The verbosity here could be eliminated by changing the match arm's body simply to*node_id != drop_node_id. Not sure if it's worth going down the rabbit hole fixing things of this nature, though.

Hm, new issue, maybe?

fuzz/src/chanmon_consistency.rs

lightning/src/chain/keysinterface.rs

TheBlueMatt · 2020-01-06T21:54:17Z

My biggest concern would be code churn. The only thing worse than no common format is a common format that requires regular changes and PRs with conflicts.

valentinewallace · 2020-01-08T16:30:34Z

My biggest concern would be code churn.

@TheBlueMatt is this concern because I switched to using the nightly version?

TheBlueMatt · 2020-01-08T19:12:38Z

Right, as you indicated it might cause churn, but also any inconsistencies in rustfmt would as well - you said 1.38 has different results from 1.39/1.40 which is also concerning. I presume its only getting better so I'm not objecting to it in practice, only noting that any decisions we make wrt policy should prioritize stability of output over most other issues, and that probably means not using unstable features until they stabilize.

valentinewallace · 2020-01-09T00:07:15Z

Gotcha, that would also necessitate the removal of indent_style = "Visual" which solved the awkward parts you mentioned in this comment.

@jkczyz objections to prioritizing stability over the usage of match_block_trailing_comma, format_macro_bodies and fn_single_line?

jkczyz

Gotcha, that'll necessitate the removal of indent_style = "Visual" which solved the awkward parts you mentioned in this comment.

@jkczyz objections to prioritizing stability over the usage of match_block_trailing_comma, format_macro_bodies and fn_single_line?

Was writing this up independently but it seems to pertain here...

TL;DR: I'd prefer stability, too, given my assessment below. But without format_macro_bodies disabled, we're going to get some bad formatting (misaligned braces). Maybe we can find a suitable workaround though.

There appears to be many more unstable than stable ones. I assume enabling them means we are getting the defaults for those even if we don't specify them in the config.

My opinion on the ones we're setting after looking through the code:

match_block_trailing_comma: Nice for consistency but can do without until stable if necessary.
format_macro_bodies: Disabling is a workaround for the bugs mentioned in Integrate rustfmt #425 (comment). We wouldn't want to disable this once they are resolved. Another workaround is to stop using hard_tabs if I'm reading the bug correctly. @TheBlueMatt Should we assume switching to spaces is not an option?
indent_style: Visual indentation of arrays (ec2b4ed#diff-c48f1714e930339b99af4720509c9404R235) and struct initialization seems a bit unnatural (ec2b4ed#diff-39afe74fe704e5d3029c0fd96f7db3b1R212). Applying this across all types of expressions doesn't seem ideal, but it is all or nothing apparently.
fn_single_line: Forces single-line functions rather than just allowing them. Not ideal when line limit is long, IMHO.

Additionally, max_width is making for long array initializations go across as many lines as elements, which is pretty awkward. But perhaps this is limited to testing/fuzzing code and could be mitigate somehow if undesirable.

jkczyz · 2020-01-08T21:41:26Z

fuzz/src/peer_crypt.rs

 #[inline]
 fn slice_to_be16(v: &[u8]) -> u16 {
-	((v[0] as u16) << 8*1) |
-	((v[1] as u16) << 8*0)
+	((v[0] as u16) << 8 * 1) | ((v[1] as u16) << 8 * 0)
 }


Add #[rustfmt::skip].

valentinewallace · 2020-01-09T17:52:01Z

I'd prefer stability, too, given my assessment below. But without format_macro_bodies disabled, we're going to get some bad formatting (misaligned braces).

@jkczyz there's about ~100 macros (at least according to rg "macro_rules" | wc -l). I could add #[rustfmt::skip] to all of them as a stopgap, which I'd sort of prefer over switching from tabs to spaces.

jkczyz · 2020-01-09T18:13:02Z

I'd prefer stability, too, given my assessment below. But without format_macro_bodies disabled, we're going to get some bad formatting (misaligned braces).

@jkczyz there's about ~100 macros (at least according to rg "macro_rules" | wc -l). I could add #[rustfmt::skip] to all of them as a stopgap, which I'd sort of prefer over switching from tabs to spaces.

Good call! 👍

D4nte · 2020-04-20T11:30:36Z

Can I help with this?

TheBlueMatt · 2020-04-21T00:12:48Z

Right, probably time to rerun this and check to see how stable rustfmt is across rust versions. Probably makes sense to take this after we land the few big-uns for 0.0.11.

valentinewallace · 2020-04-22T18:35:10Z

my real question is how stable is rustfmt across rust versions?

Here's the stability historically (pretty stable IMO):
1.40 == 1.39 !=(1 line of difference) 1.38 == 1.37 == 1.36 == 1.35 == 1.34

Ran it on the freshest 2 versions, and cargo +1.42.0 fmt == 1.41.0 == 1.40.0.

TheBlueMatt · 2020-04-29T20:10:58Z

Cool. Wanna rebase this on master?

codecov · 2020-04-29T20:42:42Z

Codecov Report

Merging #425 into master will decrease coverage by 0.07%.
The diff coverage is 84.01%.

@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
- Coverage   91.12%   91.04%   -0.08%     
==========================================
  Files          34       34              
  Lines       20544    26049    +5505     
==========================================
+ Hits        18720    23716    +4996     
- Misses       1824     2333     +509

Impacted Files	Coverage Δ
lightning/src/lib.rs	`100.00% <ø> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.41% <ø> (+0.07%)`	⬆️
lightning/src/ln/channel.rs	`86.91% <ø> (+0.49%)`	⬆️
lightning/src/ln/channelmanager.rs	`83.17% <ø> (-2.29%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.80% <ø> (+0.30%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.39% <ø> (-0.66%)`	⬇️
lightning/src/util/config.rs	`69.04% <0.00%> (ø)`
lightning/src/util/events.rs	`19.58% <0.00%> (-0.85%)`	⬇️
lightning/src/util/transaction_utils.rs	`94.36% <ø> (-4.29%)`	⬇️
lightning/src/ln/wire.rs	`70.22% <34.09%> (+4.07%)`	⬆️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9098240...a7b8d3b. Read the comment docs.

valentinewallace · 2020-04-29T23:53:23Z

@TheBlueMatt Update:
With these two lines added:

use_small_heuristics = "Max"
fn_args_layout = "Compressed"

the stability is this:
1.42.0 == 1.41.0 == 1.40.0 == 1.39.0 != 1.38.0 (2 lines of difference) == 1.37.0

And 1.36.0 and below are broken due to fn_args_layout

If I remove fn_args_layout and keep use_small_heuristics:
1.42.0 == 1.41.0 == 1.40.0 == 1.39.0 != 1.38.0 (2 lines of difference) == 1.37.0 == 1.36.0 == 1.35.0 != 1.34.0 (diff pictured here: https://i.imgur.com/cUkAZXJ.png)

EDIT: spoke too soon, build broken now. Gimme a sec

- Add skips to places where lists get over-expanded. - Add variables to avoid some long function call lines. - Prevent some end-of-line comments from being put onto next line. - re: ChannelContext skip, avoid rustfmt removing a comma because the comma's causes a compilation error

TheBlueMatt · 2020-04-30T03:06:15Z

Opened an issue upstream for the somewhat absurdly tall code this seems to generate. rust-lang/rustfmt#4146

TheBlueMatt · 2020-09-27T01:30:27Z

Until rustfmt upstream gets its act together and generates sane code, lets close this. We still have an issue to track it.

valentinewallace force-pushed the integrate-rustfmt branch from 115efc4 to 78acc22 Compare December 19, 2019 22:56

valentinewallace changed the title ~~Integrate rustfmt~~ [WIP] Integrate rustfmt Dec 19, 2019

valentinewallace force-pushed the integrate-rustfmt branch 4 times, most recently from fc7cf66 to 706413f Compare December 20, 2019 18:48

valentinewallace force-pushed the integrate-rustfmt branch 2 times, most recently from 8e9f5ed to b99199d Compare December 20, 2019 22:17

valentinewallace force-pushed the integrate-rustfmt branch 3 times, most recently from c20773b to b8de015 Compare December 21, 2019 00:42

TheBlueMatt reviewed Dec 23, 2019

View reviewed changes

jkczyz reviewed Jan 2, 2020

View reviewed changes

valentinewallace force-pushed the integrate-rustfmt branch from b8de015 to b0ed084 Compare January 6, 2020 21:29

valentinewallace changed the title ~~[WIP] Integrate rustfmt~~ Integrate rustfmt Jan 6, 2020

valentinewallace requested a review from jkczyz January 6, 2020 21:37

valentinewallace force-pushed the integrate-rustfmt branch 3 times, most recently from d06112e to 8a5bcb6 Compare January 6, 2020 21:46

valentinewallace force-pushed the integrate-rustfmt branch 2 times, most recently from e196f92 to 4cd92bf Compare January 8, 2020 00:43

jkczyz reviewed Jan 9, 2020

View reviewed changes

valentinewallace force-pushed the integrate-rustfmt branch from b4ccb00 to 90bb8fa Compare February 26, 2020 17:42

TheBlueMatt mentioned this pull request Apr 19, 2020

Include height to incorrect_or_unknown_payment_details failure #596

Merged

ariard added the Easy to Review label Apr 27, 2020

valentinewallace force-pushed the integrate-rustfmt branch from 90bb8fa to f4371a2 Compare April 29, 2020 20:41

valentinewallace force-pushed the integrate-rustfmt branch 5 times, most recently from 5a53aab to ff7d747 Compare April 29, 2020 21:08

Fix situations where rustfmt broke custom readability.

30f55ee

valentinewallace force-pushed the integrate-rustfmt branch 4 times, most recently from 00645b6 to 12f1054 Compare April 29, 2020 23:41

valentinewallace added 6 commits April 29, 2020 20:14

rustfmt: skip all macros due to bug in rustfmt (see issues 3068, 3858)

441899d

Update fuzzing generated code to conform to rustfmt.

2fbe48b

Add rustfmt config.

0c2066a

Initial run of rustfmt.

410fbf6

Integrate rustfmt into travis and Github Actions.

a7b8d3b

valentinewallace force-pushed the integrate-rustfmt branch from 12f1054 to a7b8d3b Compare April 30, 2020 00:18

TheBlueMatt closed this Sep 27, 2020

Integrate rustfmt #425

Integrate rustfmt #425

Conversation

valentinewallace commented Dec 19, 2019 • edited Loading

TheBlueMatt commented Dec 20, 2019

valentinewallace commented Dec 20, 2019

TheBlueMatt 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

valentinewallace Dec 23, 2019 • edited Loading

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

TheBlueMatt commented Jan 6, 2020

valentinewallace commented Jan 8, 2020 • edited Loading

TheBlueMatt commented Jan 8, 2020

valentinewallace commented Jan 9, 2020 • edited Loading

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Jan 9, 2020

jkczyz commented Jan 9, 2020

D4nte commented Apr 20, 2020

TheBlueMatt commented Apr 21, 2020

valentinewallace commented Apr 22, 2020

TheBlueMatt commented Apr 29, 2020

codecov bot commented Apr 29, 2020 • edited Loading

Codecov Report

valentinewallace commented Apr 29, 2020 • edited Loading

TheBlueMatt commented Apr 30, 2020

TheBlueMatt commented Sep 27, 2020

valentinewallace commented Dec 19, 2019 •

edited

Loading

valentinewallace Dec 23, 2019 •

edited

Loading

valentinewallace commented Jan 8, 2020 •

edited

Loading

valentinewallace commented Jan 9, 2020 •

edited

Loading

codecov bot commented Apr 29, 2020 •

edited

Loading

valentinewallace commented Apr 29, 2020 •

edited

Loading