-
Notifications
You must be signed in to change notification settings - Fork 56
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
[RFC] Merge imports by default #140
Comments
There are some typos in your last example, but to the extent I understand it, I would just like to comment (2¢): It's not entirely clear to me that transforming to the
Some of the last might already be of applicable concern, without this RFC addition. |
Thanks, hopefully fixed.
I think these combinations need to be handled without grouping imports as well, but I am interested in examples that cause problems with import grouping, and not without. One of those is this one, where : 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. IIUC, what's imported depends only on the last component of a path, so for |
Last example still looks odd to me. It went from Sorry, I'm a bit of a details guy. |
The original example also had this bug, should be fixed now. |
Let's just make a special case for importing a module: use alloc::vec; // imports *both* the macro and the module
use alloc::vec::Vec;
use alloc::{vec, vec::Vec}; I guess all the logic is already there because it's replacing |
This might be a bit out of scope, but recent work I'm doing with futures 0.3 + 0.1 offers some complex examples of merged import blocks. Here I manually format with a single outer #[cfg(feature = "futures03")] use {
std::future::Future as Future03,
futures03::{
compat::Future01CompatExt,
future::{Either as Either03, FutureExt as _, TryFutureExt},
}
}; #[cfg(feature = "futures03")] use {
std::pin::Pin,
std::task::{Context, Poll as Poll03},
futures03::stream::Stream as Stream03,
}; Note in the latter, I manually choice not to #[cfg(feature = "futures03")] use {
std::{pin::Pin, task::{Context, Poll as Poll03}},
futures03::stream::Stream as Stream03,
}; I think this relates to rust-lang/rustfmt#2982, but shouldn't it first be addressed in more detail in the style guide? |
I'm not sure how common this practice currently is, but I noticed the following statement by Alex Crichton in an older thread about import order (#24)
Applied to your example, this rule would produce: use rocket::{Request, Response};
use rocket::request::{self, Form};
use rocket::response::{Flash, Redirect};
use rocket_contrib::JsonValue;
use strum::EnumMessage; It's not a style I've personally used – though now that I see it mentioned, I can see certain advantages. And, as I said, I'm not sure how widespread it is. But I thought it worth mentioning because, if it is widespread, it would conflict with this RFC. |
On April 11, 2020 5:49:25 AM PDT, Daniel Sockwell ***@***.***> wrote:
I'm not sure how common this practice currently is, but I noticed the
following statement by Alex Crichton in an older thread about import
order (rust-lang/style-team#24)>
>
> Never import multiple modules one one line (e.g. use std::{f32,
f64}), instead always split them on multiple lines.>
>
Applied to your example, this rule would produce:>
>
```rust>
use rocket::{Request, Response};>
use rocket::request::{self, Form};>
use rocket::response::{Flash, Redirect};>
use rocket_contrib::JsonValue;>
>
use strum::EnumMessage;>
```>
>
It's not a style I've personally used – though now that I see it
mentioned, I can see certain advantages. And, as I said, I'm not sure
how widespread it is. But I thought it worth mentioning because, if it
is widespread, it would conflict with this RFC.
This is my preferred style as well: one `use` per module, braces for using multiple names from that module.
|
The The style @codesections talks about, from a citation from Alex Crichton is there named the Even if there are four ways of merging (plus
|
My personal opinion is that the
|
I like module style imports, one but one question that isn't answered naively by them is how to handle over-long lines? I recall it being discussed, and I think some of the options were: No-Overflow:
Overflow-wrapped:
Overflow-vertical:
Overflow-module-prefix:
Overflow-module-prefix-semantic (prefer to break where different types of imports begin, so break between lowercase and titlecase names, instead of at end of line):
Personally I like any of the overflow-module-prefix styles since they make grep more likely to be useful. Aesthetically I prefer overflow-module-prefix-semantic, but I recognize that the implementation complexity for that might not be worth it. |
Putting in a vote for the 'item' style here (aka having each 'use' on a separate line). It seems to be the only one of the suggested options that actually has reasons going for it that is not based solely on differing opinions on aesthetics. Those reasons are (as @faern mentioned) fewer merge conflicts and (in addition) more readable git diffs during code reviews. You immediately see which 'use' statements got added and which deleted, whereas in the 'module' style you have to parse all things on the two diff lines to understand what is going on. -use alib::afeature::{TypeA, TypeC, TypeD};
+use alib::afeature::{TypeA, TypeB, TypeC}; vs use alib::afeature::TypeA;
+use alib::afeature::TypeB;
use alib::afeature::TypeC;
-use alib::afeature::TypeD; For an example of how it avoids merge conflicts, see here. I cannot really follow the reasoning why the 'module' style is supposed to be better for small to mid-size Rust projects. I do agree that merge conflicts and code reviews become more important in larger projects, but using that as an argument that therefore the 'item' style is less useful in smaller projects than the 'module' style feels like specious reasoning to me (logically reversing the above statement only means that the 'item' style is less useful in small projects than in large projects, but that does not imply that it is less useful than the 'module' style). |
The git diffs are more readable. I agree with that. But the code itself is way less readable under the
|
I totally believe you that you think the module style is more readable. I just think the reason why is very much due to personal preference. The next person could now easily stand here and put in an emotional speech for why they think the item style is much more readable. Personally I think what someone's personal preference is mostly depends on what they are used to, because in that case it is objectively "easier" to read for them, because their brain is now quicker at parsing the style they are used to. If one were intent on deciding based on personal preference than the best thing to do would probably to have a poll to ask as many Rust programmers as possible about their preferences and let the people decide. I am a little uncomfortable with the thought of shoving a style on all Rust programmers which justification is: "The 3 people discussing the GitHub issue at the time really liked that style". Thus if you disregard aesthetics I think the "item" style is the one that is easiest to justify when someone asks later on. All that being said => I would also be completely fine with the person who ends up implementing the different styles getting to decide which one they prefer. After all they are the ones doing the real work here. 😉 I would in any case be a pretty happy cookie if |
Letting a single person who happens to have a bit more free time than the others decide the format for everyone, but not letting three people decide because they are too few seem a bit strange to me... When it comes to style, very few things are objectively better than others. I think you will find that very much of the style guide and rustfmt defaults got to where they are by people expressing subjective opinions on issue trackers ;) |
(let me ignore your jab about my free time 🙈) in the absence of better arguments and better statistics the "3 vs 1" should definitely win, I completely agree. :) Just in this case, it felt to me like there is an opportunity to make a decision based on a little bit more sound footing than a popularity contest, which is all I wanted to point out. It seems to me that you don't agree, which is fine. 😅 Good chance that I am misguided in my desire for more objective reasoning and good chance that you are right in your belief that the subjective preferences of people reading code is more important than the (rather?) objective benefits you could get when reading code diffs. In the end I think you are definitely right, most aspects of style are subjective and that is exactly why |
My 2c: One |
In my opinion the |
https://github.com/stratis-storage/stratisd/blob/develop-2.1.0/src/engine/strat_engine/engine.rs is how we do it. One "use" for the standard library, one each for every external crate, and one for our own crate. This is pretty much the same thing we enforce with Python's isort and black libraries in our Python code. I let rustfmt and rustc sort out all merge conflicts and discrepancies. The cost is small compared to the high cost of other things. But, we don't enforce this by our CI; don't even run rustmt with merge_imports set to true. We ran it about a year ago on nightly, to do the merging of our existing imports which had a hodgepodge of styles, and since then we let stable rustfmt sort out our imports. But we mostly remember to conform to the style. I prefer the merged style greatly because it is more readable to me. I would very much like if this were stabilized in almost any form, at which point we would promptly make it part of our CI. Probably it's time for me to do another rustfmt-on-nightly sweep, to fix up anything that's decayed since a year ago... |
I find crate-level merging annoying when I know what module I need to import another item from and want to find the existing location of its import in source. I don't want to dig into the tree and have to think about parents in order to find my module name (which can get annoying when not every item has its own line). I'd rather scan for I believe module level merging is the best middle ground, as it's easy to visually scan and modify. I think it's reasonable to believe that this shouldn't result in serious merge conflict problems either, as
Any large enough project to be impacted enough by merge conflicts on module or crate level merging could easily switch to item style by a quick rustfmt entry + reformat commit. I really think module-level import merging is the best tradeoff and that we should choose that as our default, but I agree with several previous posters that it would be nice just to land this with any default so we can get the options into rustfmt on stable as soon as possible, as I find the readability arguments rather subjective, and the potential merge conflict problems to be mild enough to not warrant any serious concern. Can we find any other arguments? How should we proceed in order to settle this? Edit: just want to link in rust-lang/rustfmt#3362 |
I'm with @martingronlund. Some style that is mostly compatible w/ what we do now, doesn't have to be the default, would be fine w/ me. We would even be willing to absorb a little reformatting in all our Rust projects. But, in this discussion, I think the word "aesthetics" ought to be something more like "semantics". The merge style we use conveys a lot to me about what we import in a particular module, and that says a lot about what the particular module is doing. So, anything that makes it easier for useful summary information to go from my eyes to my brain makes me happy. I realize that others may not find a world of fascination in imports. I happen to and that's why, for this one rustfmt option, I was willing to engage in the awkward shenanigans described in #140 (comment). We don't have a rustfmt configuration file in any of our projects; because in every other case the defaults have been "good enough", maybe not what we would really want, but definitely not worth tweaking. This one is different, and its arrival in stable is eagerly awaited by me. |
As requested. The item style is absolutely, objectively the right way to write
100% agree. <rant> I think the feeling that aggregated statements are "easier to read" stems from an aesthetic heuristic most developers develop when writing actual code. And by actual I mean code that does stuff, not Random Dijkstra quote:
To illustrate: fn hmm(a: FooBar, b: FooBar) {
let mut buffer = vec![];
match a {
Foo => buffer.push(0),
Bar => buffer.push(1),
}
match b {
Foo => buffer.push(0),
Bar => buffer.push(1),
}
whatever(buffer);
} Duplication. Boo. OCD twitching, aesthetic heuristic kicking in. fn foo_bar_to_byte(foo_bar: FooBar) -> u8 {
match foo_bar {
Foo => 0,
Bar => 1,
}
}
fn hmm(a: FooBar, b: FooBar) {
let mut buffer = vec![];
buffer.push(foo_bar_to_byte(a));
buffer.push(foo_bar_to_byte(b));
whatever(buffer);
} Relieved smile. Coffee slurp. Why is the second code preferable to the first one? Is it because there is less duplication? No. It's because it scopes reasoning. When mapping the code text of Use/import statements are a different beast. There is no process to comprehend, no dynamic state to understand which is eased by lexical scoping. The goal of these statements is simply to bring a symbol into scope. In other words, when reasoning about use statements the mental model of the code text is a map from global identifier to local identifier. So, suffice to say, our coding style should make constructing this mental map as seamless as possible. Let's say I want to understand where symbol use a::{
b::{
c::{X,W}
Y,
},
d::Z
} with use a::b::c::X;
use a::b::c::W;
use a::b::Y;
use a::d::Z; In which case can we construct that mental map easier? Think about what we're doing when looking at the first version. Now compare with the second version. First we find </rant> I would also like to ask people who disagree: what is the benefit of aggregated imports? Can you give a concrete example where you find it easier to understand? Saying "it's easier to read" or "it's the best tradeoff" is not an argument. |
@exFalso Well, I used to think I liked the merged style, but I have to admit that you've just changed my mind :) I did not realize that DRY actually does not apply to Still, although I find it crystal clear that: use a::{
b::{
c::{X,W}
Y,
},
d::Z
} is easier read as use a::b::c::X;
use a::b::c::W;
use a::b::Y;
use a::d::Z; I would argue that use a::b::c::{X, W};
use a::b::Y;
use a::d::Z; takes it one step further:
Of course, it agree that it should not break any per-use comment or use a::b::c::X;
// per-use comment for W
use a::b::c::W;
use a::b::Y;
use a::d::Z; unchanged. This sounds like a very specific situation (same prefix all way long && no broken per-use comment && no broken |
My goals when looking at import statements are almost invariably broader than "find out where a symbol is coming from". What is the value to me of finding out where a symbol comes from? That I can use that information to look up the definition of the symbol? I've already used ripgrep for that, typically. I'm usually asking questions like: should that symbol be defined in that module anyway? It doesn't really belong w/ the rest of the symbols being imported from the module. And what is that submodule doing w/in that other module? Shouldn't it be a sibling rather than a child? And so forth. The merge style is much more helpful when trying to arrive at those sorts of judgements. As I said above, I'm not asking that the merge style be made the default, just that some merge style be accessible in stable. I'm not sure why it's even necessary to have so many discussions about the merits of the respective styles; configuration files should be adequate to allow different users to select the styles that they prefer. |
That's a subjective opinion, and it is wrong because it is presented as an objective fact rather than personal preference. I prefer one import per line. When imports are sorted, there are no issues at all to answer any question you stated, and in addition to that
The default is quite important. Because I believe most projects won't bother doing deep configuration of rustfmt, so if working on some project (opensource, or contributing to another project in the company), that style had to be used, regardless of personal preferences. |
Easy does it there, nobody who has been commenting here has been scrupulous about being careful to distinguish subjective experience from objective fact. You haven't been so careful yourself; do you really maintain that "cfg-guarded imports are easier to manage" has the same objective quality as "2 + 2 = 4"? I find that cfg-guarded imports are harder to manage with your preferred arrangement; clearly there is an element of subjectivity in your statements, yet you have stated them all as if they were objective facts. We all discuss things in this way, making statements that are somewhat subjective as if they were purely objective facts, in order to prevent every discussion from becoming ten to one-hundred times longer than it is already; not because we firmly believe in the pure objectivity of everything we say. Try rewriting your comment to explicitly acknowledge the subjectivity of every item in your list that could be considered subjective, and you'll find that it's a bit of a chore to write and to read.
|
Well, it depends. I had a scenario in mind (from various my projects) where they are easier to manage when split, e. g.
But I agree, I was a bit sloppy presenting my arguments, thank you for pointing it out. |
Interesting, I tend to only grep completely unknown codebases. For dev I use Ctrl-click to jump around definitions in an IDE. Even when I used to use emacs I just used etags. Tbh personally I use the item style throughout the Rust code I work on, so I seldom have to look at
1 and 2 are examples of "What is the value to me of finding out where a symbol comes from?".
Hmm well those are subjective questions. Personally I group symbols based on crate-level feature switches, beyond that it's mostly about helping code discoverability. I'm not sure how
I think everyone agrees that the different styles should be configurable. But, as the RFC title suggests as well, the question is around what style - if any - to make the default. In the hopes of bringing the discussion forward, perhaps we can agree on a principled way to decide:
|
@exFalso: I'll answer your last points in inverse order. [3]. I don't think there can be a clear winner; maybe no default is a good idea, although I bet it has never been done before w/ rustfmt. I would support that if it was a way to go forward. So, is "no default" a viable choice? |
At the danger of repeating myself. I do think that the "fewer merge conflicts"-thing is an advantage of the item-style that one can count as truly objective.. (The item-style imports will always lead to a smaller or equal number of merge conflicts compared to the number of merge conflicts if module-style imports are used.) Of course, one could make a rhetorical argument about merge conflicts being a consequence of an individual's personal choice (or contribution habit) to work with other people, but that feels a little contrived to me. Unfortunately asking how the objective advantage of fewer merge conflicts should be weighted compared to the other more subjective advantages or disadvantages of the different import styles leads down another rabbit hole of subjectivity. Seeing that there maybe really cannot be a clear winner on these other points (and seeing that we are probably all a little guilty of letting the elephant guide our rider), maybe the truly objective advantage of fewer merge conflicts should tip the scales in favor of the item-style? That being said, I also full-heartedly agree with the below statement.
At this point, I personally care more about the different styles being made available than about which one is the default. 🤗 Meta-comment |
Interesting article and very apt metaphore. Perhaps at one point in our lives we were rational actors, making this very decision consciously, but now we just keep trying to replicate the same sequence of reasoning in other people. We all know we're right.
If less merge conflicts and smaller code diffs don't count as objective advantages then there really is no hope to resolving the discussion rationally.
I'm hoping for that knight. As with software in general, the number of brains involved in a decision is inversely proportional to the coherence of the result. Perhaps we're not actually trying to convince one another, we're just trying to convince that wise and powerful leader reading through this in the hopefully-not-so-distant future.
That's pretty much what we have now. Personally I'm ok with this. What I'm not ok with is making the merge style the default. That's clearly the wrong choice 😄 |
How can diff size be the hill that so many are willing to die on? Have merge conflicts really been so evil to you in the past? Has all other rustfmt defaults been based on diff size or why is it so important here? If not, it feels like a drop in the ocean. It may be a somewhat objective advantage, but it does not feel so important that it makes all other alternatives reserved for crazy people and worth discussing this for almost two years.
Not it is not! We currently have no good way to let rustfmt deal with our imports for us. We still need stable rustfmt to be able to split out joined use statements into item style and vice versa. I don't care about the default any more. Because this has turned into a full on flame war. When people (even jokingly) tell others their preferences are wrong, there is no longer any hope for any resolution. |
Let's just set the default to |
Yes. It's unnecessary work which could be easily avoided. That work is O(n) in number of PRs and also in number of devs. But I realize the code churn's rate and shape differs per project, which could be why people are dismissing these considerations. Thankfully we seem to be converging to no default + configurable style, which hints at a "Close" resolution of this RFC. If it's brought up again then we can point people to this discussion and the relevant configuration options. |
Thanks everyone for weighing in and sharing your perspectives. My main takeaways at this time are:
For now it seems the best tact will be for rustfmt to continue to preserve the programmers formatting choice for imports (default to no action/no merging). I think the next step will be adding support for these additional strategies within rustfmt. Perhaps after some of these merge strategies have been available for a while the community may be able to land on a default merging approach before stabilization. Finally, while I think it's best to keep discussions about the stability of rustfmt configuration options in their respective threads in https://github.com/rust-lang/rustfmt I will add here that although I understand the desire from folks to have this option available on stable, it's still a ways out. The existing, comparatively simple, option has several blocking bugs and issues, and the discussed changes are only going to increase complexity. |
Would encourage folks to review and provide feedback on rust-lang/rustfmt#3362 (comment) in the rustfmt repo |
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
Merging imports by default makes imports clearer, e.g.,
is visually easier to parse than
since the
{ }
in grouped imports make scoping clearer, as opposed to having to parse whether there are differences in lines, like inwhere there is a common prefix.
Tools can optionally offer an option to disable merging imports by default.
This does not change the rules with respect to grouping imports separated by empty lines (this is still not done), that is, this:
continues to be merged as
Prior art
rustfmt
has amerge_imports
option that does this. There are some bugs open for this feature that would need to be closed before stabilization.Prior discussions
This comment in #24 proposed not merging imports by default, without rationale for that decision.
The text was updated successfully, but these errors were encountered: