"Dumb" auto import assist #762
Conversation
|
Haven't looked at the code yet, but the idea is 100% brilliant! |
crates/ra_assists/src/auto_import.rs
Outdated
| SyntaxKind::{ PATH, PATH_SEGMENT, COLONCOLON, COMMA } | ||
| }; | ||
| use crate::assist_ctx::{AssistCtx, Assist, AssistBuilder}; | ||
| use itertools::{ Itertools, EitherOrBoth }; |
There was a problem hiding this comment.
An interesting next step along the lines of this PR is "optimize imports" action, which reformats imports, sorts crate:: paths last and merges prefixes.
Though perhaps to do that we'll need a "structured editing API", that is, something like "remove this import" rather than "replace this sub string with that sub string". There's no such API in ra_syntax yet, it would be fun to design and implement it.
There was a problem hiding this comment.
Maybe having something like a mutable AST should be enough?
let mut mtreelist = treelist.get_mutable();
mtreelist.add_path(MutPath.from_str("foo::bar"))?
indent(mtreelist);
let diffs = diff(trellis, mstreelist); // get text insertions/deletions
Apart from ident and diff, I thing everything else can be autogenerated.
crates/ra_assists/src/auto_import.rs
Outdated
| use itertools::{ Itertools, EitherOrBoth }; | ||
|
|
||
| // TODO: refactor this before merge | ||
| mod formatting { |
There was a problem hiding this comment.
Yeah, formatting module in ra_ide_api_light is kinda random at the moment. Really, this should be a separate crate which is as powerful as rustfmt.
You can use formatting from ra_ide_api_light, but I would also love a separate PR which creates a new ra_fmt crate and moves this module there. I think we then would be able to drop dependency from assists on ide_api_light.
There was a problem hiding this comment.
I'll go for another PR then, introducing ra_fmt.
There was a problem hiding this comment.
Awesome! Feel free to add/remove stuff from fmt module, it's currently a somewhat ad-hoc collections of functions, I imagine it should grow into a better "all formatting" API in the future!
crates/ra_assists/src/auto_import.rs
Outdated
| return Some(v); | ||
| } | ||
|
|
||
| fn collect_path_segments_raw<'b, 'a: 'b>( |
There was a problem hiding this comment.
I think it's OK to only declare 'a lifetime and left 'b to elision.
crates/ra_assists/src/auto_import.rs
Outdated
|
|
||
| fn compare_path_segment(a: &ast::PathSegment, b: &ast::PathSegment) -> bool { | ||
| if let (Some(ka), Some(kb)) = (a.kind(), b.kind()) { | ||
| return match (ka, kb) { |
There was a problem hiding this comment.
Don't need this return. That's actually also a warning we can already implement.
crates/ra_assists/src/auto_import.rs
Outdated
| } | ||
|
|
||
| #[derive(Copy, Clone)] | ||
| enum PathSegmentsMatch { |
There was a problem hiding this comment.
Hm, I wonder if instead of enum we could return a single usize -- length of the common prefix. It sort-of has the same amount of info as this enum, and doesn't suffer the problem that Empy and Partial(0) are the same.
crates/ra_assists/src/auto_import.rs
Outdated
| // Add a brand new use statement. | ||
| AddNewUse( | ||
| Option<&'a SyntaxNode>, // anchor node | ||
| bool, // true if we want to add the new statement after the anchor |
There was a problem hiding this comment.
Perhaps it makes sense to make this a struct variant, if we need a comment anyway? Positional bools are awkward .
crates/ra_assists/src/auto_import.rs
Outdated
|
|
||
| // Find out the best ImportAction to import target path against current_use_tree. | ||
| // If current_use_tree has a nested import the function gets called recursively on every UseTree inside a UseTreeList. | ||
| fn walk_use_tree_for_best_action<'b, 'c, 'a: 'b + 'c>( |
There was a problem hiding this comment.
ditto, 'b and 'c are not required I think.
|
Haven't reviewed the bulk of the logic yet, but the general structure and tests look good to me! One alternative approach here would be to always add a simple "fully-qualified" path first, and then invoking a separate "optimize imports" action to rewrite & merge all the imports. |
|
Currently this assist is designed to modify existing code as little as possible. [The best solution in the long run in having |
Agree, but we totally need to rewrite |
|
Another thing "optimize imports" should do is to remove unused imports, so in the end it's not entirely a |
|
@matklad I addressed your comments. I removed |
766: Formatting code into ra_fmt r=matklad a=eulerdisk As discussed #762 (comment) I did only move the code without other improvements. Co-authored-by: Andrea Pretto <[email protected]>
auto_import: Removed unecessary lifetimes
|
bors r+ Awesome! |
762: "Dumb" auto import assist r=matklad a=eulerdisk This adds a new assist to "add xxx::yyy to the current file" when the cursor is on a PATH. It manages correctly nested imports,`self` keyword and creates new nested imports if necessary. [See the tests] It doesn't use name resolution so in that sense is 'dumb', but I have plans to do that. That in the future will be useful to auto import trait names in autocompletion for example. It can easily be extended to provide multiple actions to select in which scope to import. That's another thing I plan to do. @matklad I copied some indentation code from `ide_light`, I don't know at the moment if/how you want to refactor that code. This assist was meant to be in `ide_light`. Co-authored-by: Andrea Pretto <[email protected]>
Build succeeded |
|
Opened #779 about well-typed editing API. |
|
Hey, @eulerdisk just want to let you know that this code have effectively grown to a full-blown auto import recently, thanks for writing it more than a year ago! |
This adds a new assist to "add xxx::yyy to the current file" when the cursor is on a PATH. It manages correctly nested imports,
selfkeyword and creates new nested imports if necessary. [See the tests]It doesn't use name resolution so in that sense is 'dumb', but I have plans to do that. That in the future will be useful to auto import trait names in autocompletion for example.
It can easily be extended to provide multiple actions to select in which scope to import. That's another thing I plan to do.
@matklad I copied some indentation code from
ide_light, I don't know at the moment if/how you want to refactor that code. This assist was meant to be inide_light.