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

imports (`use`) #24

Closed
nrc opened this Issue Oct 3, 2016 · 169 comments

Comments

Projects
None yet
@nrc
Collaborator

nrc commented Oct 3, 2016

No description provided.

@nrc nrc referenced this issue Oct 3, 2016

Closed

Items #25

10 of 10 tasks complete
@jimmycuadra

This comment has been minimized.

jimmycuadra commented Oct 7, 2016

Here is what I do for my code:

  • Three sections, with one blank line after each:

    1. Standard library imports
    2. External crate imports
    3. Internal imports
  • Obviously, if a section is omitted because there are no imports of that type, the blank line after it is omitted.

  • Absolutely no glob imports. I like to be very explicit, even if it's verbose.

  • Always import the item itself, not the parent module. If the item has some generic name that makes it hard to identify at the site of use, rename it to something more descriptive.

  • Each import section has its lines sorted alphabetically (or more accurately, by whatever sort's rules are.)

  • If multiple items are imported from one module, those items are sorted, e.g. use foo::{Bar, Foo, baz};.

  • If a large number of items are imported from one module, such that the line would wrap past whatever column width you're using, list one item on each line, with the lines sorted, e.g.:

    use foo::{
      Bar,
      Foo,
      baz,
    };
  • When importing multiple items in the multi-line form from the previous bullet point, each item should have a comma after it, including the last item.

@alexcrichton

This comment has been minimized.

alexcrichton commented Oct 7, 2016

I personally also follow @jimmycuadra's "three sections" rule as well, and I find it very beneficial for knowing what's imported at a glance. I imagine it could be hard for rustfmt to distinguish between internal/external crate imports, however :(, so it may not end up making it all the way through.

Another rule I personally follow is:

  • Never import multiple modules one one line (e.g. use std::{f32, f64}), instead always split them on multiple lines.

And finally, I personally prefer to never split an import across multiple lines, my preference is to:

use foo::{...}; // up to the character limit
use foo::{...}; // remaining imports

Again though, just personal preferences and/or ideas!

@jimmycuadra

This comment has been minimized.

jimmycuadra commented Oct 7, 2016

Since rustfmt only operates on a file (at least as far as I understand), distinguishing between internal modules and modules in external crates might not be possible, but I could imagine a Cargo integration being able to do it by having Cargo pass information to rustfmt about external crates.

@lu-zero

This comment has been minimized.

lu-zero commented Oct 7, 2016

I also find good having "three sections", possibly 1-line separated.

And I like a lot @alexcrichton way to repeat use foo::{...} to keep the lines within limit.

@jimmycuadra

This comment has been minimized.

jimmycuadra commented Oct 7, 2016

The reason I like to use the one-line-per-item format for multi-item imports is that it makes for very clean diffs in addition to making it easy to read. It's simply a line removed for any item removed and a line added for any item added. If you use either of these styles:

use foo::{a, b, c, d, e, f, g};
use foo::{h, i, j, k, l, m, n};
use foo::{a, b, c, d, e, f, g,
          h, i, j, k, l, m, n};

then changing the items imported often results in a shuffling/reordering of items across lines that makes it less obvious what changed in a diff.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Oct 7, 2016

I follow the same style @alexcrichton described.

I do, often, import a module and use a one-level-qualified name at the call site, especially when a module uses highly generic names that assume namespacing. I prefer foo::init() over foo::foo_init().

@petrochenkov

This comment has been minimized.

petrochenkov commented Oct 7, 2016

The "three sections" rule seems to be popular and have precedents in other language style guides.

I usually use the rule "no more than three segments in a path" and and use imports respectively.
Functions are in one-level qualified form m::func(), types (including enums) in non-qualified form MyType or m::MyType if ambiguous, variants in enum-qualified form (Enum::Variant).

@ubsan

This comment has been minimized.

Contributor

ubsan commented Oct 7, 2016

What I do is:

// if they all fit, do this:
use foo::{a, b, c};
// if they fit on one line, but not with the beginning part, do this:
use foo::{
  a, b, c
};
// and if they don't fit on one line at all, do this:
use foo::{
  a,
  b,
  c,
};

basically following rustic style guide for function args. I do like the three sections.

The one thing I really disagree with from @jimmycuadra is not importing the module itself. I very often import modules in addition to items:

use std::fmt::{self, Debug, Display};

impl Display for MyType {
  fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
    write!(f, "hello, world")
  }
}
@steveklabnik

This comment has been minimized.

steveklabnik commented Oct 7, 2016

For a while, I tried to do

extern crate foo;

use foo::bar;
use foo::baz;

extern crate bar;

use bar::foo;

etc, but in the end, didn't like it as much as what @jimmycuadra suggested.

Always import the item itself, not the parent module.

The "standard style" here (in my understanding) is to import types directly, but import the parent modules for functions. I really, really like this.

I also tend to not use {}, but it seems I'm very much in the minority here. If I'm working with code that already has it, I'll use it, but also tend to prefer

use foo;
use foo::{bar, baz};

over

use foo::{self, bar, baz};

But it seems that I'm in the minority here as well.

@skade

This comment has been minimized.

skade commented Oct 7, 2016

I tend to be in the "explode everything" crowd, and rarely use "{...}". I find it confusing to manipulate and it gets noisy quick.

I do glob imports, though, especially for prelude modules, such as std::io::prelude. That shouldn't be disallowed.

@luser

This comment has been minimized.

luser commented Oct 7, 2016

The "standard style" here (in my understanding) is to import types directly, but import the parent modules for functions. I really, really like this.

I believe I generally follow this as well, with one notable exception: I always qualify std::io::Result by doing use std::io; and referring to it as io::Result rather than importing it because otherwise it shadows std::result::Result.

@steveklabnik

This comment has been minimized.

steveklabnik commented Oct 7, 2016

@luser yes, agree 100%. Nice catch. So this is something like "except types which may shadow Prelude types"

@jimmycuadra

This comment has been minimized.

jimmycuadra commented Oct 7, 2016

The "standard style" here (in my understanding) is to import types directly, but import the parent modules for functions. I really, really like this.

Although I don't do this currently, I like that, too, and wouldn't be disappointed if rustfmt ended up standardizing on that style.

Not using glob imports is also not exactly a "style" rule, so I'm not sure that is applicable to rustfmt, although it is part of my personal practice.

@softprops

This comment has been minimized.

softprops commented Oct 7, 2016

I'm pro what @ubsan said though what @jimmycuadra said about one line per import for clean diffs is is weighing on me and is the first technical argument rather than subjective I've seen so far. Whether stylistically I agree or not ( I do in this case ) the stronger technical argument should win. The diff thing is also just not a case of personally preference, is a better case for making it easier to work in teams on rust code bases.

Another thing possibly consider is perhaps instead of two or three conditionally styled cases. Just pick one. I like that about gofmt. Its consistent because the default only has one option for import line breakage.

I'm also pro the 3 sections of use clauses.

Im not sure if extern lines are out of the scope or not for this discussion but if the aren't, I like all my externs at the top of the file followed by use lines. No technical argument to back that up though. Just my current habit.

pub mod foo; is probably in the same arena, maybe for another topic thread...

@ubsan

This comment has been minimized.

Contributor

ubsan commented Oct 8, 2016

@softprops the idea of my style (beside being easy to read) is that a change in the number of arguments is one line diff, while not having so many lines :)

@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 9, 2016

Since rustfmt only operates on a file (at least as far as I understand), distinguishing between internal modules and modules in external crates might not be possible, but I could imagine a Cargo integration being able to do it by having Cargo pass information to rustfmt about external crates.

With changes to the compiler that are currently underway, Rustfmt certainly could get this information without any extra input from Cargo.

@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 9, 2016

Absolutely no glob imports. I like to be very explicit, even if it's verbose.

I'd be happy to recommend this, but I don't think Rustfmt should enforce it (it is really easy to implement as a refactoring - rustw and the RLS both support it).

Always import the item itself, not the parent module. If the item has some generic name that makes it hard to identify at the site of use, rename it to something more descriptive.

I tend to import traits directly (because you have to) and types directly, but not functions, these I prefer to import the module and use one level of scoping.

Each import section has its lines sorted alphabetically (or more accurately, by whatever sort's rules are.)

Is it worth putting either lower case of upper case - initialled names first? I sometimes do this and think it looks neater, but often don't.

If present, self should come before other names in an import, not in alphabetical order.

If a large number of items are imported from one module, such that the line would wrap past whatever column width you're using, list one item on each line, with the lines sorted, e.g.:

Personally I don't find this useful - it seriously uses up vertical space and I rarely need to scan imports - they are mostly boilerplate.

@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 9, 2016

Never import multiple modules one one line (e.g. use std::{f32, f64}), instead always split them on multiple lines.

Interesting, seems like a good recommendation, I think we might be able to enforce it too, but not until some big name resolution changes land in the compiler

And finally, I personally prefer to never split an import across multiple lines, my preference is to:

I used to do this, but Rustfmt takes the opposite approach, using visual indentation, and I think I prefer that style, feels more consistent with other formatting.

@nrc

This comment has been minimized.

Collaborator

nrc commented Oct 9, 2016

I do glob imports, though, especially for prelude modules, such as std::io::prelude. That shouldn't be disallowed.

Yeah, prelude modules are a good use for glob imports, IMO.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Oct 9, 2016

If present, self should come before other names in an import, not in alphabetical order.

Agreed.

Also, the term I've seen used for "whatever sort does" (implicitly, "in LC_COLLATE=C"): ASCIIbetically.

@solson

This comment has been minimized.

Contributor

solson commented Oct 9, 2016

I agree that glob imports should mostly be avoided, but not always. One rule I follow more stringently is to never have more than one glob import in scope.

With one glob, if I have an unknown name, I know it's either in my list of explicit imports or comes from the one glob, but with multiple globs I have no way of knowing where it comes from without tool support.

+1 for self followed by names in ASCIIbetical order.

@ubsan

This comment has been minimized.

Contributor

ubsan commented Oct 9, 2016

Never import multiple modules one one line (e.g. use std::{f32, f64}), instead always split them on multiple lines.

I do not like this. I always write use std::{f32, f64};, when the modules are related. Pulling them out into multiple lines just feels like overkill.

@kornelski

This comment has been minimized.

kornelski commented Oct 13, 2016

I'm OK with single-line use …::{…}, but multi-line use { looks weird to me, especially with one ident per line. I think I expect :: to be visually near each identifier for it to look distinct from struct/enum declarations at the first glance.

use foo::{bar, baz};
use foo::VeryLongIdentifier;
use foo::ThatWouldMakeLineTooLong;
@luser

This comment has been minimized.

luser commented Oct 13, 2016

We use multi-line import in some Mozilla Python code, and I thought maybe that was specified by PEP-8, but it's not. The PEP-8 guidelines are pretty similar to what's being suggested here:
http://pep8.org/#imports

The Django coding style guidelines do recommend using from foo import ( ... ) for long lists of imports, but not an imported-item-per-line:
https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/#imports

They even recommend a tool specifically for formatting Python imports:
https://github.com/timothycrosley/isort#readme

@Screwtapello

This comment has been minimized.

Screwtapello commented Oct 14, 2016

One advantage to never using {} is that it's easier to grep a codebase for uses of some module: you can just grep for "std::f64" instead of "std::.*\bf64\b" (and even then I'm not sure that would find all the relevant matches).

One advantage to always importing a module (possibly with a short alias), never individual functions or types: people are less likely to manually namespace things if they're already namespaced. For example, in a world where people import types directly, I might declare database.DatabaseConnection and websocket.WebSocketConnection so that you can refer to both of them in the same code without confusion. If the convention is to only import modules, I can just call them database.Connection and websocket.Connection without risk of confusion—and if that's too verbose, you can alias them to db.Connection and ws.Connection.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Oct 17, 2016

One additional item that came up in today's rust-style meeting:

Don't use super:: to reference names outside the current file. Using super:: to access names from the module in the same file from within a mod tests { seems fine, but using it to access names from other files seems excessively confusing; use absolute paths instead.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Oct 17, 2016

Note that we also need guidelines for using names renamed with as.

One extremely common name conflict I've seen handled multiple ways:

use std::fmt; // and use fmt::Write
use std::io; // and use io::Write

versus

use std::fmt::Write as FmtWrite;
use std::io::Write as IoWrite;
@solson

This comment has been minimized.

Contributor

solson commented Jul 17, 2017

in the case of option A, tedious manual reflowing still exists

In Sublime: mark first comma, hit [ctrl] + [d] for each new comma, [⇨], [enter], add trailing comma, done. And I guess it's not slower in many other editors.

I mean... if we start talking about specific editor workflows, option C is easiest for me to reflow 'manually' in vim. I just select the block and hit gq to reflow it based on the current linewidth setting.

@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Jul 17, 2017

@nrc

I'm not convinced this is significantly more of a hassle than formatting a complicated expression by hand. I'm also not really convinced that ease of manual formatting is that important as a factor. And, IMO, the effort of manually sorting a list of imports is so much greater than any re-wrapping would be that it's not a great argument here.

Formatting a complicated expression, or manually keeping a list of imports sorted, is quite a bit easier than reflowing a whole list of names; the latter is disproportional to the amount of editing done; if you make a minor tweak, you don't generally have to reflow a ton of code.

@LukasKalbertodt

And what about removing an import? The style would require to reflow everything again (if possible). But here the programmer might not do that because this style violation is much more subtle than breaking the line limit. And thus we might end up with slightly incorrect styles.

Speaking from experience, this definitely happens for other things that are formatted that way. I often see lists/vectors/arrays formatted that way in other code, and the lines never get fully reflowed with each edit.

@solson

I mean... if we start talking about specific editor workflows, option C is easiest for me to reflow 'manually' in vim. I just select the block and hit gq to reflow it based on the current linewidth setting.

True, that's what I'd do as well (at least, unless there's a similar key combination I can hit to re-rustfmt the file).

@crumblingstatue

This comment has been minimized.

crumblingstatue commented Jan 6, 2018

Now that nested imports are available, we also need to figure out how we want to format those.

Here is an example of how rustfmt currently formats them:

use {graphics::{animations, sprite_set_subrect, SpriteId},
     sfml::{graphics::{RenderTarget, RenderWindow, Sprite, Texture, Transformable}, window::Key},
     ROOM_H, ROOM_W};

We might want to format it in a more readable manner, for example:

use {
    graphics::{animations, sprite_set_subrect, SpriteId},
    sfml::{
        graphics::{RenderTarget, RenderWindow, Sprite, Texture, Transformable},
        window::Key
    },
    ROOM_H, ROOM_W
};

Or at least not revert to the 1st form if the user formats it manually to the 2nd form.

@topecongiro

This comment has been minimized.

topecongiro commented Jan 18, 2018

When reordering imported items, I would like to put them after simple imports.
And like @crumblingstatue stated in #24 (comment), we probably should put nested imports on its own line:
For example, I prefer A to B or C:

// A
use foo::{
    b, d, f, g, h,
    a::{x, xx, xxx},
    c::{y, yy, yyy},
    e::{z, zz, zzz},
};

// B
use foo::{
    a::{x, xx, xxx}, b, c::{y, yy, yyy}, d, e::{z, zz, zzz}, f, g, h,
};

// C
use foo::{
    a::{x, xx, xxx},
    b,
    c::{y, yy, yyy},
    d,
    e::{z, zz, zzz},
    f, g, h,
};
@joshtriplett

This comment has been minimized.

Contributor

joshtriplett commented Jan 18, 2018

@topecongiro I very much prefer C in that list, for maintainability. Without keeping the list alphabetized, I find it difficult to quickly scan the list. And on top of that, I often find myself converting one type to the other, when I add or drop an item. For instance, an import of c becomes an import of c::{self, foo}. Having to move it to another point in the list when doing that makes that edit more invasive.

@nrc

This comment has been minimized.

Collaborator

nrc commented Jan 18, 2018

Hmm, at first blush, I too prefer alphabetical ordering. However, I think it is worth considering that a nested list is a categorically different kind of import and just like we separate self from the other names, we might want to do the same with nested lists. Consider scanning, when scanning imports we are usually looking for an imported name primarily, and secondarily the module that the name is imported from. For nested list imports we are scanning for x or xx in the above example, not for a and therefore ordering the nested list with a might even make scanning harder.

I'm not sure how I feel about the edit distance argument. I feel like we don't have a best practice for when to use nested imports. My instinct is that if you include self in an import list, it might want to be top-level. But I am not a heavy user of the feature so I'm not sure how that works out in practice.

@andersk

This comment has been minimized.

andersk commented Jan 19, 2018

Here’s a potential compromise designed to be human-editable and keep diffs reasonably small without wasting too much vertical space.

  • Try to use one line by default.
  • If that line is too long, then break it between every two imported names whose first characters differ.
  • If any resulting line is still too long, then further break it between every two imported names whose second characters differ, and so on.

If we were importing every module in std and wrapping at 45 characters, this might look like

use std::{
    any, ascii,
    borrow, boxed,
    cell,
    char,
    clone,
    cmp,
    collections, convert,
    default,
    env, error,
    f32, f64, ffi, fmt, fs,
    hash,
    i8, i16, i32, i64, io, isize, iter,
    marker, mem,
    net, num,
    ops, option, os,
    panic, path, prelude, process, ptr,
    rc, result,
    slice, str, string, sync,
    thread, time,
    u8, u16, u32, u64, usize,
    vec,
};

Of course, with a more reasonable maximum line length, there’d be no reason to split up the c line here, so I picked 45 just for demonstration. Most diffs will be one-line changes. The diff removing (say) clone does a bit of rewrapping, but it’s still localized to the c group:

@@ -1,11 +1,7 @@
 use std::{
     any, ascii,
     borrow, boxed,
-    cell,
-    char,
-    clone,
-    cmp,
-    collections, convert,
+    cell, char, cmp, collections, convert,
     default,
     env, error,
     f32, f64, ffi, fmt, fs,
@sackery

This comment has been minimized.

sackery commented Jan 19, 2018

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

Reorder imports in groups and child uses in group of its.

@cramertj

This comment has been minimized.

cramertj commented Apr 12, 2018

I'm quite late to this discussion-- I found this thread after requesting that use_nested_groups be supported. I'd like to suggest that, rather than placing imports on one long line as is currently done, that grouped paths with multiple segments (including nested groups) always get moved to their own line.

For example:

use foo::bar::baz; // one import, one line
use foo::bar::{foo, bar, baz}; // one group, one line

// Don't do this
use foo::bar::{foo::bar, baz};
// Do this
use foo::bar::{
    foo::bar,
    baz,
};

// Don't do this
use foo::bar::{foo::{bar, baz}, biz};
// Do this
use foo::bar::{ 
    foo::{bar, baz},
    biz,
};
@nrc

This comment has been minimized.

Collaborator

nrc commented Apr 14, 2018

Going to try and summarise the thread and outstanding issues. To categorise: basic formatting, handling long imports (multi-line formatting), ordering of names in import groups, ordering multiple imports, handling nested imports, recommendations about how to import (e.g., discourage globs, recommend importing modules vs items), normalisation and merging/un-merging imports.

Simple formatting

use a::b::{foo, bar, baz};

I think this is pretty uncontroversial, the only interesting thing here is no spaces around the braces which we don't do elsewhere, but I think this is well established.

Recommendations

I propose we don't make recommendations beyond formatting, simply because I think it is hard work and beyond our remit.

Merging/un-merging imports

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

Large imports

We recommend that import lists should not be split over multiple lines (however, tools won't split them by default). Where an import list does go multi-line, we block indent and allow multiple names per line (but see nested imports below).

// Prefer
foo::{long, list, of, imports};
foo::{more, imports};

// If necessary
foo::{
    long, list, of, imports, more,
    imports,  // Note trailing comma
};

Ordering of imports

Note that tools which only have access to syntax (such as Rustfmt) cannot tell which imports are from an external crate or the std lib, etc.

I propose that newlines are used to group imports. Within a group, we sort imports ascii-betically. Groups of imports are not merged nor re-ordered.

E.g., input:

use d;
use c;

use b;
use a;

output:

use c;
use d;

use a;
use b;

Because of macro_use, attributes must also start a new group and prevent re-ordering.

Ordering within an import

I propose that names within an import are sorted ascii-betically, but with self and super first, and groups and glob imports last, this applies recursively, so a::* comes before b::a but a::b comes before a::*. E.g., use foo::bar::{a, b::c, b::d, b::d::{x, y, z}, b::{self, r, s}};.

Normalisation

Tools must make the following normalisations:

  • use a::self; -> use a;
  • use a::{}; -> ``
  • use a::{b}; -> use a::b;

And must apply these recursively.

Tools must not otherwise merge or un-merge import lists or adjust glob imports (without an explicit option).

Nested imports

See follow-up comment

@nrc

This comment has been minimized.

Collaborator

nrc commented Apr 14, 2018

Nested imports

Note that ordering of nested imports is discussed above. Given the above proposals to reorder, but not to merge nested imports, I think the only remaining question is how to layout. I think there are two possibilities (previously suggested):

  • Ignore the nested-ness of imports - fit all on one line if possible, split on multiple lines if necessary
    • a variation of this proposal is that if we split on to multiple lines, then a nested import should be on its own line, e.g.,
use a::b::{
    x, y, z,
    w::{...},
    u::{...},
};
  • If there are any nested imports, then split the import across multiple lines (either one import per line, or one nested import per line)

I think we could have a heuristic approach too - if the import is small then put on one line, if not split on multiple lines. An example small heuristic here might be no more than one nested import, and the nested import has no more than two component names, or something similar.

I think the sentiment on the thread is in favour of always multi-lining nested imports, and I think I agree.

@sdleffler

This comment has been minimized.

sdleffler commented Apr 14, 2018

I'm definitely in the "multi-line nested imports" camp, but at the same time I don't want to see an import with lots of small items split with an item on each line; I really like the nested-import-on-own-line idea. What I'm worried about is this:

use foo::{
    Bar,
    Baz,
    Quux,
    Corge,
    garply::{grault, ...}
};

I would rather have:

use foo::{
    Bar, Baz, Quux, Corge,
    garply::{grault},
};

nrc added a commit that referenced this issue May 2, 2018

Imports
Closes #24

@nrc nrc referenced this issue May 2, 2018

Merged

Imports #129

@nrc nrc closed this in #129 May 10, 2018

kgadek added a commit to blancmanges/gatekeeper that referenced this issue Jun 30, 2018

fmt: formatting imports
Heavily inspired by discussion here: rust-dev-tools/fmt-rfcs#24 .

kgadek added a commit to blancmanges/gatekeeper that referenced this issue Jun 30, 2018

fmt: formatting imports
Heavily inspired by discussion here: rust-dev-tools/fmt-rfcs#24 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment