`group_imports` does not differentiate between a local import and an external crate

[jhpratt]

trafficstars

Describe the bug

rustfmt does not differentiate between local imports not prefixed by self:: and external crates. The behavior is as expected when prefixed by self::.

To Reproduce

Using group_imports = "StdExternalCrate" in rustfmt.toml, the following code is considered formatted.

mod foo {
    struct Foo;
}

use foo::Foo;
use rand::random;

Expected behavior

rustfmt recognizes that foo is a local module, not an external crate.

mod foo {
    struct Foo;
}

use rand::random;

use foo::Foo;

Meta

• rustfmt version: rustfmt 1.4.30-stable (8c6769d 2021-01-18)
• From where did you install rustfmt?: rustup
• How do you run rustfmt: cargo fmt

[jhpratt]

@rustbot modify labels to +a-imports +only-with-option

[rustbot]

Error: This repository is not enabled to use triagebot. Add a triagebot.toml in the root of the master branch to enable it.

Please let @rust-lang/release know if you're having trouble with this bot.

[jhpratt]

Also, proc_macro should probably be grouped with std, alloc, and core.

[MattX]

rustfmt does not differentiate between local imports not prefixed by self:: and external crates. The behavior is as expected when prefixed by self::.

Hm, when we added this feature I thought there was no good way to differentiate between external and local imports if the latter were not explicitly prefixed, but taking a look at the code again, we might be able to use the (recently added?) file_mod_map field in the context. I'll experiment with this today.

Also, proc_macro should probably be grouped with std, alloc, and core.

Good point. Is there a list of standard crates somewhere? It would be nice to make sure we're not missing any others.

[calebcartwright]

Removing the bug label, as there's definitely not one here though perhaps some minor updates to the documentation for the variant could be helpful to explain the limitations.

As discussed in various other issues/pulls, including the recent #4693, I'm still highly skeptical that it will be possible to do more. Would be thrilled to be proven wrong as I know folks would appreciate these types of enhancements from rustfmt, but to reiterate a few points:

• rustfmt runs have to be idempotent, including when running rustfmt directly against a single file (e.g. format-on-save in editor scenarios) as well as when formatting larger/entire portions of a project (e.g. cargo fmt)
• different folks have different interpretations of what "local" imports mean. even zooming in on the in-same-file modules, are all modules in the file considered local? what about nested in-file modules?
• anything that's reliant on the presence of entries in the file_mod_map (which has been around for ages and is a crucial part of the formatting flow) will almost certainly fail to meet that idempotence requirement, so think about the impact of that on out-of-file/project "local" modules and the potential for confusion even if in-file mods can be grouped separately from std/crate and everything else (including other things users consider "local") in the external group

[jhpratt]

Of course the definition of "local" is up for debate, but under the definition currently used, wouldn't it be possible to just see if there is a mod foo in the file, treating foo::bar as local? It wouldn't catch all cases (you could do use crate::foo; and use foo::bar;, for example), but it would cover a large portion of what I consider good style.

[jplatte]

Also found that we had some files in tests/ in https://github.com/ruma/ruma where the imports from the crate being tested were grouped like crate-local imports in the StdExternCrate grouping. I wonder how others feel about that.

[RalfJung]

Removing the bug label, as there's definitely not one here

In that case the docs should be updated. The docs say "external crates" go into the middle group, and in the example in the OP, foo is definitely not an external crate. So rustfmt does not behave as documented, which means there must be a bug somewhere.

It seems like the middle group are "external crates as well as local modules that are not prefixed with self".

[RalfJung]

I have drafted an implementation of a lint that could be used to enforce all local imports to be recognizable by rustfmt: https://github.com/rust-lang/rust/pull/125645.