feat: Added remove unused imports assist #14723

obsgolem · 2023-05-03T03:27:59Z

This resolves the most important part of #5131. I needed to make a couple of cosmetic changes to the search infrastructure to do this.

A few open questions:

Should imports that don't resolve to anything be considered unused? I figured probably not, but it would be a trivial change to make if we want it.
Is there a cleaner way to make the edits to the use list?
Is there a cleaner way to get the list of uses that intersect the current selection?
Is the performance acceptable? When testing this on itself, it takes a good couple seconds to perform the assist.
Is there a way to hide the rustc diagnostics that overlap with this functionality?

obsgolem · 2023-05-03T03:59:59Z

The tests are failing due to trailing whitespace. This whitespace is in my test cases, how do I bypass this? Or should I go through and make sure that the assist never leaves behind trailing whitespace?

matklad · 2023-05-03T09:17:02Z

Cleaning up trailing ws would be better! This is something ted should take care of for you, but it doesn't fully yet. Eg, see how insert_all fixes up whitespace on insertion.

Also, take a look at edit_in_place.rs --- "remove use" probably belongs there.

Is the performance acceptable? When testing this on itself, it takes a good couple seconds to perform the assist.

Yeah, we definitely should be better. At the same time, I think it's ok to land a poorly performing version first, it it is disabled via config (see AssistConfig). I haven't looked into why is it slow, but I think SearchScope::module_and_children(ctx.db(), m) might be one explanation. This means that, eg, for lib.rs will look at the whole crate, which is not great.

I think what we want perhaps is "not used in the current file". Like, yes, there's always a possibility that some module somewhere does use super::super::super::ImportedItem, but that feels like something we should lint against anyway.

Veykril · 2023-05-03T09:20:27Z

SearchScope::module_and_children(ctx.db(), m) is most likely the cause as it forces all modules to be re-parsed if they aren't in the LRU cache.

obsgolem · 2023-05-04T01:15:37Z

I think what we want perhaps is "not used in the current file". Like, yes, there's always a possibility that some module somewhere does use super::super::super::ImportedItem, but that feels like something we should lint against anyway.

I am not sure I agree. Consider the test module. Uses in the test module shouldn't count as uses in the overall file.

I didn't realize that module_and_children included other files, I definitely agree that uses in other files shouldn't count.

bors · 2023-05-06T23:56:07Z

☔ The latest upstream changes (presumably #14733) made this pull request unmergeable. Please resolve the merge conflicts.

Veykril · 2023-05-09T05:25:27Z

Please remove the merge commit, we prefer rebases

bors · 2023-05-25T12:21:16Z

☔ The latest upstream changes (presumably #14881) made this pull request unmergeable. Please resolve the merge conflicts.

crates/ide-db/src/search.rs

crates/ide-assists/src/handlers/remove_unused_imports.rs

lnicola · 2023-05-30T06:10:09Z

This would be nice as a diagnostic too 😅.

Veykril · 2023-05-31T10:34:31Z

I didn't realize that module_and_children included other files, I definitely agree that uses in other files shouldn't count.

Did you change the behavior here?

obsgolem · 2023-05-31T17:42:24Z

I don't think so (been a few weeks, not 100% sure); I might have just forgotten to fix that. I will try to take a look at that tonight.

obsgolem · 2023-06-05T00:47:08Z

Went ahead and added a function for this. I needed a way to split a TextRange on another subrange, but I don't know where to put unit tests for TextRange methods, thus I haven't fully tested the module function yet.

Veykril · 2023-06-12T08:00:01Z

Sorry for the long silence, have been busy and sick (and slowly reviewing other PRs here), will try to get back to this this week.

Veykril · 2023-06-13T09:18:26Z

crates/ide-db/src/search.rs

+        fn split_at_subrange(
+            first: TextRange,
+            second: TextRange,
+        ) -> (TextRange, Option<TextRange>) {
+            let intersect = first.intersect(second);
+            if let Some(intersect) = intersect {
+                let start_range = TextRange::new(first.start(), intersect.start());
+
+                if intersect.end() < first.end() {
+                    (start_range, Some(TextRange::new(intersect.end(), first.end())))
+                } else {
+                    (start_range, None)
+                }
+            } else {
+                (first, None)
+            }
+        }
+
+        if let Some(range) = range {
+            let mut ranges = vec![range];
+
+            for child in module.children(db) {
+                let rng = match child.definition_source(db).value {
+                    ModuleSource::SourceFile(_) => continue,
+                    ModuleSource::Module(it) => it.syntax().text_range(),
+                    ModuleSource::BlockExpr(it) => it.syntax().text_range(),
+                };
+                let mut new_ranges = Vec::new();
+                for old_range in ranges.iter_mut() {
+                    let split = split_at_subrange(old_range.clone(), rng);
+                    *old_range = split.0;
+                    new_ranges.extend(split.1);
+                }
+
+                ranges.append(&mut new_ranges);
+            }
+
+            for range in ranges {
+                entries.insert(file_id, Some(range));
+            }
+        } else {
+            entries.insert(file_id, range);
+        }


Let's not add this stuff here, it's fine if we include inline child modules in a modules search range (in fact, the current logic here will remove block expressions if the declare items which is also definitely unexpected).

Without this we can have a mismatch between the rustc warning and the assist. Is that ok? I can fix the block expression thing just by ignoring that case in the match.

As in an import that is not used in the module but in the child would silence the assist?

That is correct. use_in_submodule_doesnt_count tests this. If you remove this code then that test will fail.

Well, I also just realized that this doesn't work here since SearchScope's entries is a hashmap (so the code here currently only sets the search range to the last one). I think it would be better to just construct the whole module search scope then and have the remove_unused_imports assist split the searchscope accordingly into multiple for each range of the module then (like is done here right now)

I moved this to remove_unused_imports.rs and made it return a Vec<SearchScope>. This function could still easily go in search.rs I think.

obsgolem · 2023-07-09T20:34:21Z

Apologies for the delay. Final Fantasy 16, then life got me. Gonna update this tonight.

…the search scope by reference.

adamhp · 2023-07-21T16:42:07Z

@obsgolem @Veykril Anything left on this? 🙂

Veykril · 2023-08-01T09:36:45Z

@bors r+

bors · 2023-08-01T09:36:47Z

📌 Commit 7a87a35 has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-08-01T09:50:18Z

⌛ Testing commit 7a87a35 with merge 62dcf39...

bors · 2023-08-01T10:07:22Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 62dcf39 to master...

lnicola · 2023-08-07T07:26:28Z

remove-unused-imports.mp4

lem0nify · 2023-08-09T23:45:11Z

finally!
but why not to add this to vscode's Organize Imports (Shift + Alt + O) action?

heksesang · 2023-08-11T13:04:32Z

This feature also seems to remove imports of attributes like async_trait which are actually in use. And there seems to be no test of how this works with imported attributes in the test suite?

obsgolem · 2023-08-11T19:59:35Z

@heksesang Probably want to put in an issue for that. That might be a limitation of the current find usages code.

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2023

obsgolem force-pushed the master branch from b30f596 to eb2f9be Compare May 3, 2023 03:40

obsgolem force-pushed the master branch 3 times, most recently from 56615d1 to fb0a675 Compare May 4, 2023 02:00

obsgolem force-pushed the master branch from feb5c46 to 54962af Compare May 9, 2023 13:20

WassimBenzarti approved these changes May 10, 2023

View reviewed changes

Veykril reviewed May 28, 2023

View reviewed changes

crates/ide-db/src/search.rs Outdated Show resolved Hide resolved

crates/ide-assists/src/handlers/remove_unused_imports.rs Outdated Show resolved Hide resolved

obsgolem force-pushed the master branch from 54962af to 35883a2 Compare May 30, 2023 03:19

Veykril reviewed Jun 13, 2023

View reviewed changes

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 22, 2023

obsgolem added 2 commits July 9, 2023 17:30

Updated search to expose some more functions and to make search take …

b9cef03

…the search scope by reference.

Added remove_unused_imports assist.

7a87a35

obsgolem force-pushed the master branch from b3d8238 to 7a87a35 Compare July 9, 2023 21:44

Veykril approved these changes Aug 1, 2023

View reviewed changes

bors merged commit 62dcf39 into rust-lang:master Aug 1, 2023

lnicola changed the title ~~Added remove unused imports assist~~ feat: Added remove unused imports assist Aug 1, 2023

obsgolem mentioned this pull request Aug 22, 2023

Find all usages skips usages in derives #15501

Open

mawkler mentioned this pull request Feb 26, 2024

Feature request: support removal of unused imports mrcjkb/rustaceanvim#259

Closed

feat: Added remove unused imports assist #14723

feat: Added remove unused imports assist #14723

Uh oh!

Conversation

obsgolem commented May 3, 2023

Uh oh!

obsgolem commented May 3, 2023

Uh oh!

matklad commented May 3, 2023

Uh oh!

Veykril commented May 3, 2023

Uh oh!

obsgolem commented May 4, 2023

Uh oh!

bors commented May 6, 2023

Uh oh!

Veykril commented May 9, 2023

Uh oh!

bors commented May 25, 2023

Uh oh!

Uh oh!

Uh oh!

lnicola commented May 30, 2023

Uh oh!

Veykril commented May 31, 2023

Uh oh!

obsgolem commented May 31, 2023

Uh oh!

obsgolem commented Jun 5, 2023

Uh oh!

Veykril commented Jun 12, 2023

Uh oh!

Veykril Jun 13, 2023

Choose a reason for hiding this comment

Uh oh!

obsgolem Jun 13, 2023

Choose a reason for hiding this comment

Uh oh!

Veykril Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

obsgolem Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Veykril Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

obsgolem Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

obsgolem commented Jul 9, 2023

Uh oh!

adamhp commented Jul 21, 2023

Uh oh!

Veykril commented Aug 1, 2023

Uh oh!

bors commented Aug 1, 2023

Uh oh!

bors commented Aug 1, 2023

Uh oh!

bors commented Aug 1, 2023

Uh oh!

lnicola commented Aug 7, 2023

Uh oh!

lem0nify commented Aug 9, 2023

Uh oh!

heksesang commented Aug 11, 2023

Uh oh!

obsgolem commented Aug 11, 2023

Uh oh!

Uh oh!