From 4cdde6addb5fbc69ac45b950d1ff720b1d5c04e4 Mon Sep 17 00:00:00 2001 From: Mine Starks <16928427+minestarks@users.noreply.github.com> Date: Thu, 7 Dec 2023 10:59:18 -0800 Subject: [PATCH] Fix span offsets in `additional_text_edits` in completions (#884) This bug was exposed while testing multi-file projects. Plus, some test utilities for testing language service features with multiple sources. --- language_service/src/completion.rs | 12 +- language_service/src/completion/tests.rs | 76 +++++- language_service/src/definition/tests.rs | 16 +- language_service/src/hover/tests.rs | 28 +-- language_service/src/references/tests.rs | 27 +- language_service/src/rename/tests.rs | 16 +- language_service/src/signature_help/tests.rs | 7 +- language_service/src/test_utils.rs | 249 +++++++++++-------- 8 files changed, 253 insertions(+), 178 deletions(-) diff --git a/language_service/src/completion.rs b/language_service/src/completion.rs index 90ea2843b8..68b1a3d47a 100644 --- a/language_service/src/completion.rs +++ b/language_service/src/completion.rs @@ -8,8 +8,8 @@ use std::rc::Rc; use crate::compilation::{Compilation, CompilationKind}; use crate::display::CodeDisplay; -use crate::protocol::{self, CompletionItem, CompletionItemKind, CompletionList}; -use crate::qsc_utils::span_contains; +use crate::protocol::{CompletionItem, CompletionItemKind, CompletionList}; +use crate::qsc_utils::{protocol_span, span_contains}; use qsc::ast::visit::{self, Visitor}; use qsc::hir::{ItemKind, Package, PackageId}; @@ -373,7 +373,13 @@ impl CompletionListBuilder { None => match start_of_namespace { Some(start) => { additional_edits.push(( - protocol::Span { start, end: start }, + protocol_span( + qsc::Span { + lo: start, + hi: start, + }, + &compilation.user_unit().sources, + ), format!( "open {};{}", namespace.name.clone(), diff --git a/language_service/src/completion/tests.rs b/language_service/src/completion/tests.rs index 8a93346b41..09ed9677d7 100644 --- a/language_service/src/completion/tests.rs +++ b/language_service/src/completion/tests.rs @@ -7,15 +7,35 @@ use expect_test::{expect, Expect}; use super::{get_completions, CompletionItem}; use crate::test_utils::{ - compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib, - get_source_and_marker_offsets, + compile_notebook_with_fake_stdlib_and_markers, compile_project_with_fake_stdlib_and_markers, + compile_with_fake_stdlib_and_markers, }; use indoc::indoc; fn check(source_with_cursor: &str, completions_to_check: &[&str], expect: &Expect) { - let (source, cursor_offset, _) = get_source_and_marker_offsets(source_with_cursor); - let compilation = compile_with_fake_stdlib("", &source); - let actual_completions = get_completions(&compilation, "", cursor_offset[0]); + let (compilation, cursor_offset, _) = compile_with_fake_stdlib_and_markers(source_with_cursor); + let actual_completions = get_completions(&compilation, "", cursor_offset); + 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_project( + sources_with_markers: &[(&str, &str)], + completions_to_check: &[&str], + expect: &Expect, +) { + let (compilation, cursor_uri, cursor_offset, _) = + compile_project_with_fake_stdlib_and_markers(sources_with_markers); + let actual_completions = get_completions(&compilation, &cursor_uri, cursor_offset); let checked_completions: Vec> = completions_to_check .iter() .map(|comp| { @@ -241,6 +261,52 @@ fn in_block_from_other_namespace() { ); } +#[test] +fn auto_open_multiple_files() { + check_project( + &[ + ( + "foo.qs", + indoc! {r#"namespace Foo { operation FooOperation() : Unit {} } + "#}, + ), + ( + "bar.qs", + indoc! {r#"namespace Bar { operation BarOperation() : Unit { ↘ } } + "#}, + ), + ], + &["FooOperation"], + &expect![[r#" + [ + Some( + CompletionItem { + label: "FooOperation", + kind: Function, + sort_text: Some( + "0500FooOperation", + ), + detail: Some( + "operation FooOperation() : Unit", + ), + additional_text_edits: Some( + [ + ( + Span { + start: 16, + end: 16, + }, + "open Foo;\n ", + ), + ], + ), + }, + ), + ] + "#]], + ); +} + #[ignore = "nested callables are not currently supported for completions"] #[test] fn in_block_nested_op() { diff --git a/language_service/src/definition/tests.rs b/language_service/src/definition/tests.rs index 3ceea63c81..383fe53378 100644 --- a/language_service/src/definition/tests.rs +++ b/language_service/src/definition/tests.rs @@ -9,8 +9,7 @@ use super::get_definition; use crate::{ protocol::Location, test_utils::{ - compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib, - get_source_and_marker_offsets, target_offsets_to_spans, + compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib_and_markers, }, }; @@ -18,11 +17,9 @@ use crate::{ /// The cursor position is indicated by a `↘` marker in the source text. /// The expected definition range is indicated by `◉` markers in the source text. fn assert_definition(source_with_markers: &str) { - let (source, cursor_offsets, target_offsets) = - get_source_and_marker_offsets(source_with_markers); - let target_spans = target_offsets_to_spans(&target_offsets); - let compilation = compile_with_fake_stdlib("", &source); - let actual_definition = get_definition(&compilation, "", cursor_offsets[0]); + let (compilation, cursor_offset, target_spans) = + compile_with_fake_stdlib_and_markers(source_with_markers); + let actual_definition = get_definition(&compilation, "", cursor_offset); let expected_definition = if target_spans.is_empty() { None } else { @@ -50,9 +47,8 @@ fn assert_definition_notebook(cells_with_markers: &[(&str, &str)]) { } fn check(source_with_markers: &str, expect: &Expect) { - let (source, cursor_offsets, _) = get_source_and_marker_offsets(source_with_markers); - let compilation = compile_with_fake_stdlib("", &source); - let actual_definition = get_definition(&compilation, "", cursor_offsets[0]); + let (compilation, cursor_offset, _) = compile_with_fake_stdlib_and_markers(source_with_markers); + let actual_definition = get_definition(&compilation, "", cursor_offset); expect.assert_debug_eq(&actual_definition); } diff --git a/language_service/src/hover/tests.rs b/language_service/src/hover/tests.rs index 7cb9823df1..d0a0e4e3f6 100644 --- a/language_service/src/hover/tests.rs +++ b/language_service/src/hover/tests.rs @@ -4,12 +4,8 @@ #![allow(clippy::needless_raw_string_hashes)] use super::get_hover; -use crate::{ - protocol, - test_utils::{ - compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib, - get_source_and_marker_offsets, - }, +use crate::test_utils::{ + compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib_and_markers, }; use expect_test::{expect, Expect}; use indoc::indoc; @@ -18,25 +14,17 @@ use indoc::indoc; /// The cursor position is indicated by a `↘` marker in the source text. /// The expected hover span is indicated by two `◉` markers in the source text. fn check(source_with_markers: &str, expect: &Expect) { - let (source, cursor_offsets, target_offsets) = - get_source_and_marker_offsets(source_with_markers); - let compilation = compile_with_fake_stdlib("", &source); - let actual = get_hover(&compilation, "", cursor_offsets[0]).expect("Expected a hover."); - assert_eq!( - &actual.span, - &protocol::Span { - start: target_offsets[0], - end: target_offsets[1], - } - ); + let (compilation, cursor_offset, target_spans) = + compile_with_fake_stdlib_and_markers(source_with_markers); + let actual = get_hover(&compilation, "", cursor_offset).expect("Expected a hover."); + assert_eq!(&actual.span, &target_spans[0]); expect.assert_eq(&actual.contents); } /// Asserts that there is no hover for the given test case. fn check_none(source_with_markers: &str) { - let (source, cursor_offsets, _) = get_source_and_marker_offsets(source_with_markers); - let compilation = compile_with_fake_stdlib("", &source); - let actual = get_hover(&compilation, "", cursor_offsets[0]); + let (compilation, cursor_offset, _) = compile_with_fake_stdlib_and_markers(source_with_markers); + let actual = get_hover(&compilation, "", cursor_offset); assert!(actual.is_none()); } diff --git a/language_service/src/references/tests.rs b/language_service/src/references/tests.rs index df63a52e7f..bd893a8213 100644 --- a/language_service/src/references/tests.rs +++ b/language_service/src/references/tests.rs @@ -7,8 +7,7 @@ use super::get_references; use crate::{ protocol, test_utils::{ - compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib, - get_source_and_marker_offsets, target_offsets_to_spans, + compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib_and_markers, }, }; use expect_test::{expect, Expect}; @@ -17,9 +16,8 @@ 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 (source, cursor_offsets, _) = get_source_and_marker_offsets(source_with_markers); - let compilation = compile_with_fake_stdlib("", &source); - let actual = get_references(&compilation, "", cursor_offsets[0], true); + let (compilation, cursor_offset, _) = compile_with_fake_stdlib_and_markers(source_with_markers); + let actual = get_references(&compilation, "", cursor_offset, true); expect.assert_debug_eq(&actual); } @@ -27,19 +25,12 @@ fn check_with_std(source_with_markers: &str, expect: &Expect) { /// The cursor position is indicated by a `↘` marker in the source text. /// The expected reference location ranges are indicated by `◉` markers in the source text. fn check(source_with_markers: &str, include_declaration: bool) { - let (source, cursor_offsets, target_offsets) = - get_source_and_marker_offsets(source_with_markers); - let target_spans = target_offsets_to_spans(&target_offsets); - let compilation = compile_with_fake_stdlib("", &source); - let actual = get_references( - &compilation, - "", - cursor_offsets[0], - include_declaration, - ) - .into_iter() - .map(|l| l.span) - .collect::>(); + let (compilation, cursor_offset, target_spans) = + compile_with_fake_stdlib_and_markers(source_with_markers); + let actual = get_references(&compilation, "", cursor_offset, include_declaration) + .into_iter() + .map(|l| l.span) + .collect::>(); for target in &target_spans { assert!( actual.contains(target), diff --git a/language_service/src/rename/tests.rs b/language_service/src/rename/tests.rs index ad4d4f5fe4..e7a38ab835 100644 --- a/language_service/src/rename/tests.rs +++ b/language_service/src/rename/tests.rs @@ -5,8 +5,7 @@ use super::{get_rename, prepare_rename}; use crate::test_utils::{ - compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib, - get_source_and_marker_offsets, target_offsets_to_spans, + compile_notebook_with_fake_stdlib_and_markers, compile_with_fake_stdlib_and_markers, }; use expect_test::{expect, Expect}; @@ -14,11 +13,9 @@ use expect_test::{expect, Expect}; /// The cursor position is indicated by a `↘` marker in the source text. /// The expected rename location ranges are indicated by `◉` markers in the source text. fn check(source_with_markers: &str) { - let (source, cursor_offsets, target_offsets) = - get_source_and_marker_offsets(source_with_markers); - let target_spans = target_offsets_to_spans(&target_offsets); - let compilation = compile_with_fake_stdlib("", &source); - let actual = get_rename(&compilation, "", cursor_offsets[0]) + let (compilation, cursor_offset, target_spans) = + compile_with_fake_stdlib_and_markers(source_with_markers); + let actual = get_rename(&compilation, "", cursor_offset) .into_iter() .map(|l| l.span) .collect::>(); @@ -31,9 +28,8 @@ 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 (source, cursor_offsets, _) = get_source_and_marker_offsets(source_with_markers); - let compilation = compile_with_fake_stdlib("", &source); - let actual = prepare_rename(&compilation, "", cursor_offsets[0]); + let (compilation, cursor_offset, _) = compile_with_fake_stdlib_and_markers(source_with_markers); + let actual = prepare_rename(&compilation, "", cursor_offset); assert!(actual.is_none()); } diff --git a/language_service/src/signature_help/tests.rs b/language_service/src/signature_help/tests.rs index 0b261979af..2488ce5727 100644 --- a/language_service/src/signature_help/tests.rs +++ b/language_service/src/signature_help/tests.rs @@ -4,16 +4,15 @@ #![allow(clippy::needless_raw_string_hashes)] use super::get_signature_help; -use crate::test_utils::{compile_with_fake_stdlib, get_source_and_marker_offsets}; +use crate::test_utils::compile_with_fake_stdlib_and_markers; 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 (source, cursor_offsets, _) = get_source_and_marker_offsets(source_with_markers); - let compilation = compile_with_fake_stdlib("", &source); - let actual = get_signature_help(&compilation, "", cursor_offsets[0]) + let (compilation, cursor_offset, _) = compile_with_fake_stdlib_and_markers(source_with_markers); + let actual = get_signature_help(&compilation, "", cursor_offset) .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 9013487597..7e98aed3d7 100644 --- a/language_service/src/test_utils.rs +++ b/language_service/src/test_utils.rs @@ -1,87 +1,37 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +use std::sync::Arc; + use crate::{ compilation::{Compilation, CompilationKind}, - protocol, + protocol::Span, }; use qsc::{ compile, hir::PackageId, incremental::Compiler, PackageStore, PackageType, SourceMap, TargetProfile, }; -pub(crate) fn get_source_and_marker_offsets( +pub(crate) fn compile_with_fake_stdlib_and_markers( source_with_markers: &str, -) -> (String, Vec, Vec) { - let mut cursor_offsets: Vec = Vec::new(); - let mut target_offsets: Vec = Vec::new(); - let mut source = source_with_markers.to_string(); - let markers = &['↘', '◉']; - - loop { - let next_offset = source.find(markers); - match next_offset { - #[allow(clippy::cast_possible_truncation)] - Some(offset) => match source.chars().nth(offset) { - Some('↘') => cursor_offsets.push(offset as u32), - Some('◉') => target_offsets.push(offset as u32), - _ => panic!("Expected to find marker"), - }, - None => break, - }; - source = source.replacen(markers, "", 1); - } - (source, cursor_offsets, target_offsets) +) -> (Compilation, u32, Vec) { + let (compilation, _, cursor_offset, target_spans) = + compile_project_with_fake_stdlib_and_markers(&[("", source_with_markers)]); + ( + compilation, + cursor_offset, + target_spans.iter().map(|(_, s)| *s).collect(), + ) } -pub(crate) fn target_offsets_to_spans(target_offsets: &Vec) -> Vec { - assert!(target_offsets.len() % 2 == 0); - let limit = target_offsets.len() / 2; - let mut spans = vec![]; - for i in 0..limit { - spans.push(protocol::Span { - start: target_offsets[i * 2], - end: target_offsets[i * 2 + 1], - }); - } - spans -} +pub(crate) fn compile_project_with_fake_stdlib_and_markers( + sources_with_markers: &[(&str, &str)], +) -> (Compilation, String, u32, Vec<(String, Span)>) { + let (sources, cursor_uri, cursor_offset, target_spans) = + get_sources_and_markers(sources_with_markers); -pub(crate) fn compile_with_fake_stdlib(source_name: &str, source_contents: &str) -> Compilation { - let mut package_store = PackageStore::new(compile::core()); - let std_source_map = SourceMap::new( - [( - "".into(), - r#"namespace FakeStdLib { - operation Fake() : Unit {} - operation FakeWithParam(x : Int) : Unit {} - operation FakeCtlAdj() : Unit is Ctl + Adj {} - newtype Udt = (x : Int, y : Int); - newtype UdtWrapper = (inner : Udt); - newtype UdtFn = (Int -> Int); - newtype UdtFnWithUdtParams = (Udt -> Udt); - function TakesUdt(input : Udt) : Udt { - fail "not implemented" - } - operation RefFake() : Unit { - Fake(); - } - operation FakeWithTypeParam<'A>(a : 'A) : 'A { a } - }"# - .into(), - )], - None, - ); - let (std_compile_unit, std_errors) = compile::compile( - &package_store, - &[PackageId::CORE], - std_source_map, - PackageType::Lib, - TargetProfile::Full, - ); - assert!(std_errors.is_empty()); - let std_package_id = package_store.insert(std_compile_unit); - let source_map = SourceMap::new([(source_name.into(), source_contents.into())], None); + let source_map = SourceMap::new(sources, None); + let (mut package_store, std_package_id) = compile_fake_stdlib(); let (unit, errors) = compile::compile( &package_store, &[std_package_id], @@ -92,50 +42,27 @@ pub(crate) fn compile_with_fake_stdlib(source_name: &str, source_contents: &str) let package_id = package_store.insert(unit); - Compilation { - package_store, - user_package_id: package_id, - kind: CompilationKind::OpenProject, - errors, - } + ( + Compilation { + package_store, + user_package_id: package_id, + kind: CompilationKind::OpenProject, + errors, + }, + cursor_uri, + cursor_offset, + target_spans, + ) } pub(crate) fn compile_notebook_with_fake_stdlib_and_markers( cells_with_markers: &[(&str, &str)], -) -> (Compilation, String, u32, Vec<(String, protocol::Span)>) { - let (mut cell_uri, mut offset, mut target_spans) = (None, None, Vec::new()); - let cells = cells_with_markers - .iter() - .map(|c| { - let (source, cursor_offsets, targets) = get_source_and_marker_offsets(c.1); - if !cursor_offsets.is_empty() { - assert!( - cell_uri.replace(c.0).is_none(), - "only one cell can have a cursor marker" - ); - assert!( - offset.replace(cursor_offsets[0]).is_none(), - "only one cell can have a cursor marker" - ); - } - if !targets.is_empty() { - for span in target_offsets_to_spans(&targets) { - target_spans.push((c.0.to_string(), span)); - } - } - (c.0, source) - }) - .collect::>(); +) -> (Compilation, String, u32, Vec<(String, Span)>) { + let (cells, cell_uri, offset, target_spans) = get_sources_and_markers(cells_with_markers); - let compilation = compile_notebook_with_fake_stdlib(cells.iter().map(|c| (c.0, c.1.as_str()))); - ( - compilation, - cell_uri - .expect("input should have a cursor marker") - .to_string(), - offset.expect("input string should have a cursor marker"), - target_spans, - ) + let compilation = + compile_notebook_with_fake_stdlib(cells.iter().map(|c| (c.0.as_ref(), c.1.as_ref()))); + (compilation, cell_uri, offset, target_spans) } fn compile_notebook_with_fake_stdlib<'a, I>(cells: I) -> Compilation @@ -181,3 +108,109 @@ where kind: CompilationKind::Notebook, } } + +fn compile_fake_stdlib() -> (PackageStore, PackageId) { + let mut package_store = PackageStore::new(compile::core()); + let std_source_map = SourceMap::new( + [( + "".into(), + r#"namespace FakeStdLib { + operation Fake() : Unit {} + operation FakeWithParam(x : Int) : Unit {} + operation FakeCtlAdj() : Unit is Ctl + Adj {} + newtype Udt = (x : Int, y : Int); + newtype UdtWrapper = (inner : Udt); + newtype UdtFn = (Int -> Int); + newtype UdtFnWithUdtParams = (Udt -> Udt); + function TakesUdt(input : Udt) : Udt { + fail "not implemented" + } + operation RefFake() : Unit { + Fake(); + } + operation FakeWithTypeParam<'A>(a : 'A) : 'A { a } + }"# + .into(), + )], + None, + ); + let (std_compile_unit, std_errors) = compile::compile( + &package_store, + &[PackageId::CORE], + std_source_map, + PackageType::Lib, + TargetProfile::Full, + ); + assert!(std_errors.is_empty()); + let std_package_id = package_store.insert(std_compile_unit); + (package_store, std_package_id) +} + +#[allow(clippy::type_complexity)] +fn get_sources_and_markers( + sources: &[(&str, &str)], +) -> (Vec<(Arc, Arc)>, String, u32, Vec<(String, Span)>) { + let (mut cursor_uri, mut cursor_offset, mut target_spans) = (None, None, Vec::new()); + let sources = sources + .iter() + .map(|s| { + let (source, cursor_offsets, targets) = get_source_and_marker_offsets(s.1); + if !cursor_offsets.is_empty() { + assert!( + cursor_uri.replace(s.0).is_none(), + "only one cell can have a cursor marker" + ); + assert!( + cursor_offset.replace(cursor_offsets[0]).is_none(), + "only one cell can have a cursor marker" + ); + } + if !targets.is_empty() { + for span in target_offsets_to_spans(&targets) { + target_spans.push((s.0.to_string(), span)); + } + } + (Arc::from(s.0), Arc::from(source.as_ref())) + }) + .collect(); + let cursor_uri = cursor_uri + .expect("input should have a cursor marker") + .to_string(); + let cursor_offset = cursor_offset.expect("input string should have a cursor marker"); + (sources, cursor_uri, cursor_offset, target_spans) +} + +fn get_source_and_marker_offsets(source_with_markers: &str) -> (String, Vec, Vec) { + let mut cursor_offsets: Vec = Vec::new(); + let mut target_offsets: Vec = Vec::new(); + let mut source = source_with_markers.to_string(); + let markers = &['↘', '◉']; + + loop { + let next_offset = source.find(markers); + match next_offset { + #[allow(clippy::cast_possible_truncation)] + Some(offset) => match source.chars().nth(offset) { + Some('↘') => cursor_offsets.push(offset as u32), + Some('◉') => target_offsets.push(offset as u32), + _ => panic!("Expected to find marker"), + }, + None => break, + }; + source = source.replacen(markers, "", 1); + } + (source, cursor_offsets, target_offsets) +} + +fn target_offsets_to_spans(target_offsets: &Vec) -> Vec { + assert!(target_offsets.len() % 2 == 0); + let limit = target_offsets.len() / 2; + let mut spans = vec![]; + for i in 0..limit { + spans.push(Span { + start: target_offsets[i * 2], + end: target_offsets[i * 2 + 1], + }); + } + spans +}