From f41d8bc98958da8dcd3883a9cf58fc34283418ce Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 25 Oct 2024 15:31:20 -0300 Subject: [PATCH 1/2] Extract UseSegmentPositions type --- tooling/lsp/src/lib.rs | 1 + tooling/lsp/src/requests/completion.rs | 163 +------------- .../src/requests/completion/auto_import.rs | 33 +-- tooling/lsp/src/use_segment_positions.rs | 198 ++++++++++++++++++ 4 files changed, 206 insertions(+), 189 deletions(-) create mode 100644 tooling/lsp/src/use_segment_positions.rs diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 7254e3a5f77..a85b9d043b9 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -70,6 +70,7 @@ mod solver; mod tests; mod trait_impl_method_stub_generator; mod types; +mod use_segment_positions; mod utils; mod visibility; diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index d0e0efd97bc..b69b261c9c6 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -38,7 +38,7 @@ use sort_text::underscore_sort_text; use crate::{ requests::to_lsp_location, trait_impl_method_stub_generator::TraitImplMethodStubGenerator, - utils, visibility::is_visible, LspState, + use_segment_positions::UseSegmentPositions, utils, visibility::is_visible, LspState, }; use super::process_request; @@ -88,42 +88,6 @@ pub(crate) fn on_completion_request( future::ready(result) } -/// The position of a segment in a `use` statement. -/// We use this to determine how an auto-import should be inserted. -#[derive(Debug, Default, Copy, Clone)] -enum UseSegmentPosition { - /// The segment either doesn't exist in the source code or there are multiple segments. - /// In this case auto-import will add a new use statement. - #[default] - NoneOrMultiple, - /// The segment is the last one in the `use` statement (or nested use statement): - /// - /// use foo::bar; - /// ^^^ - /// - /// Auto-import will transform it to this: - /// - /// use foo::bar::{self, baz}; - Last { span: Span }, - /// The segment happens before another simple (ident) segment: - /// - /// use foo::bar::qux; - /// ^^^ - /// - /// Auto-import will transform it to this: - /// - /// use foo::bar::{qux, baz}; - BeforeSegment { segment_span_until_end: Span }, - /// The segment happens before a list: - /// - /// use foo::bar::{qux, another}; - /// - /// Auto-import will transform it to this: - /// - /// use foo::bar::{qux, another, baz}; - BeforeList { first_entry_span: Span, list_is_empty: bool }, -} - struct NodeFinder<'a> { files: &'a FileMap, file: FileId, @@ -151,11 +115,7 @@ struct NodeFinder<'a> { nesting: usize, /// The line where an auto_import must be inserted auto_import_line: usize, - /// Remember where each segment in a `use` statement is located. - /// The key is the full segment, so for `use foo::bar::baz` we'll have three - /// segments: `foo`, `foo::bar` and `foo::bar::baz`, where the span is just - /// for the last identifier (`foo`, `bar` and `baz` in the previous example). - use_segment_positions: HashMap, + use_segment_positions: UseSegmentPositions, self_type: Option, in_comptime: bool, } @@ -200,7 +160,7 @@ impl<'a> NodeFinder<'a> { suggested_module_def_ids: HashSet::new(), nesting: 0, auto_import_line: 0, - use_segment_positions: HashMap::new(), + use_segment_positions: UseSegmentPositions::default(), self_type: None, in_comptime: false, } @@ -1078,121 +1038,6 @@ impl<'a> NodeFinder<'a> { } /// Determine where each segment in a `use` statement is located. - fn gather_use_tree_segments(&mut self, use_tree: &UseTree, mut prefix: String) { - let kind_string = match use_tree.prefix.kind { - PathKind::Crate => Some("crate".to_string()), - PathKind::Super => Some("super".to_string()), - PathKind::Dep | PathKind::Plain => None, - }; - if let Some(kind_string) = kind_string { - if let Some(segment) = use_tree.prefix.segments.first() { - self.insert_use_segment_position( - kind_string, - UseSegmentPosition::BeforeSegment { - segment_span_until_end: Span::from( - segment.ident.span().start()..use_tree.span.end() - 1, - ), - }, - ); - } else { - self.insert_use_segment_position_before_use_tree_kind(use_tree, kind_string); - } - } - - let prefix_segments_len = use_tree.prefix.segments.len(); - for (index, segment) in use_tree.prefix.segments.iter().enumerate() { - let ident = &segment.ident; - if !prefix.is_empty() { - prefix.push_str("::"); - }; - prefix.push_str(&ident.0.contents); - - if index < prefix_segments_len - 1 { - self.insert_use_segment_position( - prefix.clone(), - UseSegmentPosition::BeforeSegment { - segment_span_until_end: Span::from( - use_tree.prefix.segments[index + 1].ident.span().start() - ..use_tree.span.end() - 1, - ), - }, - ); - } else { - self.insert_use_segment_position_before_use_tree_kind(use_tree, prefix.clone()); - } - } - - match &use_tree.kind { - UseTreeKind::Path(ident, alias) => { - if !prefix.is_empty() { - prefix.push_str("::"); - } - prefix.push_str(&ident.0.contents); - - if alias.is_none() { - self.insert_use_segment_position( - prefix, - UseSegmentPosition::Last { span: ident.span() }, - ); - } else { - self.insert_use_segment_position(prefix, UseSegmentPosition::NoneOrMultiple); - } - } - UseTreeKind::List(use_trees) => { - for use_tree in use_trees { - self.gather_use_tree_segments(use_tree, prefix.clone()); - } - } - } - } - - fn insert_use_segment_position_before_use_tree_kind( - &mut self, - use_tree: &UseTree, - prefix: String, - ) { - match &use_tree.kind { - UseTreeKind::Path(ident, _alias) => { - self.insert_use_segment_position( - prefix, - UseSegmentPosition::BeforeSegment { - segment_span_until_end: Span::from( - ident.span().start()..use_tree.span.end() - 1, - ), - }, - ); - } - UseTreeKind::List(use_trees) => { - if let Some(first_use_tree) = use_trees.first() { - self.insert_use_segment_position( - prefix, - UseSegmentPosition::BeforeList { - first_entry_span: first_use_tree.prefix.span(), - list_is_empty: false, - }, - ); - } else { - self.insert_use_segment_position( - prefix, - UseSegmentPosition::BeforeList { - first_entry_span: Span::from( - use_tree.span.end() - 1..use_tree.span.end() - 1, - ), - list_is_empty: true, - }, - ); - } - } - } - } - - fn insert_use_segment_position(&mut self, segment: String, position: UseSegmentPosition) { - if self.use_segment_positions.get(&segment).is_none() { - self.use_segment_positions.insert(segment, position); - } else { - self.use_segment_positions.insert(segment, UseSegmentPosition::NoneOrMultiple); - } - } fn includes_span(&self, span: Span) -> bool { span.start() as usize <= self.byte_index && self.byte_index <= span.end() as usize @@ -1205,7 +1050,7 @@ impl<'a> Visitor for NodeFinder<'a> { if let Some(lsp_location) = to_lsp_location(self.files, self.file, item.span) { self.auto_import_line = (lsp_location.range.end.line + 1) as usize; } - self.gather_use_tree_segments(use_tree, String::new()); + self.use_segment_positions.add(use_tree); } self.includes_span(item.span) diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index 7a9f51cca74..6d44b2d3797 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -6,13 +6,14 @@ use noirc_frontend::hir::def_map::ModuleDefId; use crate::{ modules::{relative_module_full_path, relative_module_id_path}, requests::to_lsp_location, + use_segment_positions::UseSegmentPosition, }; use super::{ kinds::{FunctionCompletionKind, FunctionKind, RequestedItems}, name_matches, sort_text::auto_import_sort_text, - NodeFinder, UseSegmentPosition, + NodeFinder, }; impl<'a> NodeFinder<'a> { @@ -83,8 +84,7 @@ impl<'a> NodeFinder<'a> { completion_item.label_details = Some(label_details); // See if there's a single place where one of the parent paths is located - let (use_segment_position, name) = - self.find_use_segment_position(full_path.clone()); + let (use_segment_position, name) = self.use_segment_positions.get(&full_path); match use_segment_position { UseSegmentPosition::NoneOrMultiple => { // The parent path either isn't in any use statement, or it exists in multiple @@ -216,31 +216,4 @@ impl<'a> NodeFinder<'a> { new_text: format!("use {};{}{}", full_path, newlines, indent), }] } - - /// Given a full path like `foo::bar::baz`, returns the first non-"NoneOrMultiple" segment position - /// trying each successive parent, together with the name after the parent. - /// - /// For example, first we'll check if `foo::bar` has a single position. If not, we'll try with `foo`. - fn find_use_segment_position(&self, full_path: String) -> (UseSegmentPosition, String) { - // Build a parent path to know in which full segment we need to add this import - let mut segments: Vec<_> = full_path.split("::").collect(); - let mut name = segments.pop().unwrap().to_string(); - let mut parent_path = segments.join("::"); - - loop { - let use_segment_position = - self.use_segment_positions.get(&parent_path).cloned().unwrap_or_default(); - - if let UseSegmentPosition::NoneOrMultiple = use_segment_position { - if let Some(next_name) = segments.pop() { - name = format!("{next_name}::{name}"); - parent_path = segments.join("::"); - } else { - return (UseSegmentPosition::NoneOrMultiple, String::new()); - } - } else { - return (use_segment_position, name); - } - } - } } diff --git a/tooling/lsp/src/use_segment_positions.rs b/tooling/lsp/src/use_segment_positions.rs new file mode 100644 index 00000000000..5f9ddc2cd95 --- /dev/null +++ b/tooling/lsp/src/use_segment_positions.rs @@ -0,0 +1,198 @@ +use std::collections::HashMap; + +use noirc_errors::Span; +use noirc_frontend::ast::{PathKind, UseTree, UseTreeKind}; + +/// The position of a segment in a `use` statement. +/// We use this to determine how an auto-import should be inserted. +#[derive(Debug, Default, Copy, Clone)] +pub(crate) enum UseSegmentPosition { + /// The segment either doesn't exist in the source code or there are multiple segments. + /// In this case auto-import will add a new use statement. + #[default] + NoneOrMultiple, + /// The segment is the last one in the `use` statement (or nested use statement): + /// + /// use foo::bar; + /// ^^^ + /// + /// Auto-import will transform it to this: + /// + /// use foo::bar::{self, baz}; + Last { span: Span }, + /// The segment happens before another simple (ident) segment: + /// + /// use foo::bar::qux; + /// ^^^ + /// + /// Auto-import will transform it to this: + /// + /// use foo::bar::{qux, baz}; + BeforeSegment { segment_span_until_end: Span }, + /// The segment happens before a list: + /// + /// use foo::bar::{qux, another}; + /// + /// Auto-import will transform it to this: + /// + /// use foo::bar::{qux, another, baz}; + BeforeList { first_entry_span: Span, list_is_empty: bool }, +} + +/// Remembers where each segment in a `use` statement is located. +/// The key is the full segment, so for `use foo::bar::baz` we'll have three +/// segments: `foo`, `foo::bar` and `foo::bar::baz`, where the span is just +/// for the last identifier (`foo`, `bar` and `baz` in the previous example). +#[derive(Default)] +pub(crate) struct UseSegmentPositions { + use_segment_positions: HashMap, +} + +impl UseSegmentPositions { + pub(crate) fn add(&mut self, use_tree: &UseTree) { + self.gather_use_tree_segments(use_tree, String::new()); + } + + /// Given a full path like `foo::bar::baz`, returns the first non-"NoneOrMultiple" segment position + /// trying each successive parent, together with the name after the parent. + /// + /// For example, first we'll check if `foo::bar` has a single position. If not, we'll try with `foo`. + pub(crate) fn get(&self, full_path: &str) -> (UseSegmentPosition, String) { + // Build a parent path to know in which full segment we need to add this import + let mut segments: Vec<_> = full_path.split("::").collect(); + let mut name = segments.pop().unwrap().to_string(); + let mut parent_path = segments.join("::"); + + loop { + let use_segment_position = + self.use_segment_positions.get(&parent_path).cloned().unwrap_or_default(); + + if let UseSegmentPosition::NoneOrMultiple = use_segment_position { + if let Some(next_name) = segments.pop() { + name = format!("{next_name}::{name}"); + parent_path = segments.join("::"); + } else { + return (UseSegmentPosition::NoneOrMultiple, String::new()); + } + } else { + return (use_segment_position, name); + } + } + } + + fn gather_use_tree_segments(&mut self, use_tree: &UseTree, mut prefix: String) { + let kind_string = match use_tree.prefix.kind { + PathKind::Crate => Some("crate".to_string()), + PathKind::Super => Some("super".to_string()), + PathKind::Dep | PathKind::Plain => None, + }; + if let Some(kind_string) = kind_string { + if let Some(segment) = use_tree.prefix.segments.first() { + self.insert_use_segment_position( + kind_string, + UseSegmentPosition::BeforeSegment { + segment_span_until_end: Span::from( + segment.ident.span().start()..use_tree.span.end() - 1, + ), + }, + ); + } else { + self.insert_use_segment_position_before_use_tree_kind(use_tree, kind_string); + } + } + + let prefix_segments_len = use_tree.prefix.segments.len(); + for (index, segment) in use_tree.prefix.segments.iter().enumerate() { + let ident = &segment.ident; + if !prefix.is_empty() { + prefix.push_str("::"); + }; + prefix.push_str(&ident.0.contents); + + if index < prefix_segments_len - 1 { + self.insert_use_segment_position( + prefix.clone(), + UseSegmentPosition::BeforeSegment { + segment_span_until_end: Span::from( + use_tree.prefix.segments[index + 1].ident.span().start() + ..use_tree.span.end() - 1, + ), + }, + ); + } else { + self.insert_use_segment_position_before_use_tree_kind(use_tree, prefix.clone()); + } + } + + match &use_tree.kind { + UseTreeKind::Path(ident, alias) => { + if !prefix.is_empty() { + prefix.push_str("::"); + } + prefix.push_str(&ident.0.contents); + + if alias.is_none() { + self.insert_use_segment_position( + prefix, + UseSegmentPosition::Last { span: ident.span() }, + ); + } else { + self.insert_use_segment_position(prefix, UseSegmentPosition::NoneOrMultiple); + } + } + UseTreeKind::List(use_trees) => { + for use_tree in use_trees { + self.gather_use_tree_segments(use_tree, prefix.clone()); + } + } + } + } + + fn insert_use_segment_position_before_use_tree_kind( + &mut self, + use_tree: &UseTree, + prefix: String, + ) { + match &use_tree.kind { + UseTreeKind::Path(ident, _alias) => { + self.insert_use_segment_position( + prefix, + UseSegmentPosition::BeforeSegment { + segment_span_until_end: Span::from( + ident.span().start()..use_tree.span.end() - 1, + ), + }, + ); + } + UseTreeKind::List(use_trees) => { + if let Some(first_use_tree) = use_trees.first() { + self.insert_use_segment_position( + prefix, + UseSegmentPosition::BeforeList { + first_entry_span: first_use_tree.prefix.span(), + list_is_empty: false, + }, + ); + } else { + self.insert_use_segment_position( + prefix, + UseSegmentPosition::BeforeList { + first_entry_span: Span::from( + use_tree.span.end() - 1..use_tree.span.end() - 1, + ), + list_is_empty: true, + }, + ); + } + } + } + } + + fn insert_use_segment_position(&mut self, segment: String, position: UseSegmentPosition) { + if self.use_segment_positions.get(&segment).is_none() { + self.use_segment_positions.insert(segment, position); + } else { + self.use_segment_positions.insert(segment, UseSegmentPosition::NoneOrMultiple); + } + } +} From 1938c4feea2f1ec76a4df2e315f4a701da1468bb Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 25 Oct 2024 15:51:00 -0300 Subject: [PATCH 2/2] Let the import code action insert into existing use statements if possible --- tooling/lsp/src/requests/code_action.rs | 7 +- .../requests/code_action/import_or_qualify.rs | 62 +++++--- tooling/lsp/src/requests/code_action/tests.rs | 5 +- .../src/requests/completion/auto_import.rs | 147 ++---------------- tooling/lsp/src/requests/completion/tests.rs | 24 +-- tooling/lsp/src/requests/mod.rs | 2 +- tooling/lsp/src/tests.rs | 5 +- tooling/lsp/src/use_segment_positions.rs | 138 ++++++++++++++++ 8 files changed, 220 insertions(+), 170 deletions(-) diff --git a/tooling/lsp/src/requests/code_action.rs b/tooling/lsp/src/requests/code_action.rs index 9299dc76368..f3e9130e17d 100644 --- a/tooling/lsp/src/requests/code_action.rs +++ b/tooling/lsp/src/requests/code_action.rs @@ -22,7 +22,7 @@ use noirc_frontend::{ ParsedModule, }; -use crate::{utils, LspState}; +use crate::{use_segment_positions::UseSegmentPositions, utils, LspState}; use super::{process_request, to_lsp_location}; @@ -82,6 +82,7 @@ struct CodeActionFinder<'a> { nesting: usize, /// The line where an auto_import must be inserted auto_import_line: usize, + use_segment_positions: UseSegmentPositions, /// Text edits for the "Remove all unused imports" code action unused_imports_text_edits: Vec, code_actions: Vec, @@ -121,6 +122,7 @@ impl<'a> CodeActionFinder<'a> { interner, nesting: 0, auto_import_line: 0, + use_segment_positions: UseSegmentPositions::default(), unused_imports_text_edits: vec![], code_actions: vec![], } @@ -184,10 +186,11 @@ impl<'a> CodeActionFinder<'a> { impl<'a> Visitor for CodeActionFinder<'a> { fn visit_item(&mut self, item: &Item) -> bool { - if let ItemKind::Import(..) = &item.kind { + if let ItemKind::Import(use_tree, _) = &item.kind { if let Some(lsp_location) = to_lsp_location(self.files, self.file, item.span) { self.auto_import_line = (lsp_location.range.end.line + 1) as usize; } + self.use_segment_positions.add(use_tree); } self.includes_span(item.span) diff --git a/tooling/lsp/src/requests/code_action/import_or_qualify.rs b/tooling/lsp/src/requests/code_action/import_or_qualify.rs index bf1fe906be3..a3053f8f304 100644 --- a/tooling/lsp/src/requests/code_action/import_or_qualify.rs +++ b/tooling/lsp/src/requests/code_action/import_or_qualify.rs @@ -1,4 +1,4 @@ -use lsp_types::{Position, Range, TextEdit}; +use lsp_types::TextEdit; use noirc_errors::Location; use noirc_frontend::{ ast::{Ident, Path}, @@ -8,6 +8,9 @@ use noirc_frontend::{ use crate::{ byte_span_to_range, modules::{relative_module_full_path, relative_module_id_path}, + use_segment_positions::{ + use_completion_item_additional_text_edits, UseCompletionItemAdditionTextEditsRequest, + }, }; use super::CodeActionFinder; @@ -82,25 +85,21 @@ impl<'a> CodeActionFinder<'a> { } fn push_import_code_action(&mut self, full_path: &str) { - let line = self.auto_import_line as u32; - let character = (self.nesting * 4) as u32; - let indent = " ".repeat(self.nesting * 4); - let mut newlines = "\n"; - - // If the line we are inserting into is not an empty line, insert an extra line to make some room - if let Some(line_text) = self.lines.get(line as usize) { - if !line_text.trim().is_empty() { - newlines = "\n\n"; - } - } - let title = format!("Import {}", full_path); - let text_edit = TextEdit { - range: Range { start: Position { line, character }, end: Position { line, character } }, - new_text: format!("use {};{}{}", full_path, newlines, indent), - }; - let code_action = self.new_quick_fix(title, text_edit); + let text_edits = use_completion_item_additional_text_edits( + UseCompletionItemAdditionTextEditsRequest { + full_path, + files: self.files, + file: self.file, + lines: &self.lines, + nesting: self.nesting, + auto_import_line: self.auto_import_line, + }, + &self.use_segment_positions, + ); + + let code_action = self.new_quick_fix_multiple_edits(title, text_edits); self.code_actions.push(code_action); } @@ -238,4 +237,31 @@ fn main() { assert_code_action(title, src, expected).await; } + + #[test] + async fn test_import_code_action_for_struct_inserts_into_existing_use() { + let title = "Import foo::bar::SomeTypeInBar"; + + let src = r#"use foo::bar::SomeOtherType; + +mod foo { + mod bar { + pub struct SomeTypeInBar {} + } +} + +fn foo(x: SomeType>||<", ""), &text_edits[0]); + let result = apply_text_edits(&src.replace(">|<", ""), text_edits); if result != expected { println!("Expected:\n```\n{}\n```\n\nGot:\n```\n{}\n```", expected, result); assert_eq!(result, expected); diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index 6d44b2d3797..9ed633289c1 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -1,12 +1,10 @@ -use lsp_types::{Position, Range, TextEdit}; - -use noirc_errors::Span; use noirc_frontend::hir::def_map::ModuleDefId; use crate::{ modules::{relative_module_full_path, relative_module_id_path}, - requests::to_lsp_location, - use_segment_positions::UseSegmentPosition, + use_segment_positions::{ + use_completion_item_additional_text_edits, UseCompletionItemAdditionTextEditsRequest, + }, }; use super::{ @@ -82,114 +80,18 @@ impl<'a> NodeFinder<'a> { let mut label_details = completion_item.label_details.unwrap(); label_details.detail = Some(format!("(use {})", full_path)); completion_item.label_details = Some(label_details); - - // See if there's a single place where one of the parent paths is located - let (use_segment_position, name) = self.use_segment_positions.get(&full_path); - match use_segment_position { - UseSegmentPosition::NoneOrMultiple => { - // The parent path either isn't in any use statement, or it exists in multiple - // use statements. In either case we'll add a new use statement. - completion_item.additional_text_edits = - Some(self.new_use_completion_item_additional_text_edits(full_path)); - } - UseSegmentPosition::Last { span } => { - // We have - // - // use foo::bar; - // ^^^ -> span - // - // and we want to transform it to: - // - // use foo::bar::{self, baz}; - // ^^^^^^^^^^^^^ - // - // So we need one text edit: - // 1. insert "::{self, baz}" right after the span - if let Some(lsp_location) = to_lsp_location(self.files, self.file, span) - { - let range = lsp_location.range; - completion_item.additional_text_edits = Some(vec![TextEdit { - new_text: format!("::{{self, {}}}", name), - range: Range { start: range.end, end: range.end }, - }]); - } else { - completion_item.additional_text_edits = Some( - self.new_use_completion_item_additional_text_edits(full_path), - ); - } - } - UseSegmentPosition::BeforeSegment { segment_span_until_end } => { - // Go past the end - let segment_span_until_end = Span::from( - segment_span_until_end.start()..segment_span_until_end.end() + 1, - ); - - // We have - // - // use foo::bar::{one, two}; - // ^^^^^^^^^^^^^^^ -> segment_span_until_end - // - // and we want to transform it to: - // - // use foo::{bar::{one, two}, baz}; - // ^ ^^^^^^ - // - // So we need two text edits: - // 1. insert "{" right before the segment span - // 2. insert ", baz}" right after the segment span - if let Some(lsp_location) = - to_lsp_location(self.files, self.file, segment_span_until_end) - { - let range = lsp_location.range; - completion_item.additional_text_edits = Some(vec![ - TextEdit { - new_text: "{".to_string(), - range: Range { start: range.start, end: range.start }, - }, - TextEdit { - new_text: format!(", {}}}", name), - range: Range { start: range.end, end: range.end }, - }, - ]); - } else { - completion_item.additional_text_edits = Some( - self.new_use_completion_item_additional_text_edits(full_path), - ); - } - } - UseSegmentPosition::BeforeList { first_entry_span, list_is_empty } => { - // We have - // - // use foo::bar::{one, two}; - // ^^^ -> first_entry_span - // - // and we want to transform it to: - // - // use foo::bar::{baz, one, two}; - // ^^^^ - // - // So we need one text edit: - // 1. insert "baz, " right before the first entry span - if let Some(lsp_location) = - to_lsp_location(self.files, self.file, first_entry_span) - { - let range = lsp_location.range; - completion_item.additional_text_edits = Some(vec![TextEdit { - new_text: if list_is_empty { - name - } else { - format!("{}, ", name) - }, - range: Range { start: range.start, end: range.start }, - }]); - } else { - completion_item.additional_text_edits = Some( - self.new_use_completion_item_additional_text_edits(full_path), - ); - } - } - } - + completion_item.additional_text_edits = + Some(use_completion_item_additional_text_edits( + UseCompletionItemAdditionTextEditsRequest { + full_path: &full_path, + files: self.files, + file: self.file, + lines: &self.lines, + nesting: self.nesting, + auto_import_line: self.auto_import_line, + }, + &self.use_segment_positions, + )); completion_item.sort_text = Some(auto_import_sort_text()); self.completion_items.push(completion_item); @@ -197,23 +99,4 @@ impl<'a> NodeFinder<'a> { } } } - - fn new_use_completion_item_additional_text_edits(&self, full_path: String) -> Vec { - let line = self.auto_import_line as u32; - let character = (self.nesting * 4) as u32; - let indent = " ".repeat(self.nesting * 4); - let mut newlines = "\n"; - - // If the line we are inserting into is not an empty line, insert an extra line to make some room - if let Some(line_text) = self.lines.get(line as usize) { - if !line_text.trim().is_empty() { - newlines = "\n\n"; - } - } - - vec![TextEdit { - range: Range { start: Position { line, character }, end: Position { line, character } }, - new_text: format!("use {};{}{}", full_path, newlines, indent), - }] - } } diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index 322dbf932b5..be3a75f72c8 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -1447,7 +1447,7 @@ fn main() { ); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); assert_eq!(item.sort_text, Some(auto_import_sort_text())); } @@ -1493,7 +1493,7 @@ mod foo { ); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); } @@ -1538,7 +1538,7 @@ mod foo { ); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); } @@ -1582,7 +1582,7 @@ fn main() { let item = items.remove(0); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); } @@ -1666,7 +1666,7 @@ mod foo { ); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); } @@ -1703,7 +1703,7 @@ fn main() { let item = items.remove(0); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); assert_eq!(item.sort_text, Some(auto_import_sort_text())); } @@ -1741,7 +1741,7 @@ fn main() { let item = items.remove(0); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); assert_eq!(item.sort_text, Some(auto_import_sort_text())); } @@ -1781,7 +1781,7 @@ fn main() { let item = items.remove(0); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); assert_eq!(item.sort_text, Some(auto_import_sort_text())); } @@ -1827,7 +1827,7 @@ fn main() { let item = items.remove(0); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); assert_eq!(item.sort_text, Some(auto_import_sort_text())); } @@ -1865,7 +1865,7 @@ fn main() { let item = items.remove(0); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); assert_eq!(item.sort_text, Some(auto_import_sort_text())); } @@ -1905,7 +1905,7 @@ fn main() { let item = items.remove(0); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); assert_eq!(item.sort_text, Some(auto_import_sort_text())); } @@ -1941,7 +1941,7 @@ fn main() { let item = items.remove(0); let changed = - apply_text_edits(&src.replace(">|<", ""), &mut item.additional_text_edits.unwrap()); + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); assert_eq!(changed, expected); assert_eq!(item.sort_text, Some(auto_import_sort_text())); } diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 597d8355468..0ee66f1f618 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -378,7 +378,7 @@ fn character_to_line_offset(line: &str, character: u32) -> Result } } -fn to_lsp_location<'a, F>( +pub(crate) fn to_lsp_location<'a, F>( files: &'a F, file_id: F::FileId, definition_span: noirc_errors::Span, diff --git a/tooling/lsp/src/tests.rs b/tooling/lsp/src/tests.rs index 2788f848262..7f2d48cd23f 100644 --- a/tooling/lsp/src/tests.rs +++ b/tooling/lsp/src/tests.rs @@ -15,17 +15,18 @@ pub(crate) fn apply_text_edit(src: &str, text_edit: &TextEdit) -> String { lines.join("\n") } -pub(crate) fn apply_text_edits(src: &str, text_edits: &mut [TextEdit]) -> String { +pub(crate) fn apply_text_edits(src: &str, text_edits: &[TextEdit]) -> String { let mut text = src.to_string(); // Text edits must be applied from last to first, otherwise if we apply a text edit // that comes before another one, that other one becomes invalid (it will edit the wrong // piece of code). + let mut text_edits = text_edits.to_vec(); text_edits.sort_by_key(|edit| (edit.range.start.line, edit.range.start.character)); text_edits.reverse(); for text_edit in text_edits { - text = apply_text_edit(&text, text_edit); + text = apply_text_edit(&text, &text_edit); } text diff --git a/tooling/lsp/src/use_segment_positions.rs b/tooling/lsp/src/use_segment_positions.rs index 5f9ddc2cd95..f9a3f429029 100644 --- a/tooling/lsp/src/use_segment_positions.rs +++ b/tooling/lsp/src/use_segment_positions.rs @@ -1,8 +1,12 @@ use std::collections::HashMap; +use fm::{FileId, FileMap}; +use lsp_types::{Position, Range, TextEdit}; use noirc_errors::Span; use noirc_frontend::ast::{PathKind, UseTree, UseTreeKind}; +use crate::requests::to_lsp_location; + /// The position of a segment in a `use` statement. /// We use this to determine how an auto-import should be inserted. #[derive(Debug, Default, Copy, Clone)] @@ -196,3 +200,137 @@ impl UseSegmentPositions { } } } + +pub(crate) struct UseCompletionItemAdditionTextEditsRequest<'a> { + /// The full path of the use statement to insert + pub(crate) full_path: &'a str, + pub(crate) files: &'a FileMap, + pub(crate) file: FileId, + /// All of the current source lines + pub(crate) lines: &'a Vec<&'a str>, + /// How many nested `mod` we are in deep + pub(crate) nesting: usize, + /// The line where an auto_import must be inserted + pub(crate) auto_import_line: usize, +} + +/// Returns the text edits needed to add an auto-import for a given full path. +pub(crate) fn use_completion_item_additional_text_edits( + request: UseCompletionItemAdditionTextEditsRequest, + positions: &UseSegmentPositions, +) -> Vec { + let (use_segment_position, name) = positions.get(request.full_path); + match use_segment_position { + UseSegmentPosition::NoneOrMultiple => { + // The parent path either isn't in any use statement, or it exists in multiple + // use statements. In either case we'll add a new use statement. + + new_use_completion_item_additional_text_edits(request) + } + UseSegmentPosition::Last { span } => { + // We have + // + // use foo::bar; + // ^^^ -> span + // + // and we want to transform it to: + // + // use foo::bar::{self, baz}; + // ^^^^^^^^^^^^^ + // + // So we need one text edit: + // 1. insert "::{self, baz}" right after the span + if let Some(lsp_location) = to_lsp_location(request.files, request.file, span) { + let range = lsp_location.range; + vec![TextEdit { + new_text: format!("::{{self, {}}}", name), + range: Range { start: range.end, end: range.end }, + }] + } else { + new_use_completion_item_additional_text_edits(request) + } + } + UseSegmentPosition::BeforeSegment { segment_span_until_end } => { + // Go past the end + let segment_span_until_end = + Span::from(segment_span_until_end.start()..segment_span_until_end.end() + 1); + + // We have + // + // use foo::bar::{one, two}; + // ^^^^^^^^^^^^^^^ -> segment_span_until_end + // + // and we want to transform it to: + // + // use foo::{bar::{one, two}, baz}; + // ^ ^^^^^^ + // + // So we need two text edits: + // 1. insert "{" right before the segment span + // 2. insert ", baz}" right after the segment span + if let Some(lsp_location) = + to_lsp_location(request.files, request.file, segment_span_until_end) + { + let range = lsp_location.range; + vec![ + TextEdit { + new_text: "{".to_string(), + range: Range { start: range.start, end: range.start }, + }, + TextEdit { + new_text: format!(", {}}}", name), + range: Range { start: range.end, end: range.end }, + }, + ] + } else { + new_use_completion_item_additional_text_edits(request) + } + } + UseSegmentPosition::BeforeList { first_entry_span, list_is_empty } => { + // We have + // + // use foo::bar::{one, two}; + // ^^^ -> first_entry_span + // + // and we want to transform it to: + // + // use foo::bar::{baz, one, two}; + // ^^^^ + // + // So we need one text edit: + // 1. insert "baz, " right before the first entry span + if let Some(lsp_location) = + to_lsp_location(request.files, request.file, first_entry_span) + { + let range = lsp_location.range; + vec![TextEdit { + new_text: if list_is_empty { name } else { format!("{}, ", name) }, + range: Range { start: range.start, end: range.start }, + }] + } else { + new_use_completion_item_additional_text_edits(request) + } + } + } +} + +fn new_use_completion_item_additional_text_edits( + request: UseCompletionItemAdditionTextEditsRequest, +) -> Vec { + let line = request.auto_import_line as u32; + let character = (request.nesting * 4) as u32; + let indent = " ".repeat(request.nesting * 4); + let mut newlines = "\n"; + + // If the line we are inserting into is not an empty line, insert an extra line to make some room + if let Some(line_text) = request.lines.get(line as usize) { + if !line_text.trim().is_empty() { + newlines = "\n\n"; + } + } + + vec![TextEdit { + range: Range { start: Position { line, character }, end: Position { line, character } }, + new_text: format!("use {};{}{}", request.full_path, newlines, indent), + }] +}