Fix completions for import and exports (#2640)

This pull request refactors the completion logic for imports and exports
to be more robust and accurate.

The completion provider in the language service (`global_items.rs`) has
been rewritten to leverage the `GlobalScope` and `Locals` structs
already available from the resolver. This allows completions to use the
same "view" as the name resolution code, rather than trying to recreate
it by manually analyzing imports, enumerating items, etc. All of that
work has already been done by the resolver.


The previous implementation for completions had difficulty handling more
complex import and export scenarios, such as items re-exported from
dependencies or items under aliased namespaces. This could lead to
incomplete or incorrect suggestions. The new approach provides a more
solid foundation, giving the language service a complete and accurate
view of all available symbols.

Fixes #2142

Author: minestarks

source/compiler/qsc/src/lib.rs

@@ -15,7 +15,7 @@ pub use qsc_frontend::compile::{CompileUnit, PackageStore, SourceContents, Sourc
 
 pub mod resolve {
     pub use qsc_frontend::resolve::{
-        Local, LocalKind, Locals, Res, iter_valid_items, path_as_field_accessor,
+        GlobalScope, Local, Locals, NameKind, Res, iter_valid_items, path_as_field_accessor,
     };
 }
 

source/compiler/qsc_frontend/src/compile.rs

@@ -9,7 +9,7 @@ pub mod preprocess;
 use crate::{
     error::WithSource,
     lower::{self, Lowerer},
-    resolve::{self, Locals, Names, Resolver},
+    resolve::{self, GlobalScope, Locals, Names, Resolver},
     typeck::{self, Checker, Table},
 };
 
@@ -62,6 +62,7 @@ pub struct AstPackage {
     pub tys: Table,
     pub names: Names,
     pub locals: Locals,
+    pub globals: GlobalScope,
 }
 
 #[derive(Clone, Debug, Default)]
@@ -397,6 +398,7 @@ pub fn compile_ast(
     let ResolveResult {
         names,
         locals,
+        globals,
         errors: name_errors,
     } = resolve_all(
         store,
@@ -429,6 +431,7 @@ pub fn compile_ast(
             tys,
             names,
             locals,
+            globals,
         },
         assigner: hir_assigner,
         sources,
@@ -550,6 +553,7 @@ pub fn get_target_profile_from_entry_point(
 pub(crate) struct ResolveResult {
     pub names: Names,
     pub locals: Locals,
+    pub globals: GlobalScope,
     pub errors: Vec<resolve::Error>,
 }
 
@@ -581,13 +585,14 @@ fn resolve_all(
 
     // resolve all symbols, binding imports/export names as they're resolved
     resolver.resolve(assigner, package);
-    let (names, _, locals, mut resolver_errors) = resolver.into_result();
+    let (names, globals, locals, mut resolver_errors) = resolver.into_result();
 
     errors.append(&mut resolver_errors);
 
     ResolveResult {
         names,
         locals,
+        globals,
         errors,
     }
 }

source/compiler/qsc_frontend/src/incremental.rs

@@ -190,6 +190,7 @@ impl Compiler {
                 package: ast,
                 names: self.resolver.names().clone(),
                 locals: self.resolver.locals().clone(),
+                globals: self.resolver.globals().clone(),
                 tys: self.checker.table().clone(),
             },
             hir,
@@ -229,6 +230,7 @@ impl Compiler {
                 package: ast,
                 names: self.resolver.names().clone(),
                 locals: self.resolver.locals().clone(),
+                globals: self.resolver.globals().clone(),
                 tys: self.checker.table().clone(),
             },
             hir,
@@ -245,6 +247,7 @@ impl Compiler {
         unit.ast.names = new.ast.names;
         unit.ast.tys = new.ast.tys;
         unit.ast.locals = new.ast.locals;
+        unit.ast.globals = new.ast.globals;
 
         // Update the HIR
         extend_hir(&mut unit.package, new.hir);

source/compiler/qsc_frontend/src/resolve.rs

@@ -341,7 +341,7 @@ impl Locals {
 
         // deduping by name will effectively make locals in a child scope
         // shadow the locals in its parent scopes
-        all_locals.dedup_by(|a, b| a.name == b.name);
+        all_locals.dedup_by(|a, b| a.name().is_some() && a.name() == b.name());
 
         all_locals
     }
@@ -361,22 +361,28 @@ impl Locals {
 }
 
 #[derive(Debug)]
-pub struct Local {
-    pub name: Rc<str>,
-    pub kind: LocalKind,
-}
-
-#[derive(Debug)]
-pub enum LocalKind {
+pub enum Local {
     /// A local callable or UDT.
-    Item(ItemId),
+    Item(ItemId, Rc<str>),
     /// A type parameter.
-    TyParam(ParamId),
+    TyParam(ParamId, Rc<str>),
     /// A local variable or parameter.
-    Var(NodeId),
+    Var(NodeId, Rc<str>),
+    /// A namespace import (`import Foo as A` or `open Foo`)
+    NamespaceImport(NamespaceId, Option<Rc<str>>),
 }
 
-#[derive(Debug, Default)]
+impl Local {
+    #[must_use]
+    pub fn name(&self) -> Option<&Rc<str>> {
+        match self {
+            Local::Item(_, name) | Local::TyParam(_, name) | Local::Var(_, name) => Some(name),
+            Local::NamespaceImport(_, alias) => alias.as_ref(),
+        }
+    }
+}
+
+#[derive(Debug, Default, Clone)]
 pub struct GlobalScope {
     /// Global names that are valid in a type context (UDTs, builtin types...)
     tys: IndexMap<NamespaceId, FxHashMap<Rc<str>, Res>>,
@@ -394,12 +400,17 @@ pub struct GlobalScope {
 
 impl GlobalScope {
     fn get(&self, kind: NameKind, namespace: NamespaceId, name: &str) -> Option<&Res> {
-        let items = match kind {
+        self.table(kind)
+            .get(namespace)
+            .and_then(|items| items.get(name))
+    }
+
+    pub fn table(&self, kind: NameKind) -> &IndexMap<NamespaceId, FxHashMap<Rc<str>, Res>> {
+        match kind {
             NameKind::Term => &self.terms,
             NameKind::Ty => &self.tys,
             NameKind::Importable => &self.importables,
-        };
-        items.get(namespace).and_then(|items| items.get(name))
+        }
     }
 
     /// Creates a namespace in the namespace mapping. Note that namespaces are tracked separately from their
@@ -419,7 +430,7 @@ impl GlobalScope {
     }
 
     /// Finds a namespace by its path.
-    fn find_namespace<'a>(
+    pub fn find_namespace<'a>(
         &self,
         name: impl IntoIterator<Item = &'a str>,
         root: Option<NamespaceId>,
@@ -459,12 +470,23 @@ impl GlobalScope {
     }
 
     /// Returns the full namespace path for a given namespace ID.
-    fn format_namespace_name(&self, namespace_id: NamespaceId) -> String {
+    pub fn format_namespace_name(&self, namespace_id: NamespaceId) -> String {
         self.namespaces
             .find_namespace_by_id(&namespace_id)
             .0
             .join(".")
     }
+
+    pub fn namespace_children(&self, namespace_id: NamespaceId) -> Vec<Rc<str>> {
+        self.namespaces
+            .find_namespace_by_id(&namespace_id)
+            .1
+            .borrow()
+            .children
+            .keys()
+            .cloned()
+            .collect()
+    }
 }
 
 #[derive(Debug, Clone, Eq, PartialEq)]
@@ -475,7 +497,7 @@ enum ScopeKind {
 }
 
 #[derive(Clone, Copy, Debug)]
-enum NameKind {
+pub enum NameKind {
     /// A name that is valid as an expression context: a callable or UDT
     Term,
     /// A name that is valid in a type context: builtin or UDT
@@ -590,6 +612,10 @@ impl Resolver {
         &self.locals
     }
 
+    pub(super) fn globals(&self) -> &GlobalScope {
+        &self.globals
+    }
+
     pub(super) fn drain_errors(&mut self) -> vec::Drain<Error> {
         self.errors.drain(..)
     }
@@ -1271,6 +1297,11 @@ impl GlobalTable {
                     }
                 }
                 (global::Kind::Namespace(item_id), hir::Visibility::Public) => {
+                    if package_root == Some(namespace) && global.name.as_ref() == "Main" {
+                        // If the namespace is `Main` and we have an alias, we treat it as the root of the package,
+                        // so there's no namespace prefix between the dependency alias and the defined items.
+                        continue;
+                    }
                     let this_namespace = self
                         .scope
                         .insert_or_find_namespace(vec![global.name.clone()], Some(namespace));
@@ -2014,10 +2045,7 @@ fn get_scope_locals(scope: &Scope, offset: u32, vars: bool) -> Vec<Local> {
             // when a variable is later shadowed in the same scope,
             // it is missed in the list. https://github.com/microsoft/qsharp/issues/897
             if offset >= *valid_at {
-                Some(Local {
-                    name: name.clone(),
-                    kind: LocalKind::Var(*id),
-                })
+                Some(Local::Var(*id, name.clone()))
             } else {
                 None
             }
@@ -2027,26 +2055,28 @@ fn get_scope_locals(scope: &Scope, offset: u32, vars: bool) -> Vec<Local> {
             scope
                 .ty_vars
                 .iter()
-                .map(|(name, (id, _constraints))| Local {
-                    name: name.clone(),
-                    kind: LocalKind::TyParam(*id),
-                }),
+                .map(|(name, (id, _constraints))| Local::TyParam(*id, name.clone())),
         );
     }
 
     // items
-    // skip adding newtypes since they're already in the terms map
+    // gather from the terms table, since it happens to contain all the
+    // declared and imported callables and newtypes
     names.extend(scope.terms.iter().filter_map(|(name, res)| {
         if let Res::Item(id, _) = res {
-            Some(Local {
-                name: name.clone(),
-                kind: LocalKind::Item(*id),
-            })
+            Some(Local::Item(*id, name.clone()))
         } else {
             None
         }
     }));
 
+    // opens and namespace imports
+    names.extend(scope.opens.iter().flat_map(|(alias, opens)| {
+        opens
+            .iter()
+            .map(|open| Local::NamespaceImport(open.namespace, alias.clone()))
+    }));
+
     names
 }