Implement `One` option for imports_granularity #4680

magurotuna · 2021-01-31T08:37:29Z

This implements a new option One for imports_granularity, which merges all imports into a single use statement as long as they have the same visibility.

Resolves #4669

calebcartwright · 2021-02-09T02:54:49Z

Thank you for the PR @magurotuna! As you may have noticed the PR queue is a bit backed up at the moment as I've been preoccupied with some maintenance work and trying to get rustfmt back and available on nightly again. Should have bandwidth to get through some of these PRs in the near future though!

magurotuna · 2021-02-09T05:09:04Z

@calebcartwright Alright, no rush :)

msmorgan · 2021-02-12T17:33:55Z

src/formatting/imports.rs

@@ -560,6 +574,7 @@ impl UseTree {
                SharedPrefix::Module => {
                    self.path[..self.path.len() - 1] == other.path[..other.path.len() - 1]
                }
+                SharedPrefix::One => unreachable!(),


Did you try making this branch return true? It seems like that could mean you don't need temp_merge_by.

Yes, I tried that, but I found that ended up requiring additional special logic to address SharedPrefix::One cases.
For example, consider the following input:

use b; use a::ac::{aca, acb}; use a::{aa::*, ab};

If we changed SharedPrefix::One => unreachable!(), to SharedPrefix::One => true, with the rest code reverted to master, the above code would be formatted into:

use { a::{ ab, ac::acb, {aa::*, ac::aca}, }, b, };

Here, ac::acb and ac::aca should be put together, but actually not.
After bumping into this issue, I thought it would be a good and simple solution to leverage the same logic as the SharedPrefix::Crate and then to merge the result.

Thanks @msmorgan for reviewing and @magurotuna for explaining the test case. I'm not sure about the implementation to deal with those types of cases though. I suspect the unreachable arm and temp/actual vars will be puzzling to both casual and future browsers of the code.

I will take a closer look at the proposed changes later on, but I think it will be preferable if we can come up with an approach that avoids those

Okay, I will consider more as well.

@calebcartwright
I attempted to fix the code to avoid unreachable! arm. I think this implementation is kind of naive but seems to work fine. To ensure it works, I added more test cases.
Could you take a look again?

I think this implementation is kind of naive but seems to work fine

As in, you think your original approach was better than this, or you think there's a better approach than this updated implementation?

To ensure it works, I added more test cases

Brilliant thank you! If you don't already have these, I'd advising having a few cases that exercise some self and comment scenarios

Could you take a look again?

Yes indeed. I've not had a ton of bandwidth to allocate to PR reviews lately though so may be a while yet.

Also, don't necessarily feel like you have to shoehorn the One strategy into the existing flow that's leveraged for other variants, especially if doing so is more cumbersome than helpful.

As in, you think your original approach was better than this, or you think there's a better approach than this updated implementation?

Actually, on second thought, now I think the current approach is reasonably good. Although there are a couple of ad-hoc if-expressions, which seemed a little bit ugly to me, it apparently works just fine. Implementing imports_granularity essentially involves complexity so I feel like working fine is more important than anything else.

If you don't already have these, I'd advising having a few cases that exercise some self and comment scenarios

Those definitely should be added! I've added more tests that cover them.

calebcartwright · 2021-03-20T02:30:31Z

Thanks again for the updates @magurotuna! I've done a quick pass through the updated implementation and agree that this could work.

Would you do me a favor though and rebase on the latest from master? There's been a lot of moving changes around this part of the codebase, as well as some forced updates of the rustc-ap crates that I'd like to have pulled into this PR before doing a final review and merge

magurotuna · 2021-03-20T06:37:06Z

@calebcartwright
Sure! I've done with rebasing.

calebcartwright

Thanks again! One minor request left inline below, but otherwise should be good to go for the initial rollout.

Not strictly related to this PR but every time I review changes to the imports handling code I am reminded of how great it would be to one day be able to rid of all these clones 😞

calebcartwright · 2021-03-21T18:41:34Z

src/formatting/imports.rs

+            let similarity = tree
+                .path
+                .iter()
+                .zip(&use_tree.path)
+                .take_while(|(a, b)| a.equal_except_alias(b))
+                .count();


I think we should be able to skip the derivation of this value when merge_by is not the SharedPrefix::One variant, correct? If so, then would be good to variant gate this and default to 0 for the others. However, if not then let's add some test cases with the Module and/or Crate variants that demonstrates the necessity.

I think that's right! Only SharedPrefix::One requires similarity. I fixed the code so computing similarity will be done in the case of SharedPrefix::One only.

I am reminded of how great it would be to one day be able to rid of all these clones 😞

True... :-|

This option merges all imports into a single `use` statement as long as they have the same visibility.

calebcartwright · 2021-03-25T22:03:09Z

Brilliant thank you!

Boscop · 2021-05-01T08:11:51Z

I was looking for a config option that merges all imports.
The One option is not mentioned in the doc:
https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#imports_granularity
But it's already fully implemented & merged, right?

Boscop · 2021-05-01T08:23:52Z

Hm no, it's not working:

Error: Decoding config file failed:
unknown variant `One`, expected one of `Preserve`, `Crate`, `Module`, `Item` for key `imports_granularity`
Please check your config file.

When will it be available? :)

I have

> cargo +nightly fmt --version
rustfmt 1.4.37-nightly (0bd2b192 2021-04-03)

vorot93 · 2021-06-25T05:24:07Z

@Boscop looks like it was merged into 2.0 experimental branch, not into current master 😕

karyon · 2021-10-25T10:28:17Z

Backport was done in #4973

calebcartwright · 2021-10-26T02:11:08Z

Believe this is avail as of nightly-2021-10-24 if not the day before

calebcartwright added the pr-not-reviewed label Jan 31, 2021

msmorgan reviewed Feb 12, 2021

View reviewed changes

calebcartwright removed the pr-not-reviewed label Feb 27, 2021

magurotuna force-pushed the one-variant-for-imports-granularity branch from e1dd5fa to c165107 Compare March 2, 2021 16:17

calebcartwright added the pr-follow-up-review-pending label Mar 3, 2021

calebcartwright added pr-waiting-on-author and removed pr-follow-up-review-pending labels Mar 20, 2021

magurotuna force-pushed the one-variant-for-imports-granularity branch 2 times, most recently from 3b92ef9 to 04c5a99 Compare March 20, 2021 06:31

calebcartwright removed the pr-waiting-on-author label Mar 21, 2021

calebcartwright reviewed Mar 21, 2021

View reviewed changes

calebcartwright added the pr-waiting-on-author label Mar 21, 2021

magurotuna force-pushed the one-variant-for-imports-granularity branch 2 times, most recently from 967cff2 to 22c5926 Compare March 24, 2021 15:31

Implement One option for imports_granularity (rust-lang#4669)

22c5926

This option merges all imports into a single `use` statement as long as they have the same visibility.

calebcartwright merged commit 509ac2e into rust-lang:master Mar 25, 2021

calebcartwright removed the pr-waiting-on-author label Mar 25, 2021

magurotuna deleted the one-variant-for-imports-granularity branch March 25, 2021 23:15

irh mentioned this pull request Aug 31, 2021

Implement One option for imports_granularity (#4669) (Repeat PR) #4973

Merged

karyon added the 1x-backport:completed label Oct 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `One` option for imports_granularity #4680

Implement `One` option for imports_granularity #4680

magurotuna commented Jan 31, 2021

calebcartwright commented Feb 9, 2021

magurotuna commented Feb 9, 2021

msmorgan Feb 12, 2021

magurotuna Feb 14, 2021

calebcartwright Feb 25, 2021

magurotuna Feb 25, 2021

magurotuna Mar 2, 2021

calebcartwright Mar 12, 2021

magurotuna Mar 13, 2021

calebcartwright commented Mar 20, 2021

magurotuna commented Mar 20, 2021

calebcartwright left a comment

calebcartwright Mar 21, 2021

magurotuna Mar 24, 2021

calebcartwright commented Mar 25, 2021

Boscop commented May 1, 2021

Boscop commented May 1, 2021 •

edited

Loading

vorot93 commented Jun 25, 2021 •

edited

Loading

karyon commented Oct 25, 2021

calebcartwright commented Oct 26, 2021

Implement One option for imports_granularity #4680

Implement One option for imports_granularity #4680

Conversation

magurotuna commented Jan 31, 2021

calebcartwright commented Feb 9, 2021

magurotuna commented Feb 9, 2021

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

calebcartwright commented Mar 20, 2021

magurotuna commented Mar 20, 2021

calebcartwright left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright commented Mar 25, 2021

Boscop commented May 1, 2021

Boscop commented May 1, 2021 • edited Loading

vorot93 commented Jun 25, 2021 • edited Loading

karyon commented Oct 25, 2021

calebcartwright commented Oct 26, 2021

Implement `One` option for imports_granularity #4680

Implement `One` option for imports_granularity #4680

Boscop commented May 1, 2021 •

edited

Loading

vorot93 commented Jun 25, 2021 •

edited

Loading