update chain handling of long lines and block-like parents #3884

calebcartwright · 2019-10-25T01:27:03Z

Resolves #3863, resolves #2970, and, I believe, resolves #3157 as well.

Apologies that this PR is on the larger side, though worth noting that it mostly consists of test files. As I'm sure folks are well aware, chains are tricky 😄, so I included a lot of tests for my own sanity (happy to remove anything that seems duplicative/unnecessary).

I also had to change my approach quite a few times while working on this and didn't do a great job of squashing commits along the way so I'd probably advise against trying to review this commit-by-commit. Instead I'd suggest reviewing the files associated with the update for the two referenced issues, and I've included a summary of the respective changes below for each issue accordingly.

Issue 3863 - handling of long chain elements

Currently, rustfmt will fail to format a chain if that chain contains one or more elements that exceeds max_width which has surfaced in a few issues (#3863 and rust-lang/rustup#2097 for example).

This PR updates the handling of chains to ensure that rustfmt will still be able to format the chain even if there's a long item. There's one edge case (when indent_style is Visual and the long chain item is the chain's parent) that's not perfect, but IMO is still an improvement over not being able to format the chain at all.

Files associated with this update:

src/chains.rs (all the changes except the fn format_chain_parent_inner() function that starts on line 730)
src/expr.rs (See my inline comment in this file as this change may not be necessary/helpful)
tests/source/chains_long_item_block.rs
tests/source/chains_long_item_visual.rs
tests/target/chains_long_item_block.rs
tests/target/chains_long_item_block.rs

Issue 3157 - configurability of chain indents for chains with block-like parents

The current state is described in far better detail in #3157, but the gist is that when the chain parent is block-like, then the chain elements are not indented like they are with a simple chain parent.

Based on that thread, my sense is that there's some subjective/stylistic-preferences around the indentation flavors. As such the approach I've proposed in this PR leverages two new configuration options (with the default indent behavior being rustfmt's current style/approach for these types of chains) to give users some flexibility and to support the requested formatting in #3157

Files associated with this update:

Configurations.md
src/chains.rs (just the fn format_chain_parent_inner() function that starts on line 730)
src/config/mod.rs
src/config/options.rs
tests/source/chains_multiline_parent_always.rs
tests/source/chains_multiline_parent_never.rs
tests/source/chains_multiline_parent_simple_calls.rs
tests/source/chains_multiline_parent_tuples_and_simple_calls.rs
tests/target/chains_multiline_parent_always.rs
tests/target/chains_multiline_parent_never.rs
tests/target/chains_multiline_parent_simple_calls.rs
tests/target/chains_multiline_parent_tuples_and_simple_calls.rs

Configurations.md

src/chains.rs

calebcartwright · 2019-10-25T01:51:38Z

src/expr.rs

-                (Some(orig_rhs), _) => Some(format!(" {}", orig_rhs)),
+                (Some(orig_rhs), _) => {
+                    let prefix = if orig_rhs.starts_with('\n') { "" } else { " " };
+                    Some(format!("{}{}", prefix, orig_rhs))


While working on this I tried a few different things to get the formatting to look right (especially for chains with long elements when using the Visual indent style). This change was added to handle some of those approaches that started with a newline in order to prevent the error[internal]: left behind trailing whitespace error from occurring.

The ultimate approach I went with does not need this, so I can remove this check if folks would prefer (though I do wonder if it may help with some other/unrelated trailing whitespace errors)

Configurations.md

src/chains.rs

calebcartwright · 2019-11-20T02:08:26Z

I think this is probably finally ready for review whenever folks get a chance, apologies it took so long!

calebcartwright · 2019-11-20T02:10:35Z

Configurations.md

@@ -389,6 +389,255 @@ fn example() {
 }
 ```

+## `chains_block_parent_indent_children`


I always have such a hard time coming up with config names 😄 If this approach seems viable for handling the scenario in #3157 (and it should indeed be configurable) then this should probably be renamed to something more succinct

calebcartwright · 2019-11-20T02:13:25Z

src/chains.rs

+    // Whether one or more of the inner chain items exceed the max width
+    has_long_inner_item: bool,
+    // Whether the final chain item exceeds the max width
+    has_long_tail: bool,


instead of a single has_long flag, these fields are used to have a more detailed view of where the chain has a long element based on how chains are formatted (parent and tail are formatted separately)

calebcartwright · 2019-11-20T02:16:28Z

src/chains.rs

+        wrap_str(rewrite, max_width, shape).is_none()
+    }
+
+    fn format_chain_parent_inner(


This entire function (which runs to line 832) is to support #3157 (indentation for chains with block-like parents), all the other changes in this file are to address #3863 (long chain elements)

calebcartwright · 2019-11-20T02:19:42Z

tests/source/chains_long_items_block.rs

+    ).foo().bar().baz();
+}
+
+fn long_parent() {


While working on this I discovered quite a variety of chain scenarios (position of long element in the chain, rhs of an assignment vs. free, etc.) that had to be explicitly handled, so there's test cases for each of those scenarios

calebcartwright · 2019-11-20T02:28:19Z

tests/target/chains_long_items_visual.rs

@@ -0,0 +1,167 @@
+// rustfmt-indent_style: Visual
+
+fn long_parent() {


This scenario was the most difficult to deal with (Visual for indent_style with the long element being the chain parent).

As I mentioned in the PR description this is definitely not perfect, but probably still better than not formatting the chains at all. I felt like this approach was the better of the evils, as doing the typical Visual indent with this scenario would basically result in the entire chain body being off screen (as all the child elements would be shifted to the right of the end of the long parent).

In the future, it might be possible to leverage some of the heurstics added here to improve upon this scenario (for example checking whether the first child is long, etc.) but this feels like quite an edge case to me and I need a break from chains for a bit 😁

calebcartwright · 2020-01-06T00:03:22Z

@topecongiro - I'm going to close this and then open a new PR with the changes. that should make it easier to review, provide a cleaner commit history, and allow me to avoid dealing with all the merge conflicts

superhawk610 · 2020-08-05T16:49:25Z

@calebcartwright any updates? #3157 linked here but I wasn't sure if anything else had been done since.

calebcartwright · 2020-08-05T18:16:59Z

Sorry @superhawk610, but no updates. There's some other discussions and decisions that will impact this relative to major version upgrades and the stability guarantee which need to settle first so we don't end up with divergent chain formatting implementations that are each massive.

Will take another pass after some of those other items are resolved, but TBH this is still a ways out from being included in a released version of rustfmt.

superhawk610 · 2020-08-05T18:21:29Z

Makes sense. Thanks for working on this! 🙂

calebcartwright commented Oct 25, 2019

View reviewed changes

Configurations.md Outdated Show resolved Hide resolved

calebcartwright commented Oct 25, 2019

View reviewed changes

src/chains.rs Outdated Show resolved Hide resolved

calebcartwright commented Oct 25, 2019

View reviewed changes

src/chains.rs Outdated Show resolved Hide resolved

calebcartwright commented Oct 25, 2019

View reviewed changes

topecongiro reviewed Oct 25, 2019

View reviewed changes

Configurations.md Outdated Show resolved Hide resolved

src/chains.rs Outdated Show resolved Hide resolved

calebcartwright force-pushed the long-chains branch from d8506f6 to c115af7 Compare October 29, 2019 17:34

calebcartwright changed the title ~~support wrapping call chains with long elements~~ [WIP] support wrapping call chains with long elements Oct 29, 2019

calebcartwright marked this pull request as ready for review October 29, 2019 17:36

calebcartwright force-pushed the long-chains branch from afaca31 to 159756d Compare October 29, 2019 18:02

calebcartwright changed the title ~~[WIP] support wrapping call chains with long elements~~ [WIP] update chain handling of long lines and block-like parents Oct 31, 2019

calebcartwright added 5 commits October 31, 2019 15:35

feat: support wrapping call chains with long elements

f0c94d0

fix: indenting chain elements with long parent

40d1544

feat: support configurable chain indent with block-like parent

617e22a

fix: chain wrapping with long child item

25ac66b

tests: add more tests for long chain elements

79452d5

calebcartwright force-pushed the long-chains branch from 8ec76e5 to 79452d5 Compare October 31, 2019 20:36

tests: more visual indent style tests

0a8b110

calebcartwright force-pushed the long-chains branch from 6c8a10c to 0a8b110 Compare November 7, 2019 17:28

refactor: support tuple lit chain parent indent

065dbaf

calebcartwright force-pushed the long-chains branch from d58732c to 065dbaf Compare November 9, 2019 17:11

calebcartwright added 2 commits November 9, 2019 11:33

tests: add more tests for long chain elements

c850ac7

refactor: more control over chain item indents

e9acbb5

calebcartwright force-pushed the long-chains branch from b65c769 to 62846f4 Compare November 12, 2019 01:27

refactor: add chain child indent options

d4cae1f

calebcartwright force-pushed the long-chains branch from 62846f4 to d4cae1f Compare November 12, 2019 03:55

calebcartwright added 3 commits November 12, 2019 19:43

tests: more tests for block-like parents

8089198

refactor: bit of cleanup on chain formatting

70f6d1e

tests: add more chains tests

5a84221

calebcartwright changed the title ~~[WIP] update chain handling of long lines and block-like parents~~ update chain handling of long lines and block-like parents Nov 20, 2019

calebcartwright commented Nov 20, 2019

View reviewed changes

calebcartwright closed this Jan 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update chain handling of long lines and block-like parents #3884

update chain handling of long lines and block-like parents #3884

calebcartwright commented Oct 25, 2019 •

edited

Loading

calebcartwright Oct 25, 2019 •

edited

Loading

calebcartwright commented Nov 20, 2019

calebcartwright Nov 20, 2019

calebcartwright Nov 20, 2019

calebcartwright Nov 20, 2019

calebcartwright Nov 20, 2019

calebcartwright Nov 20, 2019

calebcartwright commented Jan 6, 2020

superhawk610 commented Aug 5, 2020

calebcartwright commented Aug 5, 2020

superhawk610 commented Aug 5, 2020

		@@ -0,0 +1,167 @@
		// rustfmt-indent_style: Visual

		fn long_parent() {

update chain handling of long lines and block-like parents #3884

update chain handling of long lines and block-like parents #3884

Conversation

calebcartwright commented Oct 25, 2019 • edited Loading

Issue 3863 - handling of long chain elements

Issue 3157 - configurability of chain indents for chains with block-like parents

calebcartwright Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

calebcartwright commented Nov 20, 2019

calebcartwright Nov 20, 2019

Choose a reason for hiding this comment

calebcartwright Nov 20, 2019

Choose a reason for hiding this comment

calebcartwright Nov 20, 2019

Choose a reason for hiding this comment

calebcartwright Nov 20, 2019

Choose a reason for hiding this comment

calebcartwright Nov 20, 2019

Choose a reason for hiding this comment

calebcartwright commented Jan 6, 2020

superhawk610 commented Aug 5, 2020

calebcartwright commented Aug 5, 2020

superhawk610 commented Aug 5, 2020

calebcartwright commented Oct 25, 2019 •

edited

Loading

calebcartwright Oct 25, 2019 •

edited

Loading