make path changes LSP spec conformant #8949

pascalkuthe · 2023-11-30T02:18:21Z

Closes #8942
Supercedes #8924

Currently, helix implements operations which change the paths of files
incorrectly and inconsistently. This PR ensures that we do the following
whenever a buffer is renamed (:move and workspace edits)

always send did_open/did_close notifications
send will_rename/did_rename requests correctly
- send them to all LSP servers not just those that are active for a
  buffer
- also send these requests for paths that are not yet open in a buffer (if
  triggered from workspace edit).
- only send these if the server registered interests in the path
autodetect language, indent, line ending, ..

This PR also centralizes the infrastructure for path setting and
therefore :w <path> benefits from similar fixed (but without didRename and willRename).

I also used the chance to move the apply_workspace_edit function to helix_view (as that was required anyway to avoid pushing a bunch of new logic down to helix_term that really didn't belong there).

pascalkuthe · 2023-11-30T02:22:05Z

helix-view/src/editor.rs

@@ -1222,7 +1305,7 @@ impl Editor {
                .collect::<HashMap<_, _>>()
        });

-        if language_servers.is_empty() {
+        if language_servers.is_empty() && doc.language_servers.is_empty() {


existing bug that I noticed that I fixetd since its related (to refreshing document language)

pascalkuthe · 2023-11-30T02:23:01Z

helix-view/src/handlers/lsp.rs

+                                let edits = document_edit
+                                    .edits
+                                    .iter()
+                                    .map(|edit| match edit {
+                                        lsp::OneOf::Left(text_edit) => text_edit,
+                                        lsp::OneOf::Right(annotated_text_edit) => {
+                                            &annotated_text_edit.text_edit
+                                        }
+                                    })
+                                    .cloned()
+                                    .collect();
+                                apply_edits(
+                                    self,
+                                    &document_edit.text_document.uri,
+                                    document_edit.text_document.version,
+                                    edits,
+                                )
+                                .map_err(|kind| {
+                                    ApplyEditError {
+                                        kind,
+                                        failed_change_idx: i,
+                                    }
+                                })?;
+                            }
+                        }
+                    }
+                }
+            }
+
+            return Ok(());
+        }
+
+        if let Some(ref changes) = workspace_edit.changes {
+            log::debug!("workspace changes: {:?}", changes);
+            for (i, (uri, text_edits)) in changes.iter().enumerate() {
+                let text_edits = text_edits.to_vec();
+                apply_edits(self, uri, None, text_edits).map_err(|kind| ApplyEditError {
+                    kind,
+                    failed_change_idx: i,
+                })?;
+            }
+        }
+
+        Ok(())
+    }
+
+    fn apply_document_resource_op(&mut self, op: &lsp::ResourceOp) -> std::io::Result<()> {
+        use lsp::ResourceOp;
+        use std::fs;
+        match op {
+            ResourceOp::Create(op) => {
+                let path = op.uri.to_file_path().unwrap();
+                let ignore_if_exists = op.options.as_ref().map_or(false, |options| {
+                    !options.overwrite.unwrap_or(false) && options.ignore_if_exists.unwrap_or(false)
+                });
+                if !ignore_if_exists || !path.exists() {
+                    // Create directory if it does not exist
+                    if let Some(dir) = path.parent() {
+                        if !dir.is_dir() {
+                            fs::create_dir_all(dir)?;
+                        }
+                    }
+
+                    fs::write(&path, [])?;
+                    self.language_servers.file_event_handler.file_changed(path);
+                }
+            }
+            ResourceOp::Delete(op) => {
+                let path = op.uri.to_file_path().unwrap();
+                if path.is_dir() {
+                    let recursive = op
+                        .options
+                        .as_ref()
+                        .and_then(|options| options.recursive)
+                        .unwrap_or(false);
+
+                    if recursive {
+                        fs::remove_dir_all(&path)?
+                    } else {
+                        fs::remove_dir(&path)?
+                    }
+                    self.language_servers.file_event_handler.file_changed(path);
+                } else if path.is_file() {
+                    fs::remove_file(&path)?;
+                }
+            }
+            ResourceOp::Rename(op) => {
+                let from = op.old_uri.to_file_path().unwrap();
+                let to = op.new_uri.to_file_path().unwrap();
+                let ignore_if_exists = op.options.as_ref().map_or(false, |options| {
+                    !options.overwrite.unwrap_or(false) && options.ignore_if_exists.unwrap_or(false)
+                });
+                if !ignore_if_exists || !to.exists() {
+                    self.move_path(&from, &to)?;
+                }
+            }
+        }
+        Ok(())
+    }
+}


this was moved here from helix_term and responsible for most of the diff. I needed to call this from editor.rs. I think this never belonged in helix-term to begin with.

the-mikedavis

Does this also fix #5300?

helix-lsp/src/file_operations.rs

helix-view/src/document.rs

helix-view/src/editor.rs

pascalkuthe · 2023-12-08T20:46:37Z

Does this also fix #5300?

hmm I am not entirely sure. The answer is mostly no. I don't touch the set_language function. However the only call site for set_language is now the :language command which handles everything described in that issue manually (including starting/stopping LSPs if necessary) manually.

I could create a similar set_doc_language function that encapsulates this behavior (potentially similar/unifiend with the new refresh_doc_langugage function although that function auto-detects language instead of accepting it manually) but it doesn't seem necessary right now since it does not affect any user observable behaviour right now.

I am not sure if that issue was always jus a refactoring issue or if this has been fixed elsewhere (potentially the multilsp PR)

helix-view/src/handlers/lsp.rs

helix-view/src/editor.rs

the-mikedavis

I've been testing this for a while with no problems and the changes look good to me. It's nice to have the Editor::apply_workspace_edits and Editor::move_path functionality centralized on the Editor now 👍

(not to mention starting to use the helix-view/src/handlers/lsp.rs file now :P)

pascalkuthe · 2024-01-17T18:35:23Z

rebased as there were quite a few conflicts

archseer

LGTM just needs one last rebase

Currently, helix implements operations which change the paths of files incorrectly and inconsistently. This PR ensures that we do the following whenever a buffer is renamed (`:move` and workspace edits) * always send did_open/did_close notifications * send will_rename/did_rename requests correctly * send them to all LSP servers not just those that are active for a buffer * also send these requests for paths that are not yet open in a buffer (if triggered from workspace edit). * only send these if the server registered interests in the path * autodetect language, indent, line ending, .. This PR also centralizes the infrastructure for path setting and therefore `:w <path>` benefits from similar fixed (but without didRename)

pascalkuthe · 2024-01-25T11:25:15Z

rebased again to resolve conflicts with #8021 (no real changes just conflicts in use statements)

Currently, helix implements operations which change the paths of files incorrectly and inconsistently. This PR ensures that we do the following whenever a buffer is renamed (`:move` and workspace edits) * always send did_open/did_close notifications * send will_rename/did_rename requests correctly * send them to all LSP servers not just those that are active for a buffer * also send these requests for paths that are not yet open in a buffer (if triggered from workspace edit). * only send these if the server registered interests in the path * autodetect language, indent, line ending, .. This PR also centralizes the infrastructure for path setting and therefore `:w <path>` benefits from similar fixed (but without didRename)

pascalkuthe added C-bug Category: This is a bug A-language-server Area: Language server client E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 30, 2023

pascalkuthe commented Nov 30, 2023

View reviewed changes

the-mikedavis changed the title ~~make path changes LSP spec conform~~ make path changes LSP spec conformant Nov 30, 2023

pascalkuthe mentioned this pull request Nov 30, 2023

feat(lsp): will save #8952

Open

the-mikedavis reviewed Dec 7, 2023

View reviewed changes

pascalkuthe force-pushed the lsp_rename_buffers branch from 7491577 to f74d6a5 Compare December 8, 2023 20:47

archseer added this to the next milestone Dec 15, 2023

the-mikedavis reviewed Dec 20, 2023

View reviewed changes

helix-view/src/handlers/lsp.rs Outdated Show resolved Hide resolved

helix-view/src/editor.rs Show resolved Hide resolved

helix-view/src/editor.rs Outdated Show resolved Hide resolved

pascalkuthe force-pushed the lsp_rename_buffers branch 2 times, most recently from 56e725f to ee1a4ad Compare January 7, 2024 22:14

the-mikedavis previously approved these changes Jan 17, 2024

View reviewed changes

pascalkuthe dismissed the-mikedavis’s stale review via 4635b5f January 17, 2024 18:19

pascalkuthe force-pushed the lsp_rename_buffers branch from ee1a4ad to 4635b5f Compare January 17, 2024 18:19

the-mikedavis previously approved these changes Jan 17, 2024

View reviewed changes

archseer previously approved these changes Jan 18, 2024

View reviewed changes

pascalkuthe dismissed stale reviews from archseer and the-mikedavis via c8e41f6 January 18, 2024 17:10

pascalkuthe force-pushed the lsp_rename_buffers branch from 4635b5f to c8e41f6 Compare January 18, 2024 17:10

the-mikedavis previously approved these changes Jan 18, 2024

View reviewed changes

pascalkuthe dismissed the-mikedavis’s stale review via 8c23d3a January 25, 2024 11:24

pascalkuthe force-pushed the lsp_rename_buffers branch from c8e41f6 to 8c23d3a Compare January 25, 2024 11:24

archseer merged commit 87a720c into master Jan 28, 2024
6 checks passed

archseer deleted the lsp_rename_buffers branch January 28, 2024 16:34

pascalkuthe mentioned this pull request Apr 7, 2024

Restart language server when changing document language #5300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make path changes LSP spec conformant #8949

make path changes LSP spec conformant #8949

pascalkuthe commented Nov 30, 2023 •

edited

Loading

pascalkuthe Nov 30, 2023 •

edited

Loading

pascalkuthe Nov 30, 2023

the-mikedavis left a comment

pascalkuthe commented Dec 8, 2023

the-mikedavis left a comment

pascalkuthe commented Jan 17, 2024

archseer left a comment

pascalkuthe commented Jan 25, 2024

make path changes LSP spec conformant #8949

make path changes LSP spec conformant #8949

Conversation

pascalkuthe commented Nov 30, 2023 • edited Loading

pascalkuthe Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

pascalkuthe Nov 30, 2023

Choose a reason for hiding this comment

the-mikedavis left a comment

Choose a reason for hiding this comment

pascalkuthe commented Dec 8, 2023

the-mikedavis left a comment

Choose a reason for hiding this comment

pascalkuthe commented Jan 17, 2024

archseer left a comment

Choose a reason for hiding this comment

pascalkuthe commented Jan 25, 2024

pascalkuthe commented Nov 30, 2023 •

edited

Loading

pascalkuthe Nov 30, 2023 •

edited

Loading