From 98bc46002cdd8daff1baf0756ecc60dbdf420fd9 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 23 Sep 2024 16:14:23 -0300 Subject: [PATCH] feat: (LSP) remove unused imports (#6129) # Description ## Problem Part of #1579 ## Summary This implements another commonly used feature: "remove unused import/s". This is when the entire `use` item is removed: ![lsp-remove-unused-imports-1](https://github.com/user-attachments/assets/7b10d429-5e31-4f05-bbc2-ba87e7ccf9bf) This is when part of the `use` item is removed: ![lsp-remove-unused-imports-2](https://github.com/user-attachments/assets/01a819a6-1790-4701-8820-b0d6188c5497) This is when "Remove all the unused imports" is chosen with a selection range: ![lsp-remove-unused-imports-3](https://github.com/user-attachments/assets/9797d88d-960e-4da5-8d11-913431741381) ## Additional Context It works exactly the same as Rust Analyzer: 1. You get a "Remove unused import", "Remove unused imports" or "Remove the entire `use` item" on each `use` item that has unused import(s) 2. You also get a "Remove all the unused imports" code action. This will remove all unused imports in the current selection. I need to clarify 2 because it's something I learned while coding this: a code action is suggested in the selection you have (the selection is the cursor in case there's no selection, though VSCode would sometimes use the current line's error span as the selection). That's why if you have the cursor on an import and choose "Remove all the unused imports" it just removes the imports in the current `use` item. You can get VSCode to actually remove **all** the unused imports if you select all the `use` items, then ask for code actions, then choose "Remove all the unused imports". To be honest this is kind of annoying, but it kind of also makes sense. We _could_ make "Remove all the unused imports" remove all of them regardless of what's the selection (or, well, if at least one selection has unused items) but we could do that as a follow-up. --- Other things to know: 1. This works by creating a new `UseTree` from the original `UseTree` but with imports removed, then converting that to a string (formatting it with `nargo fmt`). What happens if there were comments in the middle of the import? They are lost! But... this is also what Rust Analyzer does. I guess worse case scenario is you revert that code action and manually remove things (comments in these positions are very unlikely to happen). 2. I didn't test the "Remove all the unused imports" action because right now our tests don't support a selection range. But given that the logic for that is relatively simple (just get all the "remove unused import" edits and present that as a new code action) I thought it was okay not to test it (I did try it out, though). We could eventually add tests for this, though. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/ast/statement.rs | 6 +- compiler/noirc_frontend/src/ast/visitor.rs | 4 +- .../src/hir/def_collector/dc_crate.rs | 2 +- compiler/noirc_frontend/src/node_interner.rs | 7 + tooling/lsp/src/requests/code_action.rs | 63 ++-- .../code_action/remove_unused_import.rs | 314 ++++++++++++++++++ tooling/lsp/src/requests/code_action/tests.rs | 2 + tooling/lsp/src/requests/completion.rs | 7 +- 8 files changed, 379 insertions(+), 26 deletions(-) create mode 100644 tooling/lsp/src/requests/code_action/remove_unused_import.rs diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index 49568d42038..299c42b85c6 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -330,9 +330,13 @@ impl Display for UseTree { match &self.kind { UseTreeKind::Path(name, alias) => { + if !(self.prefix.segments.is_empty() && self.prefix.kind == PathKind::Plain) { + write!(f, "::")?; + } + write!(f, "{name}")?; - while let Some(alias) = alias { + if let Some(alias) = alias { write!(f, " as {alias}")?; } diff --git a/compiler/noirc_frontend/src/ast/visitor.rs b/compiler/noirc_frontend/src/ast/visitor.rs index fb116d34e8a..80442d29398 100644 --- a/compiler/noirc_frontend/src/ast/visitor.rs +++ b/compiler/noirc_frontend/src/ast/visitor.rs @@ -271,7 +271,7 @@ pub trait Visitor { true } - fn visit_import(&mut self, _: &UseTree, _visibility: ItemVisibility) -> bool { + fn visit_import(&mut self, _: &UseTree, _: Span, _visibility: ItemVisibility) -> bool { true } @@ -506,7 +506,7 @@ impl Item { } ItemKind::Trait(noir_trait) => noir_trait.accept(self.span, visitor), ItemKind::Import(use_tree, visibility) => { - if visitor.visit_import(use_tree, *visibility) { + if visitor.visit_import(use_tree, self.span, *visibility) { use_tree.accept(visitor); } } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 6265d0e65f2..ab045c52169 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -499,7 +499,7 @@ impl DefCollector { crate_id: CrateId, errors: &mut Vec<(CompilationError, FileId)>, ) { - let unused_imports = context.def_interner.usage_tracker.unused_items().iter(); + let unused_imports = context.def_interner.unused_items().iter(); let unused_imports = unused_imports.filter(|(module_id, _)| module_id.krate == crate_id); errors.extend(unused_imports.flat_map(|(module_id, usage_tracker)| { diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 8c00fcde2ef..75178df319d 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -28,6 +28,7 @@ use crate::hir::type_check::generics::TraitGenerics; use crate::hir_def::traits::NamedType; use crate::macros_api::ModuleDefId; use crate::macros_api::UnaryOp; +use crate::usage_tracker::UnusedItem; use crate::usage_tracker::UsageTracker; use crate::QuotedType; @@ -2249,6 +2250,12 @@ impl NodeInterner { pub fn doc_comments(&self, id: ReferenceId) -> Option<&Vec> { self.doc_comments.get(&id) } + + pub fn unused_items( + &self, + ) -> &std::collections::HashMap> { + self.usage_tracker.unused_items() + } } impl Methods { diff --git a/tooling/lsp/src/requests/code_action.rs b/tooling/lsp/src/requests/code_action.rs index f5cfabe5141..64eccab8947 100644 --- a/tooling/lsp/src/requests/code_action.rs +++ b/tooling/lsp/src/requests/code_action.rs @@ -1,6 +1,7 @@ use std::{ collections::{BTreeMap, HashMap}, future::{self, Future}, + ops::Range, }; use async_lsp::ResponseError; @@ -11,7 +12,7 @@ use lsp_types::{ }; use noirc_errors::Span; use noirc_frontend::{ - ast::{ConstructorExpression, NoirTraitImpl, Path, Visitor}, + ast::{ConstructorExpression, ItemVisibility, NoirTraitImpl, Path, UseTree, Visitor}, graph::CrateId, hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}, macros_api::NodeInterner, @@ -28,7 +29,7 @@ use super::{process_request, to_lsp_location}; mod fill_struct_fields; mod implement_missing_members; mod import_or_qualify; -#[cfg(test)] +mod remove_unused_import; mod tests; pub(crate) fn on_code_action_request( @@ -43,7 +44,7 @@ pub(crate) fn on_code_action_request( let result = process_request(state, text_document_position_params, |args| { let path = PathString::from_path(uri.to_file_path().unwrap()); args.files.get_file_id(&path).and_then(|file_id| { - utils::position_to_byte_index(args.files, file_id, &position).and_then(|byte_index| { + utils::range_to_byte_span(args.files, file_id, ¶ms.range).and_then(|byte_range| { let file = args.files.get_file(file_id).unwrap(); let source = file.source(); let (parsed_module, _errors) = noirc_frontend::parse_program(source); @@ -53,7 +54,7 @@ pub(crate) fn on_code_action_request( args.files, file_id, source, - byte_index, + byte_range, args.crate_id, args.def_maps, args.interner, @@ -71,7 +72,7 @@ struct CodeActionFinder<'a> { file: FileId, source: &'a str, lines: Vec<&'a str>, - byte_index: usize, + byte_range: Range, /// The module ID in scope. This might change as we traverse the AST /// if we are analyzing something inside an inline module declaration. module_id: ModuleId, @@ -81,7 +82,9 @@ struct CodeActionFinder<'a> { nesting: usize, /// The line where an auto_import must be inserted auto_import_line: usize, - code_actions: Vec, + /// Text edits for the "Remove all unused imports" code action + unused_imports_text_edits: Vec, + code_actions: Vec, } impl<'a> CodeActionFinder<'a> { @@ -91,7 +94,7 @@ impl<'a> CodeActionFinder<'a> { files: &'a FileMap, file: FileId, source: &'a str, - byte_index: usize, + byte_range: Range, krate: CrateId, def_maps: &'a BTreeMap, interner: &'a NodeInterner, @@ -112,12 +115,13 @@ impl<'a> CodeActionFinder<'a> { file, source, lines: source.lines().collect(), - byte_index, + byte_range, module_id, def_maps, interner, nesting: 0, auto_import_line: 0, + unused_imports_text_edits: vec![], code_actions: vec![], } } @@ -129,20 +133,30 @@ impl<'a> CodeActionFinder<'a> { return None; } + // We also suggest a single "Remove all the unused imports" code action that combines all of the + // "Remove unused imports" (similar to Rust Analyzer) + if self.unused_imports_text_edits.len() > 1 { + let text_edits = std::mem::take(&mut self.unused_imports_text_edits); + let code_action = self.new_quick_fix_multiple_edits( + "Remove all the unused imports".to_string(), + text_edits, + ); + self.code_actions.push(code_action); + } + let mut code_actions = std::mem::take(&mut self.code_actions); - code_actions.sort_by_key(|code_action| { - let CodeActionOrCommand::CodeAction(code_action) = code_action else { - panic!("We only gather code actions, never commands"); - }; - code_action.title.clone() - }); - - Some(code_actions) + code_actions.sort_by_key(|code_action| code_action.title.clone()); + + Some(code_actions.into_iter().map(CodeActionOrCommand::CodeAction).collect()) } - fn new_quick_fix(&self, title: String, text_edit: TextEdit) -> CodeActionOrCommand { + fn new_quick_fix(&self, title: String, text_edit: TextEdit) -> CodeAction { + self.new_quick_fix_multiple_edits(title, vec![text_edit]) + } + + fn new_quick_fix_multiple_edits(&self, title: String, text_edits: Vec) -> CodeAction { let mut changes = HashMap::new(); - changes.insert(self.uri.clone(), vec![text_edit]); + changes.insert(self.uri.clone(), text_edits); let workspace_edit = WorkspaceEdit { changes: Some(changes), @@ -150,7 +164,7 @@ impl<'a> CodeActionFinder<'a> { change_annotations: None, }; - CodeActionOrCommand::CodeAction(CodeAction { + CodeAction { title, kind: Some(CodeActionKind::QUICKFIX), diagnostics: None, @@ -159,11 +173,12 @@ impl<'a> CodeActionFinder<'a> { is_preferred: None, disabled: None, data: None, - }) + } } fn includes_span(&self, span: Span) -> bool { - span.start() as usize <= self.byte_index && self.byte_index <= span.end() as usize + let byte_range_span = Span::from(self.byte_range.start as u32..self.byte_range.end as u32); + span.intersects(&byte_range_span) } } @@ -207,6 +222,12 @@ impl<'a> Visitor for CodeActionFinder<'a> { false } + fn visit_import(&mut self, use_tree: &UseTree, span: Span, visibility: ItemVisibility) -> bool { + self.remove_unused_import(use_tree, visibility, span); + + true + } + fn visit_path(&mut self, path: &Path) { self.import_or_qualify(path); } diff --git a/tooling/lsp/src/requests/code_action/remove_unused_import.rs b/tooling/lsp/src/requests/code_action/remove_unused_import.rs new file mode 100644 index 00000000000..c660dd57e47 --- /dev/null +++ b/tooling/lsp/src/requests/code_action/remove_unused_import.rs @@ -0,0 +1,314 @@ +use std::collections::HashMap; + +use lsp_types::TextEdit; +use noirc_errors::Span; +use noirc_frontend::{ + ast::{Ident, ItemVisibility, UseTree, UseTreeKind}, + parser::{Item, ItemKind}, + usage_tracker::UnusedItem, + ParsedModule, +}; + +use crate::byte_span_to_range; + +use super::CodeActionFinder; + +impl<'a> CodeActionFinder<'a> { + pub(super) fn remove_unused_import( + &mut self, + use_tree: &UseTree, + visibility: ItemVisibility, + span: Span, + ) { + if !self.includes_span(span) { + return; + } + + let Some(unused_items) = self.interner.unused_items().get(&self.module_id) else { + return; + }; + + if unused_items.is_empty() { + return; + } + + if has_unused_import(use_tree, unused_items) { + let byte_span = span.start() as usize..span.end() as usize; + let Some(range) = byte_span_to_range(self.files, self.file, byte_span) else { + return; + }; + + let (use_tree, removed_count) = use_tree_without_unused_import(use_tree, unused_items); + let (title, new_text) = match use_tree { + Some(use_tree) => ( + if removed_count == 1 { + "Remove unused import".to_string() + } else { + "Remove unused imports".to_string() + }, + use_tree_to_string(use_tree, visibility, self.nesting), + ), + None => ("Remove the whole `use` item".to_string(), "".to_string()), + }; + + let text_edit = TextEdit { range, new_text }; + self.unused_imports_text_edits.push(text_edit.clone()); + + let code_action = self.new_quick_fix(title, text_edit); + self.code_actions.push(code_action); + } + } +} + +fn has_unused_import(use_tree: &UseTree, unused_items: &HashMap) -> bool { + match &use_tree.kind { + UseTreeKind::Path(name, alias) => { + let ident = alias.as_ref().unwrap_or(name); + unused_items.contains_key(ident) + } + UseTreeKind::List(use_trees) => { + use_trees.iter().any(|use_tree| has_unused_import(use_tree, unused_items)) + } + } +} + +/// Returns a new `UseTree` with all the unused imports removed, and the number of removed imports. +fn use_tree_without_unused_import( + use_tree: &UseTree, + unused_items: &HashMap, +) -> (Option, usize) { + match &use_tree.kind { + UseTreeKind::Path(name, alias) => { + let ident = alias.as_ref().unwrap_or(name); + if unused_items.contains_key(ident) { + (None, 1) + } else { + (Some(use_tree.clone()), 0) + } + } + UseTreeKind::List(use_trees) => { + let mut new_use_trees: Vec = Vec::new(); + let mut total_count = 0; + + for use_tree in use_trees { + let (new_use_tree, count) = use_tree_without_unused_import(use_tree, unused_items); + if let Some(new_use_tree) = new_use_tree { + new_use_trees.push(new_use_tree); + } + total_count += count; + } + + let new_use_tree = if new_use_trees.is_empty() { + None + } else if new_use_trees.len() == 1 { + let new_use_tree = new_use_trees.remove(0); + + let mut prefix = use_tree.prefix.clone(); + prefix.segments.extend(new_use_tree.prefix.segments); + + Some(UseTree { prefix, kind: new_use_tree.kind }) + } else { + Some(UseTree { + prefix: use_tree.prefix.clone(), + kind: UseTreeKind::List(new_use_trees), + }) + }; + + (new_use_tree, total_count) + } + } +} + +fn use_tree_to_string(use_tree: UseTree, visibility: ItemVisibility, nesting: usize) -> String { + // We are going to use the formatter to format the use tree + let source = if visibility == ItemVisibility::Private { + format!("use {};", &use_tree) + } else { + format!("{} use {};", visibility, &use_tree) + }; + let parsed_module = ParsedModule { + items: vec![Item { + kind: ItemKind::Import(use_tree, visibility), + span: Span::from(0..source.len() as u32), + doc_comments: Vec::new(), + }], + inner_doc_comments: Vec::new(), + }; + + // Adjust the max width according to the current nesting + let mut config = nargo_fmt::Config::default(); + config.max_width -= nesting * 4; + + let string = nargo_fmt::format(&source, parsed_module, &config); + + let string = if nesting > 0 && string.contains('\n') { + // If the import is nested in a module, we just formatted it without indents so we need to add them. + let indent = " ".repeat(nesting * 4); + string.lines().map(|line| format!("{}{}", indent, line)).collect::>().join("\n") + } else { + string + }; + string.trim().to_string() +} + +#[cfg(test)] +mod tests { + use tokio::test; + + use crate::requests::code_action::tests::assert_code_action; + + #[test] + async fn test_removes_entire_unused_import_at_top_level() { + let title = "Remove the whole `use` item"; + + let src = r#" + mod moo { + fn foo() {} + } + use moo::fo>||||| Visitor for NodeFinder<'a> { self.includes_span(item.span) } - fn visit_import(&mut self, use_tree: &UseTree, _visibility: ItemVisibility) -> bool { + fn visit_import( + &mut self, + use_tree: &UseTree, + _span: Span, + _visibility: ItemVisibility, + ) -> bool { let mut prefixes = Vec::new(); self.find_in_use_tree(use_tree, &mut prefixes); false