Merge #3049

3049: Introduce assists utils r=matklad a=matklad



Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

crates/ra_assists/src/assist_ctx.rs

@@ -19,6 +19,8 @@ pub(crate) enum Assist {
     Resolved { assist: ResolvedAssist },
 }
 
+pub(crate) type AssistHandler = fn(AssistCtx) -> Option<Assist>;
+
 /// `AssistCtx` allows to apply an assist or check if it could be applied.
 ///
 /// Assists use a somewhat over-engineered approach, given the current needs. The
@@ -57,7 +59,7 @@ pub(crate) struct AssistCtx<'a> {
     should_compute_edit: bool,
 }
 
-impl<'a> Clone for AssistCtx<'a> {
+impl Clone for AssistCtx<'_> {
     fn clone(&self) -> Self {
         AssistCtx {
             db: self.db,
@@ -69,31 +71,18 @@ impl<'a> Clone for AssistCtx<'a> {
 }
 
 impl<'a> AssistCtx<'a> {
-    pub(crate) fn with_ctx<F, T>(
+    pub fn new(db: &RootDatabase, frange: FileRange, should_compute_edit: bool) -> AssistCtx {
-        db: &RootDatabase,
-        frange: FileRange,
-        should_compute_edit: bool,
-        f: F,
-    ) -> T
-    where
-        F: FnOnce(AssistCtx) -> T,
-    {
         let parse = db.parse(frange.file_id);
-
+        AssistCtx { db, frange, source_file: parse.tree(), should_compute_edit }
-        let ctx = AssistCtx { db, frange, source_file: parse.tree(), should_compute_edit };
-        f(ctx)
     }
-}
 
-impl<'a> AssistCtx<'a> {
     pub(crate) fn add_assist(
         self,
         id: AssistId,
         label: impl Into<String>,
         f: impl FnOnce(&mut ActionBuilder),
     ) -> Option<Assist> {
-        let label = AssistLabel { label: label.into(), id };
+        let label = AssistLabel::new(label.into(), id);
-        assert!(label.label.chars().nth(0).unwrap().is_uppercase());
 
         let assist = if self.should_compute_edit {
             let action = {
@@ -115,7 +104,7 @@ impl<'a> AssistCtx<'a> {
         label: impl Into<String>,
         f: impl FnOnce() -> Vec<ActionBuilder>,
     ) -> Option<Assist> {
-        let label = AssistLabel { label: label.into(), id };
+        let label = AssistLabel::new(label.into(), id);
         let assist = if self.should_compute_edit {
             let actions = f();
             assert!(!actions.is_empty(), "Assist cannot have no");

crates/ra_assists/src/handlers/apply_demorgan.rs

@@ -1,7 +1,6 @@
-use super::invert_if::invert_boolean_expression;
 use ra_syntax::ast::{self, AstNode};
 
-use crate::{Assist, AssistCtx, AssistId};
+use crate::{utils::invert_boolean_expression, Assist, AssistCtx, AssistId};
 
 // Assist: apply_demorgan
 //

crates/ra_assists/src/handlers/auto_import.rs

@@ -1,4 +1,5 @@
 use hir::ModPath;
+use ra_ide_db::imports_locator::ImportsLocator;
 use ra_syntax::{
     ast::{self, AstNode},
     SyntaxNode,
@@ -8,7 +9,7 @@ use crate::{
     assist_ctx::{ActionBuilder, Assist, AssistCtx},
     auto_import_text_edit, AssistId,
 };
-use ra_ide_db::imports_locator::ImportsLocator;
+use std::collections::BTreeSet;
 
 // Assist: auto_import
 //
@@ -60,7 +61,8 @@ pub(crate) fn auto_import(ctx: AssistCtx) -> Option<Assist> {
         .filter_map(|module_def| module_with_name_to_import.find_use_path(ctx.db, module_def))
         .filter(|use_path| !use_path.segments.is_empty())
         .take(20)
-        .collect::<std::collections::BTreeSet<_>>();
+        .collect::<BTreeSet<_>>();
+
     if proposed_imports.is_empty() {
         return None;
     }
@@ -82,9 +84,10 @@ fn import_to_action(import: ModPath, position: &SyntaxNode, anchor: &SyntaxNode)
 
 #[cfg(test)]
 mod tests {
-    use super::*;
     use crate::helpers::{check_assist, check_assist_not_applicable};
 
+    use super::*;
+
     #[test]
     fn applicable_when_found_an_import() {
         check_assist(

crates/ra_assists/src/handlers/early_return.rs

@@ -10,7 +10,7 @@ use ra_syntax::{
 
 use crate::{
     assist_ctx::{Assist, AssistCtx},
-    assists::invert_if::invert_boolean_expression,
+    utils::invert_boolean_expression,
     AssistId,
 };
 

crates/ra_assists/src/handlers/invert_if.rs

@@ -1,7 +1,7 @@
-use ra_syntax::ast::{self, make, AstNode};
+use ra_syntax::ast::{self, AstNode};
 use ra_syntax::T;
 
-use crate::{Assist, AssistCtx, AssistId};
+use crate::{utils::invert_boolean_expression, Assist, AssistCtx, AssistId};
 
 // Assist: invert_if
 //
@@ -51,27 +51,6 @@ pub(crate) fn invert_if(ctx: AssistCtx) -> Option<Assist> {
     None
 }
 
-pub(crate) fn invert_boolean_expression(expr: ast::Expr) -> ast::Expr {
-    if let Some(expr) = invert_special_case(&expr) {
-        return expr;
-    }
-    make::expr_prefix(T![!], expr)
-}
-
-pub(crate) fn invert_special_case(expr: &ast::Expr) -> Option<ast::Expr> {
-    match expr {
-        ast::Expr::BinExpr(bin) => match bin.op_kind()? {
-            ast::BinOp::NegatedEqualityTest => bin.replace_op(T![==]).map(|it| it.into()),
-            ast::BinOp::EqualityTest => bin.replace_op(T![!=]).map(|it| it.into()),
-            _ => None,
-        },
-        ast::Expr::PrefixExpr(pe) if pe.op_kind()? == ast::PrefixOp::Not => pe.expr(),
-        // FIXME:
-        // ast::Expr::Literal(true | false )
-        _ => None,
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

crates/ra_assists/src/lib.rs

@@ -9,16 +9,19 @@ mod assist_ctx;
 mod marks;
 #[cfg(test)]
 mod doc_tests;
+mod utils;
 pub mod ast_transform;
 
+use std::cmp::Ordering;
+
 use either::Either;
 use ra_db::FileRange;
 use ra_ide_db::RootDatabase;
 use ra_syntax::{TextRange, TextUnit};
 use ra_text_edit::TextEdit;
 
-pub(crate) use crate::assist_ctx::{Assist, AssistCtx};
+pub(crate) use crate::assist_ctx::{Assist, AssistCtx, AssistHandler};
-pub use crate::assists::add_import::auto_import_text_edit;
+pub use crate::handlers::add_import::auto_import_text_edit;
 
 /// Unique identifier of the assist, should not be shown to the user
 /// directly.
@@ -32,11 +35,20 @@ pub struct AssistLabel {
     pub id: AssistId,
 }
 
+impl AssistLabel {
+    pub(crate) fn new(label: String, id: AssistId) -> AssistLabel {
+        // FIXME: make fields private, so that this invariant can't be broken
+        assert!(label.chars().nth(0).unwrap().is_uppercase());
+        AssistLabel { label: label.into(), id }
+    }
+}
+
 #[derive(Debug, Clone)]
 pub struct AssistAction {
     pub label: Option<String>,
     pub edit: TextEdit,
     pub cursor_position: Option<TextUnit>,
+    // FIXME: This belongs to `AssistLabel`
     pub target: Option<TextRange>,
 }
 
@@ -60,16 +72,15 @@ impl ResolvedAssist {
 /// Assists are returned in the "unresolved" state, that is only labels are
 /// returned, without actual edits.
 pub fn unresolved_assists(db: &RootDatabase, range: FileRange) -> Vec<AssistLabel> {
-    AssistCtx::with_ctx(db, range, false, |ctx| {
+    let ctx = AssistCtx::new(db, range, false);
-        assists::all()
+    handlers::all()
-            .iter()
+        .iter()
-            .filter_map(|f| f(ctx.clone()))
+        .filter_map(|f| f(ctx.clone()))
-            .map(|a| match a {
+        .map(|a| match a {
-                Assist::Unresolved { label } => label,
+            Assist::Unresolved { label } => label,
-                Assist::Resolved { .. } => unreachable!(),
+            Assist::Resolved { .. } => unreachable!(),
-            })
+        })
-            .collect()
+        .collect()
-    })
 }
 
 /// Return all the assists applicable at the given position.
@@ -77,22 +88,20 @@ pub fn unresolved_assists(db: &RootDatabase, range: FileRange) -> Vec<AssistLabe
 /// Assists are returned in the "resolved" state, that is with edit fully
 /// computed.
 pub fn resolved_assists(db: &RootDatabase, range: FileRange) -> Vec<ResolvedAssist> {
-    AssistCtx::with_ctx(db, range, true, |ctx| {
+    let ctx = AssistCtx::new(db, range, true);
-        let mut a = assists::all()
+    let mut a = handlers::all()
-            .iter()
+        .iter()
-            .filter_map(|f| f(ctx.clone()))
+        .filter_map(|f| f(ctx.clone()))
-            .map(|a| match a {
+        .map(|a| match a {
-                Assist::Resolved { assist } => assist,
+            Assist::Resolved { assist } => assist,
-                Assist::Unresolved { .. } => unreachable!(),
+            Assist::Unresolved { .. } => unreachable!(),
-            })
+        })
-            .collect();
+        .collect::<Vec<_>>();
-        sort_assists(&mut a);
+    sort_assists(&mut a);
-        a
+    a
-    })
 }
 
-fn sort_assists(assists: &mut Vec<ResolvedAssist>) {
+fn sort_assists(assists: &mut [ResolvedAssist]) {
-    use std::cmp::Ordering;
     assists.sort_by(|a, b| match (a.get_first_action().target, b.get_first_action().target) {
         (Some(a), Some(b)) => a.len().cmp(&b.len()),
         (Some(_), None) => Ordering::Less,
@@ -101,8 +110,8 @@ fn sort_assists(assists: &mut Vec<ResolvedAssist>) {
     });
 }
 
-mod assists {
+mod handlers {
-    use crate::{Assist, AssistCtx};
+    use crate::AssistHandler;
 
     mod add_derive;
     mod add_explicit_type;
@@ -130,7 +139,7 @@ mod assists {
     mod move_bounds;
     mod early_return;
 
-    pub(crate) fn all() -> &'static [fn(AssistCtx) -> Option<Assist>] {
+    pub(crate) fn all() -> &'static [AssistHandler] {
         &[
             add_derive::add_derive,
             add_explicit_type::add_explicit_type,
@@ -175,7 +184,7 @@ mod helpers {
     use ra_syntax::TextRange;
     use test_utils::{add_cursor, assert_eq_text, extract_offset, extract_range};
 
-    use crate::{Assist, AssistCtx};
+    use crate::{Assist, AssistCtx, AssistHandler};
 
     pub(crate) fn with_single_file(text: &str) -> (RootDatabase, FileId) {
         let (mut db, file_id) = RootDatabase::with_single_file(text);
@@ -186,13 +195,13 @@ mod helpers {
         (db, file_id)
     }
 
-    pub(crate) fn check_assist(assist: fn(AssistCtx) -> Option<Assist>, before: &str, after: &str) {
+    pub(crate) fn check_assist(assist: AssistHandler, before: &str, after: &str) {
         let (before_cursor_pos, before) = extract_offset(before);
         let (db, file_id) = with_single_file(&before);
         let frange =
             FileRange { file_id, range: TextRange::offset_len(before_cursor_pos, 0.into()) };
         let assist =
-            AssistCtx::with_ctx(&db, frange, true, assist).expect("code action is not applicable");
+            assist(AssistCtx::new(&db, frange, true)).expect("code action is not applicable");
         let action = match assist {
             Assist::Unresolved { .. } => unreachable!(),
             Assist::Resolved { assist } => assist.get_first_action(),
@@ -210,16 +219,12 @@ mod helpers {
         assert_eq_text!(after, &actual);
     }
 
-    pub(crate) fn check_assist_range(
+    pub(crate) fn check_assist_range(assist: AssistHandler, before: &str, after: &str) {
-        assist: fn(AssistCtx) -> Option<Assist>,
-        before: &str,
-        after: &str,
-    ) {
         let (range, before) = extract_range(before);
         let (db, file_id) = with_single_file(&before);
         let frange = FileRange { file_id, range };
         let assist =
-            AssistCtx::with_ctx(&db, frange, true, assist).expect("code action is not applicable");
+            assist(AssistCtx::new(&db, frange, true)).expect("code action is not applicable");
         let action = match assist {
             Assist::Unresolved { .. } => unreachable!(),
             Assist::Resolved { assist } => assist.get_first_action(),
@@ -232,17 +237,13 @@ mod helpers {
         assert_eq_text!(after, &actual);
     }
 
-    pub(crate) fn check_assist_target(
+    pub(crate) fn check_assist_target(assist: AssistHandler, before: &str, target: &str) {
-        assist: fn(AssistCtx) -> Option<Assist>,
-        before: &str,
-        target: &str,
-    ) {
         let (before_cursor_pos, before) = extract_offset(before);
         let (db, file_id) = with_single_file(&before);
         let frange =
             FileRange { file_id, range: TextRange::offset_len(before_cursor_pos, 0.into()) };
         let assist =
-            AssistCtx::with_ctx(&db, frange, true, assist).expect("code action is not applicable");
+            assist(AssistCtx::new(&db, frange, true)).expect("code action is not applicable");
         let action = match assist {
             Assist::Unresolved { .. } => unreachable!(),
             Assist::Resolved { assist } => assist.get_first_action(),
@@ -252,16 +253,12 @@ mod helpers {
         assert_eq_text!(&before[range.start().to_usize()..range.end().to_usize()], target);
     }
 
-    pub(crate) fn check_assist_range_target(
+    pub(crate) fn check_assist_range_target(assist: AssistHandler, before: &str, target: &str) {
-        assist: fn(AssistCtx) -> Option<Assist>,
-        before: &str,
-        target: &str,
-    ) {
         let (range, before) = extract_range(before);
         let (db, file_id) = with_single_file(&before);
         let frange = FileRange { file_id, range };
         let assist =
-            AssistCtx::with_ctx(&db, frange, true, assist).expect("code action is not applicable");
+            assist(AssistCtx::new(&db, frange, true)).expect("code action is not applicable");
         let action = match assist {
             Assist::Unresolved { .. } => unreachable!(),
             Assist::Resolved { assist } => assist.get_first_action(),
@@ -271,26 +268,20 @@ mod helpers {
         assert_eq_text!(&before[range.start().to_usize()..range.end().to_usize()], target);
     }
 
-    pub(crate) fn check_assist_not_applicable(
+    pub(crate) fn check_assist_not_applicable(assist: AssistHandler, before: &str) {
-        assist: fn(AssistCtx) -> Option<Assist>,
-        before: &str,
-    ) {
         let (before_cursor_pos, before) = extract_offset(before);
         let (db, file_id) = with_single_file(&before);
         let frange =
             FileRange { file_id, range: TextRange::offset_len(before_cursor_pos, 0.into()) };
-        let assist = AssistCtx::with_ctx(&db, frange, true, assist);
+        let assist = assist(AssistCtx::new(&db, frange, true));
         assert!(assist.is_none());
     }
 
-    pub(crate) fn check_assist_range_not_applicable(
+    pub(crate) fn check_assist_range_not_applicable(assist: AssistHandler, before: &str) {
-        assist: fn(AssistCtx) -> Option<Assist>,
-        before: &str,
-    ) {
         let (range, before) = extract_range(before);
         let (db, file_id) = with_single_file(&before);
         let frange = FileRange { file_id, range };
-        let assist = AssistCtx::with_ctx(&db, frange, true, assist);
+        let assist = assist(AssistCtx::new(&db, frange, true));
         assert!(assist.is_none());
     }
 }

crates/ra_assists/src/utils.rs

@@ -0,0 +1,27 @@
+//! Assorted functions shared by several assists.
+
+use ra_syntax::{
+    ast::{self, make},
+    T,
+};
+
+pub(crate) fn invert_boolean_expression(expr: ast::Expr) -> ast::Expr {
+    if let Some(expr) = invert_special_case(&expr) {
+        return expr;
+    }
+    make::expr_prefix(T![!], expr)
+}
+
+fn invert_special_case(expr: &ast::Expr) -> Option<ast::Expr> {
+    match expr {
+        ast::Expr::BinExpr(bin) => match bin.op_kind()? {
+            ast::BinOp::NegatedEqualityTest => bin.replace_op(T![==]).map(|it| it.into()),
+            ast::BinOp::EqualityTest => bin.replace_op(T![!=]).map(|it| it.into()),
+            _ => None,
+        },
+        ast::Expr::PrefixExpr(pe) if pe.op_kind()? == ast::PrefixOp::Not => pe.expr(),
+        // FIXME:
+        // ast::Expr::Literal(true | false )
+        _ => None,
+    }
+}

xtask/src/codegen.rs

@@ -25,7 +25,7 @@ const ERR_INLINE_TESTS_DIR: &str = "crates/ra_syntax/test_data/parser/inline/err
 pub const SYNTAX_KINDS: &str = "crates/ra_parser/src/syntax_kind/generated.rs";
 pub const AST: &str = "crates/ra_syntax/src/ast/generated.rs";
 
-const ASSISTS_DIR: &str = "crates/ra_assists/src/assists";
+const ASSISTS_DIR: &str = "crates/ra_assists/src/handlers";
 const ASSISTS_TESTS: &str = "crates/ra_assists/src/doc_tests/generated.rs";
 const ASSISTS_DOCS: &str = "docs/user/assists.md";
 

xtask/tests/tidy-tests/docs.rs

@@ -6,7 +6,7 @@ use xtask::project_root;
 fn is_exclude_dir(p: &Path) -> bool {
     // Test hopefully don't really need comments, and for assists we already
     // have special comments which are source of doc tests and user docs.
-    let exclude_dirs = ["tests", "test_data", "assists"];
+    let exclude_dirs = ["tests", "test_data", "handlers"];
     let mut cur_path = p;
     while let Some(path) = cur_path.parent() {
         if exclude_dirs.iter().any(|dir| path.ends_with(dir)) {