fix: Offset of bare completions

When completing an empty string, the completions for files would include the wrong offset, causing vscode to chop off the end of the line after accepting the completion. This is a relatively simple fix in `get_filesystem_entries`, but a lot of this PR is adding testing and extracting `bazel query` into the `MockBazel` client.

This also introduces a builder pattern for the test context, to allow adding certain information such as the mock results to `bazel query`.

fixtures/simple/root/.bazelrc

@@ -0,0 +1 @@
+common --noenable_bzlmod

fixtures/simple/root/BUILD

@@ -0,0 +1,8 @@
+cc_library(
+    name = "foo",
+    srcs = [
+        "//foo:main.cc",
+        "main.cc"
+    ],
+    deps = ["//foo:main"],
+)

fixtures/simple/root/foo/BUILD

@@ -0,0 +1,4 @@
+cc_library(
+    name = "main",
+    srcs = ["main.cc"],
+)

src/bazel.rs

@@ -31,7 +31,6 @@ use std::io;
 use std::iter;
 use std::path::Path;
 use std::path::PathBuf;
-use std::process::Command;
 
 use either::Either;
 use lsp_types::CompletionItemKind;
@@ -133,7 +132,7 @@ struct FilesystemCompletionOptions {
     targets: bool,
 }
 
-pub(crate) struct BazelContext {
+pub(crate) struct BazelContext<Client> {
     pub(crate) workspace_name: Option<String>,
     pub(crate) external_output_base: Option<PathBuf>,
     pub(crate) mode: ContextMode,
@@ -142,14 +141,15 @@ pub(crate) struct BazelContext {
     pub(crate) module: Option<Module>,
     pub(crate) builtin_docs: HashMap<LspUrl, String>,
     pub(crate) builtin_symbols: HashMap<String, LspUrl>,
+    pub(crate) client: Client,
 }
 
-impl BazelContext {
+impl<Client: BazelClient> BazelContext<Client> {
     const DEFAULT_WORKSPACE_NAME: &'static str = "__main__";
     const BUILD_FILE_NAMES: [&'static str; 2] = ["BUILD", "BUILD.bazel"];
     const LOADABLE_EXTENSIONS: [&'static str; 1] = ["bzl"];
 
-    pub(crate) fn new<Client: BazelClient>(
+    pub(crate) fn new(
         client: Client,
         mode: ContextMode,
         print_non_none: bool,
@@ -211,6 +211,7 @@ impl BazelContext {
             external_output_base: info
                 .output_base
                 .map(|output_base| PathBuf::from(output_base).join("external")),
+            client,
         })
     }
 
@@ -465,12 +466,12 @@ impl BazelContext {
     ) -> anyhow::Result<()> {
         // Find the actual folder on disk we're looking at.
         let (from_path, render_base) = match from {
-            FilesystemCompletionRoot::Path(path) => (path.to_owned(), path.to_string_lossy()),
+            FilesystemCompletionRoot::Path(path) => (path.to_owned(), ""),
             FilesystemCompletionRoot::String(str) => {
                 let label = Label::parse(str)?;
                 (
                     self.resolve_folder(&label, current_file, workspace_root)?,
-                    Cow::Borrowed(str),
+                    str,
                 )
             }
         };
@@ -564,18 +565,11 @@ impl BazelContext {
         module: &str,
         workspace_dir: Option<&Path>,
     ) -> Option<Vec<String>> {
-        let mut raw_command = Command::new("bazel");
-        let mut command = raw_command.arg("query").arg(format!("{module}*"));
-        if let Some(workspace_dir) = workspace_dir {
-            command = command.current_dir(workspace_dir);
-        }
-
-        let output = command.output().ok()?;
-        if !output.status.success() {
-            return None;
-        }
+        let output = self
+            .client
+            .query(workspace_dir, &format!("{module}*"))
+            .ok()?;
 
-        let output = String::from_utf8(output.stdout).ok()?;
         Some(
             output
                 .lines()
@@ -585,7 +579,7 @@ impl BazelContext {
     }
 }
 
-impl LspContext for BazelContext {
+impl<Client: BazelClient> LspContext for BazelContext<Client> {
     fn parse_file_with_contents(&self, uri: &LspUrl, content: String) -> LspEvalResult {
         match uri {
             LspUrl::File(uri) => {
@@ -837,7 +831,11 @@ impl LspContext for BazelContext {
 
 #[cfg(test)]
 mod tests {
-    use starlark_lsp::server::{LspContext, LspUrl};
+    use lsp_types::CompletionItemKind;
+    use starlark_lsp::{
+        completion::{StringCompletionResult, StringCompletionType},
+        server::{LspContext, LspUrl},
+    };
 
     use crate::test_fixture::TestFixture;
 
@@ -897,4 +895,97 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn test_completion_for_bare_targets() -> anyhow::Result<()> {
+        let fixture = TestFixture::new("simple")?;
+        let context = fixture.context()?;
+
+        let completions = context.get_string_completion_options(
+            &LspUrl::File(fixture.workspace_root().join("BUILD")),
+            StringCompletionType::String,
+            "",
+            Some(&fixture.workspace_root()),
+        )?;
+
+        let completion = completions
+            .iter()
+            .find(|completion| completion.value == "main.cc")
+            .unwrap();
+
+        assert_eq!(
+            *completion,
+            StringCompletionResult {
+                value: "main.cc".into(),
+                insert_text: Some("main.cc".into()),
+                insert_text_offset: 0,
+                kind: CompletionItemKind::FILE,
+            }
+        );
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_completion_for_files_in_package() -> anyhow::Result<()> {
+        let fixture = TestFixture::new("simple")?;
+        let context = fixture.context()?;
+
+        let completions = context.get_string_completion_options(
+            &LspUrl::File(fixture.workspace_root().join("BUILD")),
+            StringCompletionType::String,
+            "//foo:",
+            Some(&fixture.workspace_root()),
+        )?;
+
+        let completion = completions
+            .iter()
+            .find(|completion| completion.value == "main.cc")
+            .unwrap();
+
+        assert_eq!(
+            *completion,
+            StringCompletionResult {
+                value: "main.cc".into(),
+                insert_text: Some("main.cc".into()),
+                insert_text_offset: "//foo:".len(),
+                kind: CompletionItemKind::FILE,
+            }
+        );
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_completion_for_targets_in_package() -> anyhow::Result<()> {
+        let fixture = TestFixture::new("simple")?;
+        let context = fixture
+            .context_builder()?
+            .query("//foo:*", "//foo:main\n")
+            .build()?;
+
+        let completions = context.get_string_completion_options(
+            &LspUrl::File(fixture.workspace_root().join("BUILD")),
+            StringCompletionType::String,
+            "//foo:",
+            Some(&fixture.workspace_root()),
+        )?;
+
+        let completion = completions
+            .iter()
+            .find(|completion| completion.value == "main")
+            .unwrap();
+
+        assert_eq!(
+            *completion,
+            StringCompletionResult {
+                value: "main".into(),
+                insert_text: Some("main".into()),
+                insert_text_offset: "//foo:".len(),
+                kind: CompletionItemKind::PROPERTY,
+            }
+        );
+
+        Ok(())
+    }
 }

src/client.rs

@@ -1,4 +1,13 @@
-use std::{process::Command, path::{PathBuf, Path}};
+use std::{
+    path::{Path, PathBuf},
+    process::Command,
+};
+
+#[cfg(test)]
+use std::collections::HashMap;
+
+#[cfg(test)]
+use anyhow::anyhow;
 
 #[derive(Clone)]
 pub(crate) struct BazelInfo {
@@ -11,6 +20,7 @@ pub(crate) struct BazelInfo {
 /// it involves spawning a server and each invocation takes a workspace-level lock.
 pub(crate) trait BazelClient {
     fn info(&self) -> anyhow::Result<BazelInfo>;
+    fn query(&self, workspace_dir: Option<&Path>, query: &str) -> anyhow::Result<String>;
 }
 
 pub(crate) struct BazelCli {
@@ -54,16 +64,39 @@ impl BazelClient for BazelCli {
             output_base: output_base.map(|x| x.into()),
         })
     }
+
+    fn query(&self, workspace_dir: Option<&Path>, query: &str) -> anyhow::Result<String> {
+        let mut command = Command::new(&self.bazel);
+        let mut command = command.arg("query").arg(query);
+        if let Some(workspace_dir) = workspace_dir {
+            command = command.current_dir(workspace_dir)
+        }
+        let output = command.output()?;
+
+        if !output.status.success() {
+            return Err(anyhow::anyhow!("Command `bazel query` failed"));
+        }
+
+        Ok(String::from_utf8(output.stdout)?)
+    }
 }
 
 #[cfg(test)]
 pub(crate) struct MockBazel {
     pub(crate) info: BazelInfo,
+    pub(crate) queries: HashMap<String, String>,
 }
 
 #[cfg(test)]
 impl BazelClient for MockBazel {
     fn info(&self) -> anyhow::Result<BazelInfo> {
         Ok(self.info.clone())
     }
-}
+
+    fn query(&self, _workspace_dir: Option<&Path>, query: &str) -> anyhow::Result<String> {
+        self.queries
+            .get(query)
+            .map(|result| result.clone())
+            .ok_or_else(|| anyhow!("Query {} not registered in mock", query))
+    }
+}

src/test_fixture.rs

@@ -1,8 +1,16 @@
-use std::{fs, io, path::{PathBuf, Path}, collections::HashMap};
+use std::{
+    collections::HashMap,
+    fs, io,
+    path::{Path, PathBuf},
+};
 
 use anyhow::anyhow;
 
-use crate::{eval::ContextMode, bazel::BazelContext, client::{MockBazel, BazelInfo}};
+use crate::{
+    bazel::BazelContext,
+    client::{BazelInfo, MockBazel},
+    eval::ContextMode,
+};
 
 pub struct TestFixture {
     path: PathBuf,
@@ -27,18 +35,55 @@ impl TestFixture {
         self.output_base().join("external").join(repo)
     }
 
-    pub(crate) fn context(&self) -> anyhow::Result<BazelContext> {
-        let client = MockBazel {
-            info: BazelInfo {
-                output_base: Some(path_to_string(self.output_base())?),
-                execution_root: Some(path_to_string(self.output_base().join("execroot").join("root"))?),
+    pub(crate) fn context(&self) -> anyhow::Result<BazelContext<MockBazel>> {
+        self.context_builder()?.build()
+    }
+
+    pub(crate) fn context_builder(&self) -> anyhow::Result<ContextBuilder> {
+        Ok(ContextBuilder {
+            client: MockBazel {
+                info: BazelInfo {
+                    output_base: Some(path_to_string(self.output_base())?),
+                    execution_root: Some(path_to_string(
+                        self.output_base().join("execroot").join("root"),
+                    )?),
+                },
+                queries: HashMap::new(),
             },
-        };
+            mode: ContextMode::Check,
+            print_non_none: true,
+            prelude: Vec::new(),
+            module: true,
+        })
+    }
+
+
+}
+
+pub(crate) struct ContextBuilder {
+    client: MockBazel,
+    mode: ContextMode,
+    print_non_none: bool,
+    prelude: Vec<PathBuf>,
+    module: bool,
+}
+
+impl ContextBuilder {
+    pub(crate) fn query(mut self, query: &str, result: &str) -> Self {
+        self.client.queries.insert(query.into(), result.into());
+
+        self
+    }
 
-        BazelContext::new(client, ContextMode::Check, true, &[], true)
+    pub(crate) fn build(self) -> anyhow::Result<BazelContext<MockBazel>> {
+        BazelContext::new(self.client, self.mode, self.print_non_none, &self.prelude, self.module)
     }
 }
 
 fn path_to_string<P: AsRef<Path>>(path: P) -> anyhow::Result<String> {
-    Ok(path.as_ref().to_str().ok_or_else(|| anyhow!("Cannot convert path to string"))?.into())
+    Ok(path
+        .as_ref()
+        .to_str()
+        .ok_or_else(|| anyhow!("Cannot convert path to string"))?
+        .into())
 }