Skip to content
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.

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

[unstable option] merge_imports #3362

Closed
scampi opened this issue Feb 13, 2019 · 61 comments
Closed

[unstable option] merge_imports #3362

scampi opened this issue Feb 13, 2019 · 61 comments
Labels
unstable option tracking issue of an unstable option

Comments

@scampi
Copy link
Contributor

scampi commented Feb 13, 2019

Tracking issue for unstable option: merge_imports

@scampi scampi added the unstable option tracking issue of an unstable option label Feb 13, 2019
@scampi scampi changed the title [unstable option] unstable option: merge_imports [unstable option] merge_imports Feb 18, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 19, 2019

Would it be possible to update this Tracking issue with more information?

For example, why is this unstable? Should this always be opt-in, or is the plan to enable it by default, what remains to be done to make this option stable, etc.

Which PR implemented this (e.g. #2603) ?

There is a closed fmt-rfc: rust-lang/style-team#24 where @nrc said:

I propose that we not do this by default, but tools may offer options to do this.

But that's about it, the merged RFC does not talk about it, so I'd guess there is no merged RFC saying anything about this. Is the next step towards stabilization to merge an RFC about this ? (independently of which direction that RFC goes).

@jrobsonchase
Copy link
Contributor

Assuming the rust compiler is behaving as intended, this can break imports. Currently, these are not semantically equivalent, but rustfmt with merge_imports will format the former into the later:

use alloc::vec; // imports *both* the macro and the module
use alloc::vec::Vec;
use alloc::vec::{self, Vec}; // only imports the module and `Vec`. `vec!` is left out.

@valff
Copy link
Contributor

valff commented Aug 24, 2019

Perhaps merge_imports shouldn't use self in a leaf position at all? So this

use alloc::vec::{self, Vec};

should always be converted to

use alloc::{vec, vec::Vec};

@topecongiro topecongiro removed this from the 1.5.0 milestone Aug 29, 2019
@stepancheg
Copy link
Contributor

I wish there was a third option: unmerge imports into one import per line.

@faern
Copy link

faern commented Sep 6, 2019

I would love merge_imports to be stabilized. I use it in all my personal and work crates. But if splitting imports is also desired, it should probably be under the same config directive, so probably good to think that through first.

@nico-abram
Copy link

With #3753 merged, is the only blocker renaming the option so it is a list of 3 values (Never merge, always merge, leave imports untouched)?

@faern
Copy link

faern commented Oct 4, 2019

With "never merge" I suppose you mean something that splits imports, or unmerge them?

I suppose we want one value that turns

// this
use std::{ptr, mem};
// into
use std::ptr;
use std::mem;

And one value that does the opposite. And of course the third one which disables this feature altogether, not touching use statements.

@nrc
Copy link
Member

nrc commented Nov 25, 2019

I would like to see merge_imports, but it seems like there is some work to do before it is bug-free enough to do so.

One observation is that it is a little inconsistent about when it uses one line vs multiple lines (though I guess this is a problem without the option, but you notice it more with the option). Using merge_imports does give an opportunity to change behaviour in a backwards-compatible way.

E.g.,

// Formatted on one line.
use futures::{compat::Compat01As03, future::BoxFuture, prelude::*};
// Formatted on multiple lines.
use std::{
    fmt::{self, Display},
    result,
};

IMO both of those should be multi-line.

@Lokathor
Copy link

Shouldn't it just be controlled by the max line width and small_heuristics like code expressions are?

@jannickj
Copy link

What is missing for this to be stabilized?

@CBenoit
Copy link

CBenoit commented Jun 1, 2020

Hi! I'm asking the question again. What is missing exactly? I'm willing to help and make a PR if necessary. Should I try to change the behavior pointed by @nrc ?

@calebcartwright
Copy link
Member

The process for stabilizing a configuration option is detailed here. That documentation also enumerates the conditions/requirements to stabilize:

* Is the default value correct ?
* The design and implementation of the option are sound and clean.
* The option is well tested, both in unit tests and, optimally, in real usage.
* There is no open bug about the option that prevents its use.

As it relates to merge_imports, I believe at least a couple of those conditions have not yet been met.

There are several open issues related to the option in addition to some of the items discussed in this thread. There are also outstanding questions regarding the default value, or even if this option should be an enum like some other config options instead of the current boolean.

@Robbepop
Copy link

Robbepop commented Jun 5, 2020

According to the stability of the implementation:
We are using this formatting option in our codebase with approx 30k lines of code and it is even integrated into our CI - so commits that do not adhere to our formatting are automatically rejected.
This works since many months now and I have never had a single problem with this formatting scheme.

Also I think it would be nice if the default was true instead of false since it is to my opinion one of the most useful formatting schemes by the amount of formatting work it takes from my shoulders.
However, I'd argue that for a start we should have false as default value to avoid changing the formatting of many crates out there.

Maybe an edition change could be used to change the default formatting rules?

@CBenoit
Copy link

CBenoit commented Jun 5, 2020

Thank you @calebcartwright! I had a look. I already started to check code.

@Robbepop
I agree that it's quite stable. We're using this option in my company as well (in a "soft" way, we format with it, but CI check is using stable channel). However looks like edgecases are remaining, with comments and already invalid Rust code (?) (not sure about what should be done with that one as it sounds fair that invalid code is formatted into invalid code).
Also, I see there is an RFC to enable merge imports by default: rust-lang/style-team#140 .
I believe the matter of the default should be discussed there.

I saw several comments in this thread about an alternative "shallow merge behavior": never import multiple modules on one line. Essentially only struct, enum, etc are merged (e.g. use std::ffi::{CStr, CString} but never use std::{convert::TryFrom, ffi::{CStr, CString}}). That's what this issue is about I believe.

Regarding the option. I believe enum-like would be a good idea.

  • we can add new values without breaking existing configuration
  • we already have multiple good values: Always, Never, Shallow, LeaveUntouched (default: LeaveUntouched)

Shallow being the aforementioned behavior.

Big question. What about imports_layout? Looks like this option is similar:

imports_layout [Vertical|Horizontal|HorizontalVertical|LimitedHorizontalVertical|Mixed] Default: Mixed

@exFalso
Copy link

exFalso commented Jun 9, 2020

Is there any way to do the automatic unmerging with the current rustfmt version?

<rant>
Personally I have no idea why Rust even has this import merging "feature". It's just a source of merge conflicts, makes copy-pasting code more difficult, and makes understanding of the import lines more time consuming... why is this a thing? What is the benefit? Reminds me of the Makefile syntax's itch to deduplicate every single symbol, at the heavy cost of readability and maintainability.

use futures::{compat::Compat01As03, future::BoxFuture, prelude::*};
use std::{
    fmt::{self, Display},
    result,
};

I mean what am I even looking at? Import lines should be the most straightforward piece of code. Compare the above with

use futures::compat::Compat01As03;
use futures::future::BoxFuture;
use futures::prelude::*;
use std::fmt::self;
use std::fmt::Display;
use std::result;

Wow. Boring, I know. But suddenly, if there are 2 commits adding a futures import, they won't conflict? And the imports are actually readable?

Surely I am missing something...
</rant>

@vcfxb
Copy link

vcfxb commented Jun 11, 2020

I'd really like to see this stabilized (preferably with options "Never" and "Shallow").

@lunacookies
Copy link

I think another option that I’ve seen in some codebases is the use of only one use statement:

use {
    futures::{compat::Compat01As03, future::BoxFuture, prelude::*},
    std::{
        fmt::{self, Display},
        result,
    },
};

rather than

use futures::{compat::Compat01As03, future::BoxFuture, prelude::*};
use std::{
    fmt::{self, Display},
    result,
};

@exFalso
Copy link

exFalso commented Dec 30, 2020

I only realized the question was about naming after a second reading, I think it would be helpful to put more emphasis on it in the original question.

Also, alternate naming suggestion for One: Single

@calebcartwright

This comment has been minimized.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Dec 30, 2020

Config option name: imports_style

I would like to propose imports_blocks (or imports_blocks_style if we want to retain the "use style postfix for enum-based options" convention).

We have several options for changing the formatting of imports in some way:

  • imports_layout
  • group_imports
  • reorder_imports
  • imports_ident

I think imports_style by itself does not convey enough how it is different from the others.

Why imports_**blocks**_style?

I found the following terminology being used within existing rustfmt options:

  1. An "import group" is a set of imports separated by a newline. See the recently added group_imports option.
  2. imports_layout calls all imports within a single use an "import block".

(2) suggests that our new option controls how imports are organized into blocks:

  • one: only have one import block
  • module: create one import block per module
  • crate: create one import block per crate
  • item: create one import block per item

This looks fairly consistent with one exception: I believe syntactially, a block is identified by curly braces { }. With the item variant, there would not be any curly braces and hence the terminology of block is slightly off.

@calebcartwright
Copy link
Member

I think imports_style by itself does not convey enough how it is different from the others.
I would like to propose imports_blocks (or imports_blocks_style

I understand that thinking, and have gone through a few iterations of it myself. However, I think the usage of block within the name is a bad idea because there's far too many other connotations within the rustfmt context that aren't applicable here.

Though the imports_style name doesn't necessarily convey how it differs from other import options, imports_block_* would almost certainly be a source of confusion due to overlap, both with import specific options and more broadly.

I think an additional modifier in the config option name could be helpful, but I'm opposed to block being that modifier personally

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Dec 30, 2020

I think an additional modifier in the config option name could be helpful, but I'm opposed to block being that modifier personally

I agree with that, block is just the best thing I could come up with. I am completely open to use a different modifier.

Edit: Based on the number of upvotes, people also seem to be happy with the imports_style name. I am also happy with that and actually super excited about this issue making progress!

@dtolnay
Copy link
Member

dtolnay commented Dec 30, 2020

imports_granularity would accurately describe that this option determines what goes in one import statement vs another.

imports_layout = "Horizontal"
imports_granularity = "Module"

@calebcartwright
Copy link
Member

calebcartwright commented Dec 31, 2020

imports_granularity would accurately describe that this option determines what goes in one import statement vs another.

Agreed. I like this one a lot more.

Given the work in flight, we'll first need to finalize the config option name and the name of the variants that map to the current true/false values for merge_imports, but can bikeshed on the other variant names if needed for a while. I've updated the items that have proposed alternatives and would encourage folks to only vote for one/change votes as needed (for example if you'd prefer imports_granularity over imports_style then remove any positive reaction from the imports_style comment)

@piegamesde
Copy link

Alternative suggestions for Preserve, depending on how the option is called in the end: Disable, Manual.

@piegamesde
Copy link

I think the verb "grouping" best describes what this option does. Something like group_imports_style or something maybe?

@calebcartwright
Copy link
Member

Alternative suggestions for Preserve, depending on how the option is called in the end: Disable, Manual.

Preserve variants are used within rustfmt config options to cover scenarios where rustfmt's maintains/does not modify the original formatting, so this was chosen intentionally for consistency.

Obviously a variant names applicability is dependent upon the option itself, but with the main option names discussed so far Disable looks pretty odd to me: imports_style = Disable, imports_granularity = Disable. Manual seems reasonable on its own IMO, but would introduce some inconsistency in the broader rustfmt configuration ecosystem

@calebcartwright
Copy link
Member

calebcartwright commented Dec 31, 2020

I think the verb "grouping" best describes what this option does. Something like group_imports_style or something maybe?

Understand the thinking with this one, but it's another case where it'd be problematic in the broader context since there's also a desire (and a new, unstable config option group_imports) to support moving/regrouping use items separately from the granularity (refs #4107)

@iago-lito
Copy link

Sorry to hear that there are problems with Manual since I find it much better than Preserve or Disable. Alternate suggestion: Free?

@iago-lito
Copy link

iago-lito commented Jan 4, 2021

Alternate suggestion for One or Single (I still like the latter): Factorized.

@mwilliammyers
Copy link

mwilliammyers commented Jan 6, 2021

Further notes regarding consistency:

Preserve is used in several config option names and variants already. Disable and Manual are not used anywhere.

single is used in several config names but one is not.

@calebcartwright
Copy link
Member

single is used in several config names but one is not.

Understand the thrust of the point, but this does need a minor correction. It's true that there are no option variants that utilize Single, and there are no option names that have One, but there is an existing option that has One as a variant

https://github.com/rust-lang/rustfmt/blob/rustfmt-1.4.31/Configurations.md#version
https://rust-lang.github.io/rustfmt/?version=v1.4.30&search=

@calebcartwright
Copy link
Member

Thanks to everyone who weighed in and voted! At this point I feel we have more than sufficient consensus to move forward with the first portion of this (#4600) to make the new configuration option and a couple of the variants available for consumption.

The new config option will be named imports_granularity, and will initially include the Crate, Module, and Preserve variants.

After incorporating the new option into the codebase I'll open up new individual issues to track the implementation of the respective additional variants and their associated styles, and then at some point down the road we'll start a new thread to track the stabilization of imports_granularity

Happy to let the discussion continue here on the names of the other variants (e.g. One vs. Single) until such time as an implementation is proposed, but we're going to go ahead and proceed with the option and ones enumerated above.

@cdmistman
Copy link

Since Item isn't contested anywhere in the discussion, would it be possible to include that variant as well?

@calebcartwright
Copy link
Member

calebcartwright commented Jan 10, 2021

Since Item isn't contested anywhere in the discussion, would it be possible to include that variant as well?

It's not really about whether or not there's contestation. The actual implementation already exists for the ones I mentioned above (rustfmt is capable of doing the corresponding formatting), but there is no such implementation for the others. If anyone is interested in doing the work to actually implement support for Item that'd be great, but we obviously can't just add an empty variant.

We're proceeding with what's implemented and possible today, and the others will be added at some point down the road. We just want to go ahead and start letting folks use what's already available now, as opposed to making everyone wait until everything is done to be able to use any variant.

@calebcartwright
Copy link
Member

Thanks again everyone! I'm going to go ahead and close this issue as it's served it's purpose and is no longer necessary. To summarize:

  • merge_imports is being replaced with imports_granularity, with an automatic mapping from the former to the latter to prevent any breakage. As such, we are naturally not going to stabilize merge_imports
  • imports_granularity is already available for those building from rustfmt source, and will be in the next release of rustfmt v1.4.33 (no target date but shouldn't be too long before this lands on nightly)
  • The Crate, Module, Item, and Preserve variants are already implemented and will be immediately available alongside the introduction of imports_granularity
  • The One (or Single) variant has not yet been implemented. A new issue (imports_granularity: implement One/Single variant #4669) has been created to track the implementation.
  • There is still work to be done before this feature can be stabilized. There were already existing issues in imports formatting that are still in place, and the additional variants/styles are of course brand new and not yet tested in real projects. Please open new issues to report any bugs/problems with imports_granularity

@calebcartwright calebcartwright removed this from the 3.0.0 milestone Jan 24, 2021
facebook-github-bot pushed a commit to facebook/sapling that referenced this issue Oct 19, 2021
Summary:
To make the codebase a bit more consistent and reduce conflicts. Similar to
Java.

Note: The fbsource Rust lint does not respect this file so it cannot conflict
with the fbcode rustfmt config. That means a more preserving (for what to group
in `{}` and what not), less controversial (when to use `{}` grouping is
controversial) choice `imports_layout = "HorizontalVertical"` (like
[black](https://github.com/psf/black) in Python) cannot be used at present.

See also:

- rust-lang/rustfmt#3361
- rust-lang/rustfmt#3362
- https://fb.workplace.com/groups/rust.language/posts/6720351014680123/
- https://fb.workplace.com/groups/rust.language/posts/5429720200409884/

This adds the config without changing the files.

Reviewed By: yancouto

Differential Revision: D31746731

fbshipit-source-id: 8e171829fd53691a59bf3b80cdc500c0b3993ba5
facebook-github-bot pushed a commit to facebook/buck2 that referenced this issue Jun 21, 2022
Summary:
Pull Request resolved: #1

Seeing a lot of warnings when running `cargo fmt`,
```
Warning: the `merge_imports` option is deprecated. Use `imports_granularity="Crate"` instead
```

[GitHub issue on the deprecation](rust-lang/rustfmt#3362 (comment))

As we already have the imports_granularity="Item", it seems we can get rid of the deprecated option?

Reviewed By: ndmitchell

Differential Revision: D37313506

fbshipit-source-id: ce2da68de2dac15df5df3e6803be67baaa42cb15
github-merge-queue bot pushed a commit to NomicFoundation/slang that referenced this issue Dec 13, 2023
Part of #155

This is going to be a fun one 😆 

I've pushed two preview commits: the first one imports using the `Crate`
granularity, the other one with `Module` granularity.

Disclaimer: I'm biased 😃 I've used this style since I started
using Rust and it's also being used by vast majority of projects in the
ecosystem that I've used or looked at; it's also used by the Rust
compiler, Cargo and the tools like Rustfmt or Clippy.

Here's a summary of the possible options:
rust-lang/rustfmt#3362.

Here is an RFC discussion about setting on a variant:
rust-lang/style-team#140, I highly recommend
reading it.
To give a brief summary, it seems that people stand behind two options:
- `Module` - good trade-off between readability and conciseness
- `Item` - minimizes conflicts when many people edit the same files
often

To bring back some of the arguments raised in favor of the `Module` and
explain some of my reasoning as well:
- it naturally settles on the module as the, well, module/boundary from
which items can be imported...
- thus, helps build the intuition how to use and structure Rust crates
- less verbose than Java-style (`Item`) imports, which can often explode
and take unreasonable amount of space (and we don't really benefit
enough from minimized conflicts as we probably won't be a team of 50 or
the like)...
- but repeats enough information to quickly trace a module path rather
than having to reconstruct the paths from the `crate`-style use import
tree...
- and already often takes less space in our case, line-wise;
- quite good `grep`pability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unstable option tracking issue of an unstable option
Projects
None yet
Development

No branches or pull requests