Merge #8510 #8533

8510: Move cursor position when using item movers r=jonas-schievink a=jonas-schievink

This updates the cursor position when moving items around to stay in the same location within the moved node.

I changed the `moveItem` response to `SnippetTextEdit[]`, since that made more sense to me (the file was ignored by the client anyways, since the edits always apply to the current document). It also matches `onEnter`, which seems logical to me, but please let me know if this doesn't make sense.

There's still a bug in the client-side snippet code that will cause the cursor position to be slightly off when moving parameters in the same line (presumably we don't track the column correctly after deleting `$0`). Not really sure how to fix that immediately, but this PR should already be an improvement despite that bug.

8533: Fix typo in style guide r=jonas-schievink a=jonas-schievink

Fixes bold text rendering

bors r+

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>

crates/ide/src/move_item.rs

@@ -1,4 +1,4 @@
-use std::iter::once;
+use std::{iter::once, mem};
 
 use hir::Semantics;
 use ide_db::{base_db::FileRange, RootDatabase};
@@ -102,7 +102,7 @@ fn move_in_direction(
             ast::GenericArgList(it) => swap_sibling_in_list(node, it.generic_args(), range, direction),
             ast::VariantList(it) => swap_sibling_in_list(node, it.variants(), range, direction),
             ast::TypeBoundList(it) => swap_sibling_in_list(node, it.bounds(), range, direction),
-            _ => Some(replace_nodes(node, &match direction {
+            _ => Some(replace_nodes(range, node, &match direction {
                 Direction::Up => node.prev_sibling(),
                 Direction::Down => node.next_sibling(),
             }?))
@@ -125,7 +125,7 @@ fn swap_sibling_in_list<A: AstNode + Clone, I: Iterator<Item = A>>(
         .next();
 
     if let Some((l, r)) = list_lookup {
-        Some(replace_nodes(l.syntax(), r.syntax()))
+        Some(replace_nodes(range, l.syntax(), r.syntax()))
     } else {
         // Cursor is beyond any movable list item (for example, on curly brace in enum).
         // It's not necessary, that parent of list is movable (arg list's parent is not, for example),
@@ -134,11 +134,38 @@ fn swap_sibling_in_list<A: AstNode + Clone, I: Iterator<Item = A>>(
     }
 }
 
-fn replace_nodes(first: &SyntaxNode, second: &SyntaxNode) -> TextEdit {
+fn replace_nodes<'a>(
+    range: TextRange,
+    mut first: &'a SyntaxNode,
+    mut second: &'a SyntaxNode,
+) -> TextEdit {
+    let cursor_offset = if range.is_empty() {
+        // FIXME: `applySnippetTextEdits` does not support non-empty selection ranges
+        if first.text_range().contains_range(range) {
+            Some(range.start() - first.text_range().start())
+        } else if second.text_range().contains_range(range) {
+            mem::swap(&mut first, &mut second);
+            Some(range.start() - first.text_range().start())
+        } else {
+            None
+        }
+    } else {
+        None
+    };
+
+    let first_with_cursor = match cursor_offset {
+        Some(offset) => {
+            let mut item_text = first.text().to_string();
+            item_text.insert_str(offset.into(), "$0");
+            item_text
+        }
+        None => first.text().to_string(),
+    };
+
     let mut edit = TextEditBuilder::default();
 
     algo::diff(first, second).into_text_edit(&mut edit);
-    algo::diff(second, first).into_text_edit(&mut edit);
+    edit.replace(second.text_range(), first_with_cursor);
 
     edit.finish()
 }
@@ -188,7 +215,7 @@ fn main() {
             expect![[r#"
 fn main() {
     match true {
-        false => {
+        false =>$0 {
             println!("Test");
         },
         true => {
@@ -222,7 +249,7 @@ fn main() {
         false => {
             println!("Test");
         },
-        true => {
+        true =>$0 {
             println!("Hello, world");
         }
     };
@@ -274,7 +301,7 @@ fn main() {
             "#,
             expect![[r#"
 fn main() {
-    let test2 = 456;
+    let test2$0 = 456;
     let test = 123;
 }
             "#]],
@@ -293,7 +320,7 @@ fn main() {
             "#,
             expect![[r#"
 fn main() {
-    println!("All I want to say is...");
+    println!("All I want to say is...");$0
     println!("Hello, world");
 }
             "#]],
@@ -313,7 +340,7 @@ fn main() {
 fn main() {
     if true {
         println!("Test");
-    }
+    }$0
 
     println!("Hello, world");
 }
@@ -334,7 +361,7 @@ fn main() {
 fn main() {
     for i in 0..10 {
         println!("Test");
-    }
+    }$0
 
     println!("Hello, world");
 }
@@ -355,7 +382,7 @@ fn main() {
 fn main() {
     loop {
         println!("Test");
-    }
+    }$0
 
     println!("Hello, world");
 }
@@ -376,7 +403,7 @@ fn main() {
 fn main() {
     while true {
         println!("Test");
-    }
+    }$0
 
     println!("Hello, world");
 }
@@ -393,7 +420,7 @@ fn main() {
             "#,
             expect![[r#"
 fn main() {
-    return 123;
+    return 123;$0
 
     println!("Hello, world");
 }
@@ -430,7 +457,7 @@ fn main() {}
 fn foo() {}$0$0
             "#,
             expect![[r#"
-fn foo() {}
+fn foo() {}$0
 
 fn main() {}
             "#]],
@@ -451,7 +478,7 @@ impl Wow for Yay $0$0{}
             expect![[r#"
 struct Yay;
 
-impl Wow for Yay {}
+impl Wow for Yay $0{}
 
 trait Wow {}
             "#]],
@@ -467,7 +494,7 @@ use std::vec::Vec;
 use std::collections::HashMap$0$0;
             "#,
             expect![[r#"
-use std::collections::HashMap;
+use std::collections::HashMap$0;
 use std::vec::Vec;
             "#]],
             Direction::Up,
@@ -502,7 +529,7 @@ fn main() {
     }
 
     #[test]
-    fn test_moves_param_up() {
+    fn test_moves_param() {
         check(
             r#"
 fn test(one: i32, two$0$0: u32) {}
@@ -512,7 +539,7 @@ fn main() {
 }
             "#,
             expect![[r#"
-fn test(two: u32, one: i32) {}
+fn test(two$0: u32, one: i32) {}
 
 fn main() {
     test(123, 456);
@@ -520,6 +547,15 @@ fn main() {
             "#]],
             Direction::Up,
         );
+        check(
+            r#"
+fn f($0$0arg: u8, arg2: u16) {}
+            "#,
+            expect![[r#"
+fn f(arg2: u16, $0arg: u8) {}
+            "#]],
+            Direction::Down,
+        );
     }
 
     #[test]
@@ -536,7 +572,7 @@ fn main() {
 fn test(one: i32, two: u32) {}
 
 fn main() {
-    test(456, 123);
+    test(456$0, 123);
 }
             "#]],
             Direction::Up,
@@ -557,7 +593,7 @@ fn main() {
 fn test(one: i32, two: u32) {}
 
 fn main() {
-    test(456, 123);
+    test(456, 123$0);
 }
             "#]],
             Direction::Down,
@@ -594,7 +630,7 @@ struct Test<A, B$0$0>(A, B);
 fn main() {}
             "#,
             expect![[r#"
-struct Test<B, A>(A, B);
+struct Test<B$0, A>(A, B);
 
 fn main() {}
             "#]],
@@ -616,7 +652,7 @@ fn main() {
 struct Test<A, B>(A, B);
 
 fn main() {
-    let t = Test::<&str, i32>(123, "yay");
+    let t = Test::<&str$0, i32>(123, "yay");
 }
             "#]],
             Direction::Up,
@@ -636,7 +672,7 @@ fn main() {}
             "#,
             expect![[r#"
 enum Hello {
-    Two,
+    Two$0,
     One
 }
 
@@ -663,7 +699,7 @@ trait One {}
 
 trait Two {}
 
-fn test<T: Two + One>(t: T) {}
+fn test<T: Two$0 + One>(t: T) {}
 
 fn main() {}
             "#]],
@@ -709,7 +745,7 @@ trait Yay {
 impl Yay for Test {
     type One = i32;
 
-    fn inner() {
+    fn inner() {$0
         println!("Mmmm");
     }
 
@@ -736,7 +772,7 @@ fn test() {
             "#,
             expect![[r#"
 fn test() {
-    mod hi {
+    mod hi {$0
         fn inner() {}
     }
 
@@ -764,7 +800,7 @@ fn main() {}
             expect![[r#"
 fn main() {}
 
-#[derive(Debug)]
+$0#[derive(Debug)]
 enum FooBar {
     Foo,
     Bar,
@@ -784,7 +820,7 @@ fn main() {}
             expect![[r#"
 fn main() {}
 
-enum FooBar {
+$0enum FooBar {
     Foo,
     Bar,
 }
@@ -804,7 +840,7 @@ fn main() {}
             expect![[r#"
 struct Test;
 
-impl SomeTrait for Test {}
+$0impl SomeTrait for Test {}
 
 trait SomeTrait {}
 
@@ -831,7 +867,7 @@ fn main() {}
 enum FooBar {
     Foo,
     Bar,
-}
+}$0
             "#]],
             Direction::Down,
         );
@@ -848,7 +884,7 @@ fn main() {}
             expect![[r#"
 struct Test;
 
-impl SomeTrait for Test {}
+impl SomeTrait for Test {}$0
 
 trait SomeTrait {}
 

crates/rust-analyzer/src/handlers.rs

@@ -1410,7 +1410,7 @@ pub(crate) fn handle_open_cargo_toml(
 pub(crate) fn handle_move_item(
     snap: GlobalStateSnapshot,
     params: lsp_ext::MoveItemParams,
-) -> Result<Option<lsp_types::TextDocumentEdit>> {
+) -> Result<Vec<lsp_ext::SnippetTextEdit>> {
     let _p = profile::span("handle_move_item");
     let file_id = from_proto::file_id(&snap, &params.text_document.uri)?;
     let range = from_proto::file_range(&snap, params.text_document, params.range)?;
@@ -1421,8 +1421,11 @@ pub(crate) fn handle_move_item(
     };
 
     match snap.analysis.move_item(range, direction)? {
-        Some(text_edit) => Ok(Some(to_proto::text_document_edit(&snap, file_id, text_edit)?)),
+        Some(text_edit) => {
-        None => Ok(None),
+            let line_index = snap.file_line_index(file_id)?;
+            Ok(to_proto::snippet_text_edit_vec(&line_index, true, text_edit))
+        }
+        None => Ok(vec![]),
     }
 }
 

crates/rust-analyzer/src/lsp_ext.rs

@@ -407,7 +407,7 @@ pub enum MoveItem {}
 
 impl Request for MoveItem {
     type Params = MoveItemParams;
-    type Result = Option<lsp_types::TextDocumentEdit>;
+    type Result = Vec<SnippetTextEdit>;
     const METHOD: &'static str = "experimental/moveItem";
 }
 

crates/rust-analyzer/src/to_proto.rs

@@ -688,18 +688,6 @@ pub(crate) fn goto_definition_response(
     }
 }
 
-pub(crate) fn text_document_edit(
-    snap: &GlobalStateSnapshot,
-    file_id: FileId,
-    edit: TextEdit,
-) -> Result<lsp_types::TextDocumentEdit> {
-    let text_document = optional_versioned_text_document_identifier(snap, file_id);
-    let line_index = snap.file_line_index(file_id)?;
-    let edits =
-        edit.into_iter().map(|it| lsp_types::OneOf::Left(text_edit(&line_index, it))).collect();
-    Ok(lsp_types::TextDocumentEdit { text_document, edits })
-}
-
 pub(crate) fn snippet_text_document_edit(
     snap: &GlobalStateSnapshot,
     is_snippet: bool,

docs/dev/lsp-extensions.md

@@ -1,8 +1,8 @@
 <!---
-lsp_ext.rs hash: faae991334a151d0
+lsp_ext.rs hash: b19ddc3ab8767af9
 
 If you need to change the above hash to make the test pass, please check if you
-need to adjust this doc as well and ping this  issue:
+need to adjust this doc as well and ping this issue:
 
   https://github.com/rust-analyzer/rust-analyzer/issues/4604
 
@@ -620,7 +620,7 @@ This request is sent from client to server to move item under cursor or selectio
 
 **Request:** `MoveItemParams`
 
-**Response:** `TextDocumentEdit | null`
+**Response:** `SnippetTextEdit[]`
 
 ```typescript
 export interface MoveItemParams {

docs/dev/style.md

@@ -159,7 +159,7 @@ More than one mark per test / code branch doesn't add significantly to understan
 Do not use `#[should_panic]` tests.
 Instead, explicitly check for `None`, `Err`, etc.
 
-**Rationale:**a `#[should_panic]` is a tool for library authors, to makes sure that API does not fail silently, when misused.
+**Rationale:** `#[should_panic]` is a tool for library authors, to makes sure that API does not fail silently, when misused.
 `rust-analyzer` is not a library, we don't need to test for API misuse, and we have to handle any user input without panics.
 Panic messages in the logs from the `#[should_panic]` tests are confusing.
 

editors/code/src/commands.ts

@@ -148,34 +148,16 @@ export function moveItem(ctx: Ctx, direction: ra.Direction): Cmd {
         const client = ctx.client;
         if (!editor || !client) return;
 
-        const edit = await client.sendRequest(ra.moveItem, {
+        const lcEdits = await client.sendRequest(ra.moveItem, {
             range: client.code2ProtocolConverter.asRange(editor.selection),
             textDocument: ctx.client.code2ProtocolConverter.asTextDocumentIdentifier(editor.document),
             direction
         });
 
-        if (!edit) return;
+        if (!lcEdits) return;
 
-        let cursor: vscode.Position | null = null;
+        const edits = client.protocol2CodeConverter.asTextEdits(lcEdits);
-
+        await applySnippetTextEdits(editor, edits);
-        await editor.edit((builder) => {
-            client.protocol2CodeConverter.asTextEdits(edit.edits).forEach((edit: any) => {
-                builder.replace(edit.range, edit.newText);
-
-                if (direction === ra.Direction.Up) {
-                    if (!cursor || edit.range.end.isBeforeOrEqual(cursor)) {
-                        cursor = edit.range.end;
-                    }
-                } else {
-                    if (!cursor || edit.range.end.isAfterOrEqual(cursor)) {
-                        cursor = edit.range.end;
-                    }
-                }
-            });
-        }).then(() => {
-            const newPosition = cursor ?? editor.selection.start;
-            editor.selection = new vscode.Selection(newPosition, newPosition);
-        });
     };
 }
 

editors/code/src/lsp_ext.ts

@@ -129,7 +129,7 @@ export interface OpenCargoTomlParams {
     textDocument: lc.TextDocumentIdentifier;
 }
 
-export const moveItem = new lc.RequestType<MoveItemParams, lc.TextDocumentEdit | void, void>("experimental/moveItem");
+export const moveItem = new lc.RequestType<MoveItemParams, lc.TextEdit[], void>("experimental/moveItem");
 
 export interface MoveItemParams {
     textDocument: lc.TextDocumentIdentifier;