Merge #10523

10523: minor: Simplify and add a few doc comments r=Veykril a=Veykril

bors r+

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

crates/ide_completion/src/completions/postfix.rs

@@ -19,7 +19,7 @@ use crate::{
     context::CompletionContext,
     item::{Builder, CompletionKind},
     patterns::ImmediateLocation,
-    CompletionItem, CompletionItemKind, CompletionRelevance, Completions,
+    CompletionItem, CompletionItemKind, CompletionRelevance, Completions, SnippetScope,
 };
 
 pub(crate) fn complete_postfix(acc: &mut Completions, ctx: &CompletionContext) {
@@ -231,7 +231,7 @@ fn add_custom_postfix_completions(
 ) -> Option<()> {
     let import_scope =
         ImportScope::find_insert_use_container_with_macros(&ctx.token.parent()?, &ctx.sema)?;
-    ctx.config.postfix_snippets().filter(|(_, snip)| snip.is_expr()).for_each(
+    ctx.config.postfix_snippets().filter(|(_, snip)| snip.scope == SnippetScope::Expr).for_each(
         |(trigger, snippet)| {
             let imports = match snippet.imports(ctx, &import_scope) {
                 Some(imports) => imports,

crates/ide_completion/src/config.rs

@@ -22,13 +22,14 @@ pub struct CompletionConfig {
 
 impl CompletionConfig {
     pub fn postfix_snippets(&self) -> impl Iterator<Item = (&str, &Snippet)> {
-        self.snippets.iter().flat_map(|snip| {
-            snip.postfix_triggers.iter().map(move |trigger| (trigger.as_str(), snip))
-        })
+        self.snippets
+            .iter()
+            .flat_map(|snip| snip.postfix_triggers.iter().map(move |trigger| (&**trigger, snip)))
     }
+
     pub fn prefix_snippets(&self) -> impl Iterator<Item = (&str, &Snippet)> {
-        self.snippets.iter().flat_map(|snip| {
-            snip.prefix_triggers.iter().map(move |trigger| (trigger.as_str(), snip))
-        })
+        self.snippets
+            .iter()
+            .flat_map(|snip| snip.prefix_triggers.iter().map(move |trigger| (&**trigger, snip)))
     }
 }

crates/ide_completion/src/context.rs

@@ -85,6 +85,7 @@ pub(crate) struct CompletionContext<'a> {
     pub(super) original_token: SyntaxToken,
     /// The token before the cursor, in the macro-expanded file.
     pub(super) token: SyntaxToken,
+    /// The crate of the current file.
     pub(super) krate: Option<hir::Crate>,
     pub(super) expected_name: Option<NameOrNameRef>,
     pub(super) expected_type: Option<Type>,
@@ -135,11 +136,11 @@ impl<'a> CompletionContext<'a> {
         let fake_ident_token =
             file_with_fake_ident.syntax().token_at_offset(position.offset).right_biased().unwrap();
 
-        let krate = sema.to_module_def(position.file_id).map(|m| m.krate());
         let original_token =
             original_file.syntax().token_at_offset(position.offset).left_biased()?;
         let token = sema.descend_into_macros(original_token.clone());
         let scope = sema.scope_at_offset(&token, position.offset);
+        let krate = scope.krate();
         let mut locals = vec![];
         scope.process_all_names(&mut |name, scope| {
             if let ScopeDef::Local(local) = scope {
@@ -182,6 +183,8 @@ impl<'a> CompletionContext<'a> {
         Some(ctx)
     }
 
+    /// Do the attribute expansion at the current cursor position for both original file and fake file
+    /// as long as possible. As soon as one of the two expansions fail we stop to stay in sync.
     fn expand_and_fill(
         &mut self,
         mut original_file: SyntaxNode,
@@ -428,6 +431,7 @@ impl<'a> CompletionContext<'a> {
         false
     }
 
+    /// Check if an item is `#[doc(hidden)]`.
     pub(crate) fn is_item_hidden(&self, item: &hir::ItemInNs) -> bool {
         let attrs = item.attrs(self.db);
         let krate = item.krate(self.db);
@@ -474,11 +478,11 @@ impl<'a> CompletionContext<'a> {
     }
 
     fn is_doc_hidden(&self, attrs: &hir::Attrs, defining_crate: hir::Crate) -> bool {
-        let module = match self.scope.module() {
+        let krate = match self.krate {
             Some(it) => it,
             None => return true,
         };
-        if module.krate() != defining_crate && attrs.has_doc_hidden() {
+        if krate != defining_crate && attrs.has_doc_hidden() {
             // `doc(hidden)` items are only completed within the defining crate.
             return true;
         }

crates/ide_completion/src/item.rs

@@ -218,6 +218,7 @@ impl CompletionRelevance {
     }
 }
 
+/// The type of the completion item.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum CompletionItemKind {
     SymbolKind(SymbolKind),
@@ -269,6 +270,8 @@ impl CompletionItemKind {
     }
 }
 
+// FIXME remove this?
+/// Like [`CompletionItemKind`] but solely used for filtering test results.
 #[derive(Debug, PartialEq, Eq, Copy, Clone)]
 pub(crate) enum CompletionKind {
     /// Parser-based keyword completion.
@@ -477,9 +480,10 @@ impl Builder {
     }
     pub(crate) fn insert_snippet(
         &mut self,
-        _cap: SnippetCap,
+        cap: SnippetCap,
         snippet: impl Into<String>,
     ) -> &mut Builder {
+        let _ = cap;
         self.is_snippet = true;
         self.insert_text(snippet)
     }

crates/ide_completion/src/lib.rs

@@ -173,6 +173,7 @@ pub fn completions(
 }
 
 /// Resolves additional completion data at the position given.
+/// This is used for import insertion done via completions like flyimport and custom user snippets.
 pub fn resolve_completion_edits(
     db: &RootDatabase,
     config: &CompletionConfig,

crates/ide_completion/src/patterns.rs

@@ -28,6 +28,9 @@ pub(crate) enum ImmediatePrevSibling {
 }
 
 /// Direct parent "thing" of what we are currently completing.
+///
+/// This may contain nodes of the fake file as well as the original, comments on the variants specify
+/// from which file the nodes are.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub(crate) enum ImmediateLocation {
     Use,
@@ -42,32 +45,35 @@ pub(crate) enum ImmediateLocation {
     StmtList,
     ItemList,
     TypeBound,
-    // Fake file ast node
+    /// Fake file ast node
     Attribute(ast::Attr),
-    // Fake file ast node
+    /// Fake file ast node
     ModDeclaration(ast::Module),
     Visibility(ast::Visibility),
-    // Original file ast node
+    /// Original file ast node
     MethodCall {
         receiver: Option<ast::Expr>,
         has_parens: bool,
     },
-    // Original file ast node
+    /// Original file ast node
     FieldAccess {
         receiver: Option<ast::Expr>,
         receiver_is_ambiguous_float_literal: bool,
     },
-    // Original file ast node
     // Only set from a type arg
+    /// Original file ast node
     GenericArgList(ast::GenericArgList),
-    // Original file ast node
     /// The record expr of the field name we are completing
+    ///
+    /// Original file ast node
     RecordExpr(ast::RecordExpr),
-    // Original file ast node
     /// The record expr of the functional update syntax we are completing
+    ///
+    /// Original file ast node
     RecordExprUpdate(ast::RecordExpr),
-    // Original file ast node
     /// The record pat of the field name we are completing
+    ///
+    /// Original file ast node
     RecordPat(ast::RecordPat),
 }
 
@@ -268,17 +274,20 @@ pub(crate) fn determine_location(
     Some(res)
 }
 
+/// Maximize a nameref to its enclosing path if its the last segment of said path.
+/// That is, when completing a [`NameRef`] we actually handle it as the path it is part of when determining
+/// its location.
 fn maximize_name_ref(name_ref: &ast::NameRef) -> SyntaxNode {
-    // Maximize a nameref to its enclosing path if its the last segment of said path
     if let Some(segment) = name_ref.syntax().parent().and_then(ast::PathSegment::cast) {
         let p = segment.parent_path();
         if p.parent_path().is_none() {
-            if let Some(it) = p
+            // Get rid of PathExpr, PathType, etc...
+            let path = p
                 .syntax()
                 .ancestors()
                 .take_while(|it| it.text_range() == p.syntax().text_range())
-                .last()
-            {
+                .last();
+            if let Some(it) = path {
                 return it;
             }
         }

crates/ide_completion/src/snippet.rs

@@ -1,6 +1,8 @@
 //! User (postfix)-snippet definitions.
 //!
-//! Actual logic is implemented in [`crate::completions::postfix`] and [`crate::completions::snippet`].
+//! Actual logic is implemented in [`crate::completions::postfix`] and [`crate::completions::snippet`] respectively.
+
+use std::ops::Deref;
 
 // Feature: User Snippet Completions
 //
@@ -58,6 +60,8 @@ use syntax::ast;
 
 use crate::{context::CompletionContext, ImportEdit};
 
+/// A snippet scope describing where a snippet may apply to.
+/// These may differ slightly in meaning depending on the snippet trigger.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum SnippetScope {
     Item,
@@ -65,14 +69,15 @@ pub enum SnippetScope {
     Type,
 }
 
+/// A user supplied snippet.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct Snippet {
-    pub postfix_triggers: Box<[String]>,
-    pub prefix_triggers: Box<[String]>,
+    pub postfix_triggers: Box<[Box<str>]>,
+    pub prefix_triggers: Box<[Box<str>]>,
     pub scope: SnippetScope,
     snippet: String,
-    pub description: Option<String>,
-    pub requires: Box<[String]>,
+    pub description: Option<Box<str>>,
+    pub requires: Box<[Box<str>]>,
 }
 
 impl Snippet {
@@ -84,19 +89,22 @@ impl Snippet {
         requires: &[String],
         scope: SnippetScope,
     ) -> Option<Self> {
+        if prefix_triggers.is_empty() && postfix_triggers.is_empty() {
+            return None;
+        }
         let (snippet, description) = validate_snippet(snippet, description, requires)?;
         Some(Snippet {
             // Box::into doesn't work as that has a Copy bound 😒
-            postfix_triggers: postfix_triggers.iter().cloned().collect(),
-            prefix_triggers: prefix_triggers.iter().cloned().collect(),
+            postfix_triggers: postfix_triggers.iter().map(Deref::deref).map(Into::into).collect(),
+            prefix_triggers: prefix_triggers.iter().map(Deref::deref).map(Into::into).collect(),
             scope,
             snippet,
             description,
-            requires: requires.iter().cloned().collect(),
+            requires: requires.iter().map(Deref::deref).map(Into::into).collect(),
         })
     }
 
-    /// Returns None if the required items do not resolve.
+    /// Returns [`None`] if the required items do not resolve.
     pub(crate) fn imports(
         &self,
         ctx: &CompletionContext,
@@ -112,20 +120,12 @@ impl Snippet {
     pub fn postfix_snippet(&self, receiver: &str) -> String {
         self.snippet.replace("${receiver}", receiver)
     }
-
-    pub fn is_item(&self) -> bool {
-        self.scope == SnippetScope::Item
-    }
-
-    pub fn is_expr(&self) -> bool {
-        self.scope == SnippetScope::Expr
-    }
 }
 
 fn import_edits(
     ctx: &CompletionContext,
     import_scope: &ImportScope,
-    requires: &[String],
+    requires: &[Box<str>],
 ) -> Option<Vec<ImportEdit>> {
     let resolve = |import| {
         let path = ast::Path::parse(import).ok()?;
@@ -158,7 +158,7 @@ fn validate_snippet(
     snippet: &[String],
     description: &str,
     requires: &[String],
-) -> Option<(String, Option<String>)> {
+) -> Option<(String, Option<Box<str>>)> {
     // validate that these are indeed simple paths
     // we can't save the paths unfortunately due to them not being Send+Sync
     if requires.iter().any(|path| match ast::Path::parse(path) {
@@ -171,6 +171,6 @@ fn validate_snippet(
         return None;
     }
     let snippet = snippet.iter().join("\n");
-    let description = if description.is_empty() { None } else { Some(description.to_owned()) };
+    let description = if description.is_empty() { None } else { Some(description.into()) };
     Some((snippet, description))
 }