rust-lang / style-team

Home of the Rust style team
Apache License 2.0
450 stars 56 forks source link

[RFC] Merge imports by default #140

Open gnzlbg opened 5 years ago

gnzlbg commented 5 years ago

Merging imports by default makes imports clearer, e.g.,

use rocket::{
    Request, Response,
    request::{self, Form},
    response::{Flash, Redirect}
};
use rocket_contrib::JsonValue;

use strum::EnumMessage;

is visually easier to parse than

use rocket::Request;
use rocket::Response;
use rocket::request;
use rocket::request::Form;
use rocket::response::Flash;
use rocket::response::Redirect;
use rocket_contrib::JsonValue;

use strum::EnumMessage;

since the { } in grouped imports make scoping clearer, as opposed to having to parse whether there are differences in lines, like in

use rocket::response::Redirect;
use rocket_contrib::JsonValue;

where 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:

use rocket::Request;
use rocket::Response;

use rocket::request;
use rocket::request::Form;

continues to be merged as

use rocket::{Request, Response};

use rocket::request::{self, Form};

Prior art

rustfmt has a merge_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.

dekellum commented 5 years ago

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 self syntax is going to always be an improvement. Also you might want to consider this in combination with:

Some of the last might already be of applicable concern, without this RFC addition.

gnzlbg commented 5 years ago

There are some typos in your last example

Thanks, hopefully fixed.

Some of the last might already be of applicable concern, without this RFC addition.

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 alloc::vec, both the vec! macro and the vec module are imported, but for alloc::vec::self, the components before ::self can only be modules or types, so vec is a module, and ::self only imports the module.

dekellum commented 5 years ago

hopefully fixed.

Last example still looks odd to me. It went from rocket::Form to rocket::request::From? Please also note "Form" and "From" spellings.

Sorry, I'm a bit of a details guy.

gnzlbg commented 5 years ago

The original example also had this bug, should be fixed now.

tmpolaczyk commented 5 years ago

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 vec with vec::self.

dekellum commented 5 years ago

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 use {}, just to avoid the need to repeat the #[cfg(feature=…)].

#[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 use std::{} since, its only two sub-modules, with only one import for one of those modules. I think the below is more complex to read, so not worth it:

#[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?

codesections commented 4 years ago

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-dev-tools/fmt-rfcs#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:

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.

joshtriplett commented 4 years ago

On April 11, 2020 5:49:25 AM PDT, Daniel Sockwell notifications@github.com 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-dev-tools/fmt-rfcs#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:>


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.
faern commented 3 years ago

The merge_imports (https://github.com/rust-lang/rustfmt/issues/3362) issue has seen a lot of discussion about different styles of use statements and which one should be default. In this comment (https://github.com/rust-lang/rustfmt/issues/3362#issuecomment-642564290) @dtolnay summarize four distinct import merge styles and give them some names.

The style @codesections talks about, from a citation from Alex Crichton is there named the module merge style. Some others call it the "shallow" import style, but that is a more ambigious and opaque name IMO.

Even if there are four ways of merging (plus off, disabling merging) there has to be a default value. The way I interpret the discussion in that thread, most people stand behind two of the styles:

faern commented 3 years ago

My personal opinion is that the module style merge is a great default. It's readable and still somewhat compact. It's IMO the perfect balance between all trade-offs one has to make in the different styles.

module is perfect for small to mid size Rust projects in small teams or teams who don't overlap very much while developing. Meaning probably the vast majority of Rust crates. And almost by definition how all Rust crates start out. Therefore it's a good default. The merge conflict argument for item is only valid for large projects with lots of active developers regularly touching the same modules. So large organizations can easily set the item style in their rustfmt settings and coding style guide. I don't think the defaults should reflect what's best for a large company with massive resources, rather what makes it easy to start out and satisfy the majority of crates.

quodlibetor commented 3 years ago

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:

use foo::{bar, Baz, Quux};

Overflow-wrapped:

use foo::{
    bar, Baz,
    Quux,
};

Overflow-vertical:

use foo::{
    bar,
    Baz,
    Quux,
}

Overflow-module-prefix:

use foo::{bar, Baz};
use foo::Quux;

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):

use foo::bar;
use foo::{Baz, Quux};

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.

timonbimon commented 3 years ago

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).

faern commented 3 years ago

and (in addition) more readable git diffs during code reviews

The git diffs are more readable. I agree with that. But the code itself is way less readable under the item style IMO. So one question is if people read code or if they read diffs. This is shown very well in the examples in the OP https://github.com/rust-dev-tools/fmt-rfcs/issues/140#issue-447248865. item style just verbosely repeat the exact same thing over and over again. It's very hard to see where rocket::request imports stop and where rocket::response imports start. Because every line is just a massive blob of text with 99% same content.

I cannot really follow the reasoning why the 'module' style is supposed to be better for small to mid-size Rust projects

item style is just very very long. Makes the import section of each module very long. Separates the module documentation and code by too much vertical space. Probably more so in a large project. The only advantage the item style has is the git merge property. Not counting the merge conflict property I think module style is the best style for any sized project. The only reason item might be better for larger projects is that they are usually maintained by larger teams, and as such have more overlapping changes (merge conflict potential). So yeah, saying module is better for mid-size Rust projects is probably not ideal. It would be better to say it's better for small to mid-size Rust teams.

timonbimon commented 3 years ago

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 rustfmt supported different styles for the imports irregardless of which one is the default. 🙃

faern commented 3 years ago

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 ;)

timonbimon commented 3 years ago

(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 rustfmt rules 🙌 => you can skip most annoying discussions about subjective preferences in your team and just let rustfmt decide.

jplatte commented 3 years ago

My 2c: One use per crate is most readable and I haven't had serious issues with merging in small & mid-sized projects with it. When conflicts did come up (rare) and the resolution wasn't obvious, I've had a lot of success with just adding both conflicting imports and merging everything together with nightly rustfmts merge_imports = true setting, afterwards removing any duplicate or unused imports.

rossmacarthur commented 3 years ago

In my opinion the module style is the best middle ground. I do like the readability of combined imports, but I was using merge_imports = true on a project where I am the only contributor and I have had annoying merge conflict issues a few times.

mulkieran commented 3 years ago

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...

majg0 commented 3 years 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 use ......::NAME:: to find my module.

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

  1. I don't think there should be many programmers bashing the same module concurrently.
  2. Merge conflicts would be quite easy to resolve, not much time to win or lose.
  3. Missing imports would give errors and extraneous imports would give warnings, so not much could go wrong really.

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 https://github.com/rust-lang/rustfmt/issues/3362

mulkieran commented 3 years ago

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. It's not just about what "looks" nice to me; it's about the style that conveys the most information to me.

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 https://github.com/rust-dev-tools/fmt-rfcs/issues/140#issuecomment-671131403. 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.

exFalso commented 3 years ago

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.

As requested.

The item style is absolutely, objectively the right way to write use statements. So much so that I'd even support removing the use a::{b,c}; syntax from the language altogether. Merge conflicts, smaller review diffs, cfg pragmas and per-use comments are all objective benefits that others have pointed out here and at https://github.com/rust-lang/rustfmt/issues/3362. So I'll be focusing on a perhaps more subtle difference regarding code comprehension. I'm also going to be condescending and say that I used to prefer the aggregated style (although in a different language Haskell), until a person smarter than me painstakingly showed how that preference is not grounded in any factual benefit.

When it comes to style, very few things are objectively better than others.

100% agree. use statements are one of those few things.

\<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 use statements. Utilizing lexical scoping and code deduplication is a good thing because it helps reduce mental load while reasoning about code. And the goal of good coding style is just that: reducing mental load.

Random Dijkstra quote:

(...) we should do (as wise programmers aware of our limitations) our utmost to make the correspondence between the program (spread out in text space) and the process (spread out in time) as trivial as possible.

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 hmm to a mental object, we need to use less biological working memory in the second version. We essentially enabled our code reader to comprehend the code with divide and conquer, they don't need to keep the full control flow in mind to do what they want. Deduplication was a means to an end, not the end.

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 X is coming from. Compare

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. First we visually scan the code to find X, then we start jumping to the enclosing symbols. We are flattening a path in the import tree to understand where X is coming from, literally what the second version does in the code text already.

Now compare with the second version. First we find X, which is already easier, as it can only occur at the end of a line. Aaand.. we're done, the information we're seeking is all there.

\</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.

iago-lito commented 3 years ago

@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 use statements. Now I cannot see any objective benefit of the merged style anymore, except for one niche case where you're not using autoformatting tools so it's tedious to write.

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 cfg pragma, so I would leave:

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 cfg line), but I suspect it is also very common, although I have not measured that.

mulkieran commented 3 years ago

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.

stepancheg commented 3 years ago

The merge style is much more helpful when trying to arrive at those sorts of judgements.

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

configuration files should be adequate to allow different users to select the styles that they prefer.

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.

mulkieran commented 3 years ago

The merge style is much more helpful when trying to arrive at those sorts of judgements.

That's a subjective opinion, and it is wrong because it is presented as an objective fact rather than personal preference.

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.

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

* patches are easier to read

* merge conflicts are easier to resolve

* grep works (to find all instances of `TcpSocket` from a specific crate for example)

* refactoring with `sed` is much easier (some people do that)

* imports always have the same style (no mixed curly braces/no braces; never mixed single-line/span multiple lines)

* import formatting logic is easier to replicate between tools (rustfmt is not the only tool which generates imports, editors also do that for example)

* per-import comments are possible as stated above

* cfg-guarded imports are easier to manage

* many other reasons

configuration files should be adequate to allow different users to select the styles that they prefer.

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.

stepancheg commented 3 years ago

I find that cfg-guarded imports are harder to manage with your preferred arrangement

Well, it depends. I had a scenario in mind (from various my projects) where they are easier to manage when split, e. g.

use crate::foo::Bar;
#[cfg(feature = "baz")]
use crate::foo::Baz;

But I agree, I was a bit sloppy presenting my arguments, thank you for pointing it out.

exFalso commented 3 years ago

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.

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 use blocks at all. It's usually all managed by the IDE auto-importing + rustfmt. Cases where I do have to look at it:

  1. Ambiguous imports like std::sync::Mutex vs tokio::sync::Mutex, or tokio::sync::mpsc::channel vs tokio::sync::oneshot::channel. Actually because of this ambiguity I've lately started to use qualified names in code more often, when it makes reading easier.
  2. as renames. I mostly avoid these but other devs sometimes use it.
  3. Splitting a module. Because of the use of item style, this consists of:
    1. cut+paste code to split out
    2. copy+paste full use block
    3. delete each line that triggers an unused warning. Actually I've made macros/sed expressions to do this because it's so automatable. This is of course a lot more painful with merged use statements.
  4. Auto-import IDE feature, or another dev puts merged use statements into the code, causing merge conflicts. Another couple of minutes wasted.

1 and 2 are examples of "What is the value to me of finding out where a symbol comes from?".

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.

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 use statements help answer these questions, but I guess it's a personal preference, to each their own.

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.

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:

  1. For each style, list the objective advantages and disadvantages. Example of objective advantage: smaller diffs Example of subjective advantage: I think it's nicer Example of objective disadvantage: ambiguous formatting rules Example of subjective disadvantage: it's too verbose
  2. If there is a clear winner, pick that.
  3. If there is no clear winner, let's decide on no default. (and perhaps codify that decision?)
mulkieran commented 3 years ago

@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. [2]. See (3). [1]. There are no truly objective advantages. That's why this discussion has been going on for 18 months. All the advantages or disadvantages that I've seen pointed out are only advantages or disadvantages in the context of the particular workflow, toolset, and overall contribution habits of the person involved. You and I have different preferences because you prefer to use an IDE and I prefer to use vim. No doubt there are more reasons for other preference differences. As far as I can tell, @stepancheg and I differ in that I have a preference for rich and frequent interactions with the compiler and @stepancheg prefers a more textual approach. For example, if I want to know where a symbol is used, I make it private, and see what compiler errors then appear. Seems like @stepancheg would prefer to use grep. And so on and so forth.

So, is "no default" a viable choice?

timonbimon commented 3 years ago

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.

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.

At this point, I personally care more about the different styles being made available than about which one is the default. 🤗

Meta-comment I am also actually really curious to see how things like this get resolved in the Rust community? 😅 Is it design by committee? Will there be a knight in shining armor arriving on the scene to bridge the divide, make a decision and save the day? 😉

exFalso commented 3 years ago

we are probably all a little guilty of letting the elephant guide our rider

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.

[1]. There are no truly objective advantages.

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 am also actually really curious to see how things like this get resolved in the Rust community? :sweat_smile: Is it design by committee? Will there be a knight in shining armor arriving on the scene to bridge the divide, make a decision and save the day? :wink:

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.

So, is "no default" a viable choice?

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 :smile:

faern commented 3 years ago

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.

That's pretty much what we have now ... clearly the wrong choice :smile:

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.

tmpolaczyk commented 3 years ago

Let's just set the default to None and let each project decide its style. Because otherwise it seems inevitable that someone will come here to rant about how rustfmt broke their import style, not realizing that merge_imports is a configurable option because they don't even have a rustfmt.toml.

exFalso commented 3 years ago

How can diff size be the hill that so many are willing to die on? Hare merge conflicts really been so evil to you in the past?

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.

calebcartwright commented 3 years ago

Thanks everyone for weighing in and sharing your perspectives. My main takeaways at this time are:

  1. The current binary merge_imports option in rustfmt is not sufficient and on the rustfmt side we need to add support for additional strategies
  2. There does not appear to be a sufficient consensus at this time to establish a default merging strategy

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.

calebcartwright commented 3 years ago

Would encourage folks to review and provide feedback on https://github.com/rust-lang/rustfmt/issues/3362#issuecomment-752723371 in the rustfmt repo