Assist: group imports

[matklad]

use std::fs; // <- cursor is on this line
use std::io::prelude::*;
use std::io::BufReader;
use std::path::Path;

->

use std::{
    fs,
    io::prelude::*,
    io::BufReader,
    path::Path,
};

A similar, but simpler assist is split import

[max-frai]

Oh yes, we need this! :)

p.s. Maybe it should be recursive and also group io inners?

[matklad]

@max-frai I think it should be non-recursive for the start: it's possible to apply the assist several times manually. Longterm, what we really need here is assist groups like in intelliJ

💡
 |group uses      | > | recursively |
 |optimize imports|

[max-frai]

You are right. Also, it would be very useful to introduce: remove unused imports. Can rust-analyzer find such places?

[matklad]

not yet :(

…

On Wed, 13 Nov 2019 at 17:22, Max Frai ***@***.***> wrote: You are right. Also, it would be very useful to introduce: remove unused imports. Can rust-analyzer find such places? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2220>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M4LO7E5NPW3IOZSFZLQTQERZANCNFSM4JMCZINQ> .

[killercup]

For the record, rustfmt's unstable merge_imports option does exactly this.

[Matthias247]

I have played a bit around with this. Mostly to get more familiar with how rust-analyzer works. Not yet sure whether I will get it to completion :-)

One question regarding the implementation: It seems like this can be implemented in 2 ways:

Option A: Manipulate the existing syntax tree

We identify the common path segments of all use statements, and then use edit commands to insert braces and commas at the correct places and remove the use prefix:: part of the next statement. That's what my hacky first implementation does, and where the edit looks like:

ctx.add_assist(AssistId("group_import"), "group import", |edit| {
      edit.target(use_node.syntax().text_range());
      edit.insert(open_brace_segment.syntax().text_range().start(), "{");
      edit.insert(close_brace_segment.syntax().text_range().end(), "}");
      edit.delete(to_remove);
      edit.replace(use_semicolon_text_range, ",");
  })

The downside of this approach seeems to be that it seems to either falls down or get super complex on more complex merges. E.g. if the first statement already contains an opening brace for submodules, and we need to shift the second import into it without adding another brace. I guess also if things are already trees instead of just long import statements it gets more complicated.

Option B: Rewrite import statements

We parse all imports in suitable tree structure (like hir::Path, but I think that one only represents a single path and not an import tree), try to merge those, and if the merge and if the merge succeeds write the complete tree (which replaces all existing things).

I feel like this option would be easier to manage than trying to figure out precise necessary edits. But maybe I'm overestimating the effort. The downside of Option B is certainly that it's more invasive, and if the rewriting part does not support all AST constructs it would be a lossy and buggy operation.

[matklad]

Yeah, "how to edit syntax trees" is an open question. I've been doing this since 2015 and still don't know the answer :-)

One general rule of thumb is that it's better re-use, rather than re-write bits written by the user. For example, if the user did use std::collections::{HashMap /* FIXME: use hashbrown? */, BTreeMap}, we ideally should not lose the comment. Approaches which try to build a fully-abstract IR usually don't handle this Besides comments, it's often a good default to preserve formatting.

Another choice is how to do the edits. There are two approaches:

• edit the text directly, by just replacing ranges
• construct the new syntax node and replace an existing node with a new one

The latter usually is more verbose, but in general is easier to understand and, ideally, can handle some minor details like adding whitespace or punctuation automatically.

I would go with the latter approach, using something like this:

fn merge(trees: Vec<ast::UseTree>) -> Option<ast::UseTree> {
  let common_prefix: ast::Path = common_prefix(&trees)?;
  let subtrees: Vec<ast::UseTree> = Vec::new();
  for tree in trees.iter() {
    let subtree = remove_prefix(&common_prefix, &tree)
    subtrees.push(subtree)
  }
  make::use_tree(common_prefix, subtrees);
}

fn common_prefix(trees: &[ast::UseTree]) -> Option<ast::Path> {}
fn remove_prefix(prefix: &ast::Path, tree: &ast::UseTree) -> ast::UseTree

See the make.rs module for primitive building blocks of various ast elements. It doesn't have constructos for use items yet, they should be added .

[to-mas-kral]

Is anyone still working on this ? If not, I'd like to try to implement it.

[matklad]

@TomasKralCZ not that I am aware of, so go for it!

[Matthias247]

@TomasKralCZ In case it helps you. This is the piece of code I had written to experiment with it: https://gist.github.com/Matthias247/db335901003943cac36186812a191268

It handled some cases, but it seemed in general a lot more complicated (see discussion above). I'm currently distracted by other things, so no longer working on this.