Merge #495

495: Fix on type handlers r=matklad a=matklad

Looks like our on type handlers didn't actually worked, this shoud fix that!

Co-authored-by: Aleksey Kladov <[email protected]>

4 files changed, 174 insertions, 178 deletions

diff --git a/crates/ra_ide_api_light/src/assists.rs b/crates/ra_ide_api_light/src/assists.rs
index 83eabfc85..3495ad967 100644
--- a/crates/ra_ide_api_light/src/assists.rs
+++ b/crates/ra_ide_api_light/src/assists.rs
@@ -15,10 +15,11 @@ use ra_text_edit::{TextEdit, TextEditBuilder};
 use ra_syntax::{
     Direction, SyntaxNode, TextUnit, TextRange, SourceFile, AstNode,
     algo::{find_leaf_at_offset, find_node_at_offset, find_covering_node, LeafAtOffset},
-    ast::{self, AstToken},
 };
 use itertools::Itertools;
 
+use crate::formatting::leading_indent;
+
 pub use self::{
     flip_comma::flip_comma,
     add_derive::add_derive,
@@ -165,7 +166,7 @@ impl AssistBuilder {
     }
     fn replace_node_and_indent(&mut self, node: &SyntaxNode, replace_with: impl Into<String>) {
         let mut replace_with = replace_with.into();
-        if let Some(indent) = calc_indent(node) {
+        if let Some(indent) = leading_indent(node) {
             replace_with = reindent(&replace_with, indent)
         }
         self.replace(node.range(), replace_with)
@@ -182,12 +183,6 @@ impl AssistBuilder {
     }
 }
 
-fn calc_indent(node: &SyntaxNode) -> Option<&str> {
-    let prev = node.prev_sibling()?;
-    let ws_text = ast::Whitespace::cast(prev)?.text();
-    ws_text.rfind('\n').map(|pos| &ws_text[pos + 1..])
-}
-
 fn reindent(text: &str, indent: &str) -> String {
     let indent = format!("\n{}", indent);
     text.lines().intersperse(&indent).collect()
diff --git a/crates/ra_ide_api_light/src/formatting.rs b/crates/ra_ide_api_light/src/formatting.rs
index 1f3769209..4635fbd60 100644
--- a/crates/ra_ide_api_light/src/formatting.rs
+++ b/crates/ra_ide_api_light/src/formatting.rs
@@ -1,8 +1,16 @@
 use ra_syntax::{
-    ast, AstNode,
+    AstNode,
     SyntaxNode, SyntaxKind::*,
+    ast::{self, AstToken},
 };
 
+/// If the node is on the begining of the line, calculate indent.
+pub(crate) fn leading_indent(node: &SyntaxNode) -> Option<&str> {
+    let prev = node.prev_sibling()?;
+    let ws_text = ast::Whitespace::cast(prev)?.text();
+    ws_text.rfind('\n').map(|pos| &ws_text[pos + 1..])
+}
+
 pub(crate) fn extract_trivial_expression(block: &ast::Block) -> Option<&ast::Expr> {
     let expr = block.expr()?;
     if expr.syntax().text().contains('\n') {
diff --git a/crates/ra_ide_api_light/src/typing.rs b/crates/ra_ide_api_light/src/typing.rs
index 5726209cc..c8f3dfe44 100644
--- a/crates/ra_ide_api_light/src/typing.rs
+++ b/crates/ra_ide_api_light/src/typing.rs
@@ -1,11 +1,11 @@
 use ra_syntax::{
+    AstNode, SourceFile, SyntaxKind::*,
+    SyntaxNode, TextUnit, TextRange,
     algo::{find_node_at_offset, find_leaf_at_offset, LeafAtOffset},
-    ast,
-    AstNode, Direction, SourceFile, SyntaxKind::*,
-    SyntaxNode, TextUnit,
+    ast::{self, AstToken},
 };
 
-use crate::{LocalEdit, TextEditBuilder};
+use crate::{LocalEdit, TextEditBuilder, formatting::leading_indent};
 
 pub fn on_enter(file: &SourceFile, offset: TextUnit) -> Option<LocalEdit> {
     let comment = find_leaf_at_offset(file.syntax(), offset)
@@ -53,20 +53,21 @@ fn node_indent<'a>(file: &'a SourceFile, node: &SyntaxNode) -> Option<&'a str> {
     Some(&text[pos..])
 }
 
-pub fn on_eq_typed(file: &SourceFile, offset: TextUnit) -> Option<LocalEdit> {
-    let let_stmt: &ast::LetStmt = find_node_at_offset(file.syntax(), offset)?;
+pub fn on_eq_typed(file: &SourceFile, eq_offset: TextUnit) -> Option<LocalEdit> {
+    assert_eq!(file.syntax().text().char_at(eq_offset), Some('='));
+    let let_stmt: &ast::LetStmt = find_node_at_offset(file.syntax(), eq_offset)?;
     if let_stmt.has_semi() {
         return None;
     }
     if let Some(expr) = let_stmt.initializer() {
         let expr_range = expr.syntax().range();
-        if expr_range.contains(offset) && offset != expr_range.start() {
+        if expr_range.contains(eq_offset) && eq_offset != expr_range.start() {
             return None;
         }
         if file
             .syntax()
             .text()
-            .slice(offset..expr_range.start())
+            .slice(eq_offset..expr_range.start())
             .contains('\n')
         {
             return None;
@@ -84,54 +85,44 @@ pub fn on_eq_typed(file: &SourceFile, offset: TextUnit) -> Option<LocalEdit> {
     })
 }
 
-pub fn on_dot_typed(file: &SourceFile, offset: TextUnit) -> Option<LocalEdit> {
-    let before_dot_offset = offset - TextUnit::of_char('.');
+pub fn on_dot_typed(file: &SourceFile, dot_offset: TextUnit) -> Option<LocalEdit> {
+    assert_eq!(file.syntax().text().char_at(dot_offset), Some('.'));
 
-    let whitespace = find_leaf_at_offset(file.syntax(), before_dot_offset).left_biased()?;
-
-    // find whitespace just left of the dot
-    ast::Whitespace::cast(whitespace)?;
-
-    // make sure there is a method call
-    let method_call = whitespace
-        .siblings(Direction::Prev)
-        // first is whitespace
-        .skip(1)
-        .next()?;
-
-    ast::MethodCallExpr::cast(method_call)?;
-
-    // find how much the _method call is indented
-    let method_chain_indent = method_call
-        .parent()?
-        .siblings(Direction::Prev)
-        .skip(1)
-        .next()?
-        .leaf_text()
-        .map(|x| last_line_indent_in_whitespace(x))?;
-
-    let current_indent = TextUnit::of_str(last_line_indent_in_whitespace(whitespace.leaf_text()?));
-    // TODO: indent is always 4 spaces now. A better heuristic could look on the previous line(s)
-
-    let target_indent = TextUnit::of_str(method_chain_indent) + TextUnit::from_usize(4);
-
-    let diff = target_indent - current_indent;
-
-    let indent = "".repeat(diff.to_usize());
+    let whitespace = find_leaf_at_offset(file.syntax(), dot_offset)
+        .left_biased()
+        .and_then(ast::Whitespace::cast)?;
 
-    let cursor_position = offset + diff;
+    let current_indent = {
+        let text = whitespace.text();
+        let newline = text.rfind('\n')?;
+        &text[newline + 1..]
+    };
+    let current_indent_len = TextUnit::of_str(current_indent);
+
+    // Make sure dot is a part of call chain
+    let field_expr = whitespace
+        .syntax()
+        .parent()
+        .and_then(ast::FieldExpr::cast)?;
+    let prev_indent = leading_indent(field_expr.syntax())?;
+    let target_indent = format!("    {}", prev_indent);
+    let target_indent_len = TextUnit::of_str(&target_indent);
+    if current_indent_len == target_indent_len {
+        return None;
+    }
     let mut edit = TextEditBuilder::default();
-    edit.insert(before_dot_offset, indent);
-    Some(LocalEdit {
-        label: "indent dot".to_string(),
+    edit.replace(
+        TextRange::from_to(dot_offset - current_indent_len, dot_offset),
+        target_indent.into(),
+    );
+    let res = LocalEdit {
+        label: "reindent dot".to_string(),
         edit: edit.finish(),
-        cursor_position: Some(cursor_position),
-    })
-}
-
-/// Finds the last line in the whitespace
-fn last_line_indent_in_whitespace(ws: &str) -> &str {
-    ws.split('\n').last().unwrap_or("")
+        cursor_position: Some(
+            dot_offset + target_indent_len - current_indent_len + TextUnit::of_char('.'),
+        ),
+    };
+    Some(res)
 }
 
 #[cfg(test)]
@@ -142,12 +133,18 @@ mod tests {
 
     #[test]
     fn test_on_eq_typed() {
-        fn do_check(before: &str, after: &str) {
+        fn type_eq(before: &str, after: &str) {
             let (offset, before) = extract_offset(before);
+            let mut edit = TextEditBuilder::default();
+            edit.insert(offset, "=".to_string());
+            let before = edit.finish().apply(&before);
             let file = SourceFile::parse(&before);
-            let result = on_eq_typed(&file, offset).unwrap();
-            let actual = result.edit.apply(&before);
-            assert_eq_text!(after, &actual);
+            if let Some(result) = on_eq_typed(&file, offset) {
+                let actual = result.edit.apply(&before);
+                assert_eq_text!(after, &actual);
+            } else {
+                assert_eq_text!(&before, after)
+            };
         }
 
         //     do_check(r"
@@ -159,10 +156,10 @@ mod tests {
         //     let foo =;
         // }
         // ");
-        do_check(
+        type_eq(
             r"
 fn foo() {
-    let foo =<|> 1 + 1
+    let foo <|> 1 + 1
 }
 ",
             r"
@@ -184,112 +181,113 @@ fn foo() {
         // ");
     }
 
+    fn type_dot(before: &str, after: &str) {
+        let (offset, before) = extract_offset(before);
+        let mut edit = TextEditBuilder::default();
+        edit.insert(offset, ".".to_string());
+        let before = edit.finish().apply(&before);
+        let file = SourceFile::parse(&before);
+        if let Some(result) = on_dot_typed(&file, offset) {
+            let actual = result.edit.apply(&before);
+            assert_eq_text!(after, &actual);
+        } else {
+            assert_eq_text!(&before, after)
+        };
+    }
+
     #[test]
-    fn test_on_dot_typed() {
-        fn do_check(before: &str, after: &str) {
-            let (offset, before) = extract_offset(before);
-            let file = SourceFile::parse(&before);
-            if let Some(result) = on_eq_typed(&file, offset) {
-                let actual = result.edit.apply(&before);
-                assert_eq_text!(after, &actual);
-            };
-        }
-        // indent if continuing chain call
-        do_check(
+    fn indents_new_chain_call() {
+        type_dot(
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        self.child_impl(db, name)
-        .<|>
-    }
-",
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
+                self.child_impl(db, name)
+                <|>
+            }
+            ",
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        self.child_impl(db, name)
-            .
-    }
-",
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
+                self.child_impl(db, name)
+                    .
+            }
+            ",
         );
-
-        // do not indent if already indented
-        do_check(
+        type_dot(
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        self.child_impl(db, name)
-            .<|>
-    }
-",
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
+                self.child_impl(db, name)
+                    <|>
+            }
+            ",
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        self.child_impl(db, name)
-            .
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
+                self.child_impl(db, name)
+                    .
+            }
+            ",
+        )
     }
-",
-        );
 
-        // indent if the previous line is already indented
-        do_check(
+    #[test]
+    fn indents_continued_chain_call() {
+        type_dot(
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        self.child_impl(db, name)
-            .first()
-        .<|>
-    }
-",
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
+                self.child_impl(db, name)
+                    .first()
+                <|>
+            }
+            ",
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        self.child_impl(db, name)
-            .first()
-            .
-    }
-",
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
+                self.child_impl(db, name)
+                    .first()
+                    .
+            }
+            ",
         );
-
-        // don't indent if indent matches previous line
-        do_check(
+        type_dot(
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        self.child_impl(db, name)
-            .first()
-            .<|>
-    }
-",
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
+                self.child_impl(db, name)
+                    .first()
+                    <|>
+            }
+            ",
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        self.child_impl(db, name)
-            .first()
-            .
-    }
-",
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
+                self.child_impl(db, name)
+                    .first()
+                    .
+            }
+            ",
         );
+    }
 
-        // don't indent if there is no method call on previous line
-        do_check(
+    #[test]
+    fn dont_indent_freestanding_dot() {
+        type_dot(
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        .<|>
-    }
-",
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
+                <|>
+            }
+            ",
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        .
-    }
-",
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
+                .
+            }
+            ",
         );
-
-        // indent to match previous expr
-        do_check(
+        type_dot(
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        self.child_impl(db, name)
-.<|>
-    }
-",
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
+            <|>
+            }
+            ",
             r"
-    pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
-        self.child_impl(db, name)
+            pub fn child(&self, db: &impl HirDatabase, name: &Name) -> Cancelable<Option<Module>> {
             .
-    }
-",
+            }
+            ",
         );
     }
 
diff --git a/crates/ra_lsp_server/src/main_loop/handlers.rs b/crates/ra_lsp_server/src/main_loop/handlers.rs
index 0dda9548a..5f4b27149 100644
--- a/crates/ra_lsp_server/src/main_loop/handlers.rs
+++ b/crates/ra_lsp_server/src/main_loop/handlers.rs
@@ -9,7 +9,7 @@ use languageserver_types::{
     SignatureInformation, SymbolInformation, TextDocumentIdentifier, TextEdit, WorkspaceEdit,
 };
 use ra_ide_api::{
-    FileId, FilePosition, FileRange, FoldKind, Query, RunnableKind, Severity, SourceChange,
+    FileId, FilePosition, FileRange, FoldKind, Query, RunnableKind, Severity,
 };
 use ra_syntax::{TextUnit, AstNode};
 use rustc_hash::FxHashMap;
@@ -92,35 +92,30 @@ pub fn handle_on_type_formatting(
     world: ServerWorld,
     params: req::DocumentOnTypeFormattingParams,
 ) -> Result<Option<Vec<TextEdit>>> {
-    let analysis: Option<Box<Fn(FilePosition) -> Option<SourceChange>>> = match params.ch.as_str() {
-        "=" => Some(Box::new(|pos| world.analysis().on_eq_typed(pos))),
-        "." => Some(Box::new(|pos| world.analysis().on_dot_typed(pos))),
-        _ => None,
+    let file_id = params.text_document.try_conv_with(&world)?;
+    let line_index = world.analysis().file_line_index(file_id);
+    let position = FilePosition {
+        file_id,
+        /// in `ra_ide_api`, the `on_type` invariant is that
+        /// `text.char_at(position) == typed_char`.
+        offset: params.position.conv_with(&line_index) - TextUnit::of_char('.'),
     };
 
-    if let Some(ana) = analysis {
-        let file_id = params.text_document.try_conv_with(&world)?;
-        let line_index = world.analysis().file_line_index(file_id);
-        let position = FilePosition {
-            file_id,
-            offset: params.position.conv_with(&line_index),
-        };
+    let edit = match params.ch.as_str() {
+        "=" => world.analysis().on_eq_typed(position),
+        "." => world.analysis().on_dot_typed(position),
+        _ => return Ok(None),
+    };
+    let mut edit = match edit {
+        Some(it) => it,
+        None => return Ok(None),
+    };
 
-        if let Some(mut action) = ana(position) {
-            let change: Vec<TextEdit> = action
-                .source_file_edits
-                .pop()
-                .unwrap()
-                .edit
-                .as_atoms()
-                .iter()
-                .map_conv_with(&line_index)
-                .collect();
-            return Ok(Some(change));
-        }
-    }
+    // This should be a single-file edit
+    let edit = edit.source_file_edits.pop().unwrap();
 
-    return Ok(None);
+    let change: Vec<TextEdit> = edit.edit.conv_with(&line_index);
+    return Ok(Some(change));
 }
 
 pub fn handle_document_symbol(