From 1096d872245e980c5ff3089537e66c4137672a4f Mon Sep 17 00:00:00 2001 From: Alex Hansen Date: Tue, 20 Aug 2024 11:45:36 -0700 Subject: [PATCH] generate auto imports for core callables (#1861) Closes #1860 Currently, we have a separate code path for generating completions for core callables. This code path just hard-codes the callables to not include any auto-generated imports. This may have been necessary at some point, but it no longer is and it is preventing us from generating the correct auto-imports for `QIR.Runtime`. We now have a function that generates completions from a package and is aware of if imports are needed or not, so I just went ahead and used that instead of special casing `Core`. This PR: - Uses the standard completion codepath to generate completions for `Core` - Adds the ability to unit test completions with the real stdlib, not just the fake stdlib - Adds tests to ensure auto-imports are generated for items that are not already in scope (e.g. `QIR.Runtime.*`) and are _not_ generated for items already in scope (e.g. `Length`). --- compiler/qsc_data_structures/src/index_map.rs | 10 ++ compiler/qsc_frontend/src/compile.rs | 6 + language_service/src/code_lens/tests.rs | 2 +- language_service/src/completion.rs | 55 +------ language_service/src/completion/tests.rs | 136 +++++++++++++++++- language_service/src/definition/tests.rs | 11 +- language_service/src/hover/tests.rs | 14 +- language_service/src/references/tests.rs | 11 +- language_service/src/rename/tests.rs | 15 +- language_service/src/signature_help/tests.rs | 5 +- language_service/src/test_utils.rs | 39 +++-- 11 files changed, 195 insertions(+), 109 deletions(-) diff --git a/compiler/qsc_data_structures/src/index_map.rs b/compiler/qsc_data_structures/src/index_map.rs index bab8b22bba..d5da735d46 100644 --- a/compiler/qsc_data_structures/src/index_map.rs +++ b/compiler/qsc_data_structures/src/index_map.rs @@ -227,6 +227,16 @@ pub struct IterMut<'a, K, V> { base: Enumerate>>, } +impl, V> DoubleEndedIterator for Iter<'_, K, V> { + fn next_back(&mut self) -> Option { + loop { + if let (index, Some(value)) = self.base.next_back()? { + break Some((index.into(), value)); + } + } + } +} + impl<'a, K: From, V> Iterator for IterMut<'a, K, V> { type Item = (K, &'a mut V); diff --git a/compiler/qsc_frontend/src/compile.rs b/compiler/qsc_frontend/src/compile.rs index 30a4f4339e..7b1a863c4b 100644 --- a/compiler/qsc_frontend/src/compile.rs +++ b/compiler/qsc_frontend/src/compile.rs @@ -334,6 +334,12 @@ impl<'a> Iterator for Iter<'a> { } } +impl DoubleEndedIterator for Iter<'_> { + fn next_back(&mut self) -> Option { + self.0.next_back() + } +} + pub(super) struct Offsetter(pub(super) u32); impl MutVisitor for Offsetter { diff --git a/language_service/src/code_lens/tests.rs b/language_service/src/code_lens/tests.rs index d8728d1c77..15221a9014 100644 --- a/language_service/src/code_lens/tests.rs +++ b/language_service/src/code_lens/tests.rs @@ -14,7 +14,7 @@ use expect_test::{expect, Expect}; fn check(source_with_markers: &str, expect: &Expect) { let (compilation, expected_code_lens_ranges) = - compile_with_fake_stdlib_and_markers_no_cursor(source_with_markers); + compile_with_fake_stdlib_and_markers_no_cursor(source_with_markers, true); let mut actual_code_lenses = get_code_lenses(&compilation, "", Encoding::Utf8); for expected_range in &expected_code_lens_ranges { diff --git a/language_service/src/completion.rs b/language_service/src/completion.rs index 094035b8be..e8af05f39f 100644 --- a/language_service/src/completion.rs +++ b/language_service/src/completion.rs @@ -337,25 +337,10 @@ impl CompletionListBuilder { current_namespace_name: &Option>>, indent: &String, ) { - let core = &compilation - .package_store - .get(PackageId::CORE) - .expect("expected to find core package") - .package; - - let mut all_except_core = compilation - .package_store - .iter() - .filter(|p| p.0 != PackageId::CORE) - .collect::>(); - - // Reverse to collect symbols starting at the current package backwards - all_except_core.reverse(); - - for (package_id, _) in &all_except_core { + for (package_id, _) in compilation.package_store.iter().rev() { self.push_sorted_completions(Self::get_callables( compilation, - *package_id, + package_id, imports, insert_open_range, current_namespace_name.as_deref(), @@ -363,14 +348,10 @@ impl CompletionListBuilder { )); } - self.push_sorted_completions(Self::get_core_callables(compilation, core)); - - for (id, unit) in &all_except_core { - let alias = compilation.dependencies.get(id).cloned().flatten(); + for (id, unit) in compilation.package_store.iter().rev() { + let alias = compilation.dependencies.get(&id).cloned().flatten(); self.push_completions(Self::get_namespaces(&unit.package, alias)); } - - self.push_completions(Self::get_namespaces(core, None)); } fn push_locals( @@ -500,34 +481,6 @@ impl CompletionListBuilder { }) } - /// Get all callables in the core package - fn get_core_callables<'a>( - compilation: &'a Compilation, - package: &'a Package, - ) -> impl Iterator + 'a { - let display = CodeDisplay { compilation }; - - package.items.values().filter_map(move |i| match &i.kind { - ItemKind::Callable(callable_decl) => { - let name = callable_decl.name.name.as_ref(); - let detail = Some(display.hir_callable_decl(callable_decl).to_string()); - // Everything that starts with a __ goes last in the list - let sort_group = SortPriority::from(name.starts_with("__")); - Some(( - CompletionItem { - label: name.to_string(), - kind: CompletionItemKind::Function, - sort_text: None, // This will get filled in during `push_sorted_completions` - detail, - additional_text_edits: None, - }, - sort_group, - )) - } - _ => None, - }) - } - fn get_namespaces( package: &'_ Package, package_alias: Option>, diff --git a/language_service/src/completion/tests.rs b/language_service/src/completion/tests.rs index 12d34b958c..c22a8e5535 100644 --- a/language_service/src/completion/tests.rs +++ b/language_service/src/completion/tests.rs @@ -9,16 +9,31 @@ use super::{get_completions, CompletionItem}; use crate::{ protocol::CompletionList, test_utils::{ - compile_notebook_with_fake_stdlib_and_markers, - compile_project_with_fake_stdlib_and_markers, compile_with_fake_stdlib_and_markers, + compile_notebook_with_markers, compile_project_with_markers, compile_with_markers, }, Encoding, }; use indoc::indoc; fn check(source_with_cursor: &str, completions_to_check: &[&str], expect: &Expect) { - let (compilation, cursor_position, _) = - compile_with_fake_stdlib_and_markers(source_with_cursor); + let (compilation, cursor_position, _) = compile_with_markers(source_with_cursor, true); + let actual_completions = + get_completions(&compilation, "", cursor_position, Encoding::Utf8); + let checked_completions: Vec> = completions_to_check + .iter() + .map(|comp| { + actual_completions + .items + .iter() + .find(|item| item.label == **comp) + }) + .collect(); + + expect.assert_debug_eq(&checked_completions); +} + +fn check_with_stdlib(source_with_cursor: &str, completions_to_check: &[&str], expect: &Expect) { + let (compilation, cursor_position, _) = compile_with_markers(source_with_cursor, false); let actual_completions = get_completions(&compilation, "", cursor_position, Encoding::Utf8); let checked_completions: Vec> = completions_to_check @@ -40,7 +55,7 @@ fn check_project( expect: &Expect, ) { let (compilation, cursor_uri, cursor_position, _) = - compile_project_with_fake_stdlib_and_markers(sources_with_markers); + compile_project_with_markers(sources_with_markers, true); let actual_completions = get_completions(&compilation, &cursor_uri, cursor_position, Encoding::Utf8); let checked_completions: Vec> = completions_to_check @@ -63,7 +78,7 @@ fn check_notebook( expect: &Expect, ) { let (compilation, cell_uri, cursor_position, _) = - compile_notebook_with_fake_stdlib_and_markers(cells_with_markers); + compile_notebook_with_markers(cells_with_markers); let actual_completions = get_completions(&compilation, &cell_uri, cursor_position, Encoding::Utf8); let checked_completions: Vec> = completions_to_check @@ -1457,6 +1472,115 @@ fn dont_import_if_already_directly_imported() { ); } +#[test] +fn auto_import_from_qir_runtime() { + check_with_stdlib( + r#" + namespace Test { + operation Main() : Unit { + AllocateQubitA↘ + } + }"#, + &["AllocateQubitArray"], + &expect![[r#" + [ + Some( + CompletionItem { + label: "AllocateQubitArray", + kind: Function, + sort_text: Some( + "0800AllocateQubitArray", + ), + detail: Some( + "operation AllocateQubitArray(size : Int) : Qubit[]", + ), + additional_text_edits: Some( + [ + TextEdit { + new_text: "import QIR.Runtime.AllocateQubitArray;\n ", + range: Range { + start: Position { + line: 2, + column: 12, + }, + end: Position { + line: 2, + column: 12, + }, + }, + }, + ], + ), + }, + ), + ] + "#]], + ); +} + +#[test] +fn dont_generate_import_for_core_prelude() { + check_with_stdlib( + r#" + namespace Test { + operation Main() : Unit { + Length↘ + } + }"#, + &["Length"], + // additional text edits should be None because Length is in the core prelude + &expect![[r#" + [ + Some( + CompletionItem { + label: "Length", + kind: Function, + sort_text: Some( + "0800Length", + ), + detail: Some( + "function Length<'T>(a : 'T[]) : Int", + ), + additional_text_edits: None, + }, + ), + ] + "#]], + ); +} + +#[test] +fn dont_generate_import_for_stdlib_prelude() { + check_with_stdlib( + r#" + namespace Test { + operation Main() : Unit { + MResetZ↘ + } + }"#, + &["MResetZ"], + // additional text edits should be None because MResetZ is in Std.Measurement, which + // is in the prelude. + &expect![[r#" + [ + Some( + CompletionItem { + label: "MResetZ", + kind: Function, + sort_text: Some( + "0700MResetZ", + ), + detail: Some( + "operation MResetZ(target : Qubit) : Result", + ), + additional_text_edits: None, + }, + ), + ] + "#]], + ); +} + #[test] fn callable_from_same_file() { check( diff --git a/language_service/src/definition/tests.rs b/language_service/src/definition/tests.rs index 27eeddd4a5..704f1535bb 100644 --- a/language_service/src/definition/tests.rs +++ b/language_service/src/definition/tests.rs @@ -8,9 +8,7 @@ use qsc::location::Location; use super::get_definition; use crate::{ - test_utils::{ - compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib_and_markers, - }, + test_utils::{compile_notebook_with_markers, compile_with_markers}, Encoding, }; @@ -19,7 +17,7 @@ use crate::{ /// The expected definition range is indicated by `◉` markers in the source text. fn assert_definition(source_with_markers: &str) { let (compilation, cursor_position, target_spans) = - compile_with_fake_stdlib_and_markers(source_with_markers); + compile_with_markers(source_with_markers, true); let actual_definition = get_definition(&compilation, "", cursor_position, Encoding::Utf8); let expected_definition = if target_spans.is_empty() { @@ -35,7 +33,7 @@ fn assert_definition(source_with_markers: &str) { fn assert_definition_notebook(cells_with_markers: &[(&str, &str)]) { let (compilation, cell_uri, position, target_spans) = - compile_notebook_with_fake_stdlib_and_markers(cells_with_markers); + compile_notebook_with_markers(cells_with_markers); let actual_definition = get_definition(&compilation, &cell_uri, position, Encoding::Utf8); let expected_definition = if target_spans.is_empty() { None @@ -46,8 +44,7 @@ fn assert_definition_notebook(cells_with_markers: &[(&str, &str)]) { } fn check(source_with_markers: &str, expect: &Expect) { - let (compilation, cursor_position, _) = - compile_with_fake_stdlib_and_markers(source_with_markers); + let (compilation, cursor_position, _) = compile_with_markers(source_with_markers, true); let actual_definition = get_definition(&compilation, "", cursor_position, Encoding::Utf8); expect.assert_debug_eq(&actual_definition); diff --git a/language_service/src/hover/tests.rs b/language_service/src/hover/tests.rs index 3b303c1139..fa99ff1b85 100644 --- a/language_service/src/hover/tests.rs +++ b/language_service/src/hover/tests.rs @@ -4,9 +4,7 @@ #![allow(clippy::needless_raw_string_hashes)] use super::get_hover; -use crate::test_utils::{ - compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib_and_markers, -}; +use crate::test_utils::{compile_notebook_with_markers, compile_with_markers}; use expect_test::{expect, Expect}; use indoc::indoc; use qsc::line_column::Encoding; @@ -16,7 +14,7 @@ use qsc::line_column::Encoding; /// The expected hover span is indicated by two `◉` markers in the source text. fn check(source_with_markers: &str, expect: &Expect) { let (compilation, cursor_position, target_spans) = - compile_with_fake_stdlib_and_markers(source_with_markers); + compile_with_markers(source_with_markers, true); let actual = get_hover(&compilation, "", cursor_position, Encoding::Utf8) .expect("Expected a hover."); assert_eq!(&actual.span, &target_spans[0]); @@ -25,15 +23,14 @@ fn check(source_with_markers: &str, expect: &Expect) { /// Asserts that there is no hover for the given test case. fn check_none(source_with_markers: &str) { - let (compilation, cursor_position, _) = - compile_with_fake_stdlib_and_markers(source_with_markers); + let (compilation, cursor_position, _) = compile_with_markers(source_with_markers, true); let actual = get_hover(&compilation, "", cursor_position, Encoding::Utf8); assert!(actual.is_none()); } fn check_notebook(cells_with_markers: &[(&str, &str)], expect: &Expect) { let (compilation, cell_uri, position, target_spans) = - compile_notebook_with_fake_stdlib_and_markers(cells_with_markers); + compile_notebook_with_markers(cells_with_markers); let actual = get_hover(&compilation, &cell_uri, position, Encoding::Utf8).expect("Expected a hover."); @@ -42,8 +39,7 @@ fn check_notebook(cells_with_markers: &[(&str, &str)], expect: &Expect) { } fn check_notebook_none(cells_with_markers: &[(&str, &str)]) { - let (compilation, cell_uri, position, _) = - compile_notebook_with_fake_stdlib_and_markers(cells_with_markers); + let (compilation, cell_uri, position, _) = compile_notebook_with_markers(cells_with_markers); let actual = get_hover(&compilation, &cell_uri, position, Encoding::Utf8); assert!(actual.is_none()); diff --git a/language_service/src/references/tests.rs b/language_service/src/references/tests.rs index 2cc7be9ae0..9a9d28be5b 100644 --- a/language_service/src/references/tests.rs +++ b/language_service/src/references/tests.rs @@ -5,9 +5,7 @@ use super::get_references; use crate::{ - test_utils::{ - compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib_and_markers, - }, + test_utils::{compile_notebook_with_markers, compile_with_markers}, Encoding, }; use expect_test::{expect, Expect}; @@ -16,8 +14,7 @@ use indoc::indoc; /// Asserts that the reference locations given at the cursor position matches the expected reference locations. /// The cursor position is indicated by a `↘` marker in the source text. fn check_with_std(source_with_markers: &str, expect: &Expect) { - let (compilation, cursor_position, _) = - compile_with_fake_stdlib_and_markers(source_with_markers); + let (compilation, cursor_position, _) = compile_with_markers(source_with_markers, true); let actual = get_references( &compilation, "", @@ -33,7 +30,7 @@ fn check_with_std(source_with_markers: &str, expect: &Expect) { /// The expected reference location ranges are indicated by `◉` markers in the source text. fn check(source_with_markers: &str, include_declaration: bool) { let (compilation, cursor_position, target_spans) = - compile_with_fake_stdlib_and_markers(source_with_markers); + compile_with_markers(source_with_markers, true); let actual = get_references( &compilation, "", @@ -63,7 +60,7 @@ fn check_exclude_decl(source_with_markers: &str) { fn check_notebook_exclude_decl(cells_with_markers: &[(&str, &str)]) { let (compilation, cell_uri, position, target_spans) = - compile_notebook_with_fake_stdlib_and_markers(cells_with_markers); + compile_notebook_with_markers(cells_with_markers); let actual = get_references(&compilation, &cell_uri, position, Encoding::Utf8, false) .into_iter() diff --git a/language_service/src/rename/tests.rs b/language_service/src/rename/tests.rs index de2fbd987f..ad7f511aaf 100644 --- a/language_service/src/rename/tests.rs +++ b/language_service/src/rename/tests.rs @@ -5,9 +5,7 @@ use super::{get_rename, prepare_rename}; use crate::{ - test_utils::{ - compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib_and_markers, - }, + test_utils::{compile_notebook_with_markers, compile_with_markers}, Encoding, }; use expect_test::{expect, Expect}; @@ -17,7 +15,7 @@ use expect_test::{expect, Expect}; /// The expected rename location ranges are indicated by `◉` markers in the source text. fn check(source_with_markers: &str) { let (compilation, cursor_position, target_spans) = - compile_with_fake_stdlib_and_markers(source_with_markers); + compile_with_markers(source_with_markers, true); let actual = get_rename(&compilation, "", cursor_position, Encoding::Utf8) .into_iter() .map(|l| l.range) @@ -31,22 +29,19 @@ fn check(source_with_markers: &str) { /// Asserts that the prepare rename given at the cursor position returns None. /// The cursor position is indicated by a `↘` marker in the source text. fn assert_no_rename(source_with_markers: &str) { - let (compilation, cursor_position, _) = - compile_with_fake_stdlib_and_markers(source_with_markers); + let (compilation, cursor_position, _) = compile_with_markers(source_with_markers, true); let actual = prepare_rename(&compilation, "", cursor_position, Encoding::Utf8); assert!(actual.is_none()); } fn check_notebook(cells_with_markers: &[(&str, &str)], expect: &Expect) { - let (compilation, cell_uri, position, _) = - compile_notebook_with_fake_stdlib_and_markers(cells_with_markers); + let (compilation, cell_uri, position, _) = compile_notebook_with_markers(cells_with_markers); let actual = get_rename(&compilation, &cell_uri, position, Encoding::Utf8); expect.assert_debug_eq(&actual); } fn check_prepare_notebook(cells_with_markers: &[(&str, &str)], expect: &Expect) { - let (compilation, cell_uri, position, _) = - compile_notebook_with_fake_stdlib_and_markers(cells_with_markers); + let (compilation, cell_uri, position, _) = compile_notebook_with_markers(cells_with_markers); let actual = prepare_rename(&compilation, &cell_uri, position, Encoding::Utf8); expect.assert_debug_eq(&actual); } diff --git a/language_service/src/signature_help/tests.rs b/language_service/src/signature_help/tests.rs index 5d5b448ef1..c9165562f4 100644 --- a/language_service/src/signature_help/tests.rs +++ b/language_service/src/signature_help/tests.rs @@ -4,15 +4,14 @@ #![allow(clippy::needless_raw_string_hashes)] use super::get_signature_help; -use crate::{test_utils::compile_with_fake_stdlib_and_markers, Encoding}; +use crate::{test_utils::compile_with_markers, Encoding}; use expect_test::{expect, Expect}; use indoc::indoc; /// Asserts that the signature help given at the cursor position matches the expected signature help. /// The cursor position is indicated by a `↘` marker in the source text. fn check(source_with_markers: &str, expect: &Expect) { - let (compilation, cursor_position, _) = - compile_with_fake_stdlib_and_markers(source_with_markers); + let (compilation, cursor_position, _) = compile_with_markers(source_with_markers, true); let actual = get_signature_help(&compilation, "", cursor_position, Encoding::Utf8) .expect("Expected a signature help."); expect.assert_debug_eq(&actual); diff --git a/language_service/src/test_utils.rs b/language_service/src/test_utils.rs index 3a8bcbff2f..b08aaaaff4 100644 --- a/language_service/src/test_utils.rs +++ b/language_service/src/test_utils.rs @@ -16,11 +16,12 @@ use qsc::{ use qsc_project::{PackageGraphSources, PackageInfo}; use rustc_hash::FxHashMap; -pub(crate) fn compile_with_fake_stdlib_and_markers( +pub(crate) fn compile_with_markers( source_with_markers: &str, + use_fake_stdlib: bool, ) -> (Compilation, Position, Vec) { let (compilation, _, cursor_offset, target_spans) = - compile_project_with_fake_stdlib_and_markers(&[("", source_with_markers)]); + compile_project_with_markers(&[("", source_with_markers)], use_fake_stdlib); ( compilation, cursor_offset, @@ -30,19 +31,21 @@ pub(crate) fn compile_with_fake_stdlib_and_markers( pub(crate) fn compile_with_fake_stdlib_and_markers_no_cursor( source_with_markers: &str, + use_fake_stdlib: bool, ) -> (Compilation, Vec) { - let (compilation, target_spans) = compile_project_with_fake_stdlib_and_markers_no_cursor(&[( - "", - source_with_markers, - )]); + let (compilation, target_spans) = compile_project_with_markers_no_cursor( + &[("", source_with_markers)], + use_fake_stdlib, + ); (compilation, target_spans.iter().map(|l| l.range).collect()) } -pub(crate) fn compile_project_with_fake_stdlib_and_markers( +pub(crate) fn compile_project_with_markers( sources_with_markers: &[(&str, &str)], + use_fake_stdlib: bool, ) -> (Compilation, String, Position, Vec) { let (compilation, cursor_location, target_spans) = - compile_project_with_fake_stdlib_and_markers_cursor_optional(sources_with_markers); + compile_project_with_markers_cursor_optional(sources_with_markers, use_fake_stdlib); let (cursor_uri, cursor_offset) = cursor_location.expect("input string should have a cursor marker"); @@ -50,11 +53,12 @@ pub(crate) fn compile_project_with_fake_stdlib_and_markers( (compilation, cursor_uri, cursor_offset, target_spans) } -pub(crate) fn compile_project_with_fake_stdlib_and_markers_no_cursor( +pub(crate) fn compile_project_with_markers_no_cursor( sources_with_markers: &[(&str, &str)], + use_fake_stdlib: bool, ) -> (Compilation, Vec) { let (compilation, cursor_location, target_spans) = - compile_project_with_fake_stdlib_and_markers_cursor_optional(sources_with_markers); + compile_project_with_markers_cursor_optional(sources_with_markers, use_fake_stdlib); assert!( cursor_location.is_none(), @@ -64,13 +68,18 @@ pub(crate) fn compile_project_with_fake_stdlib_and_markers_no_cursor( (compilation, target_spans) } -fn compile_project_with_fake_stdlib_and_markers_cursor_optional( +fn compile_project_with_markers_cursor_optional( sources_with_markers: &[(&str, &str)], + use_fake_stdlib: bool, ) -> (Compilation, Option<(String, Position)>, Vec) { let (sources, cursor_location, target_spans) = get_sources_and_markers(sources_with_markers); let source_map = SourceMap::new(sources.clone(), None); - let (mut package_store, std_package_id) = compile_fake_stdlib(); + let (std_package_id, mut package_store) = if use_fake_stdlib { + compile_fake_stdlib() + } else { + qsc::compile::package_store_with_stdlib(qsc::TargetCapabilityFlags::all()) + }; let (unit, errors) = compile::compile( &package_store, &[(std_package_id, None)], @@ -106,7 +115,7 @@ fn compile_project_with_fake_stdlib_and_markers_cursor_optional( ) } -pub(crate) fn compile_notebook_with_fake_stdlib_and_markers( +pub(crate) fn compile_notebook_with_markers( cells_with_markers: &[(&str, &str)], ) -> (Compilation, String, Position, Vec) { let (cells, cursor_location, target_spans) = get_sources_and_markers(cells_with_markers); @@ -172,7 +181,7 @@ where } } -fn compile_fake_stdlib() -> (PackageStore, PackageId) { +fn compile_fake_stdlib() -> (PackageId, PackageStore) { let mut package_store = PackageStore::new(compile::core()); let std_source_map = SourceMap::new( [( @@ -217,7 +226,7 @@ fn compile_fake_stdlib() -> (PackageStore, PackageId) { ); assert!(std_errors.is_empty()); let std_package_id = package_store.insert(std_compile_unit); - (package_store, std_package_id) + (std_package_id, package_store) } #[allow(clippy::type_complexity)]