Fix additional suggestions

crates/ruff_server/src/edit/document.rs

@@ -1,4 +1,4 @@
-use lsp_types::{Position, TextDocumentContentChangeEvent};
+use lsp_types::TextDocumentContentChangeEvent;
 use ruff_source_file::LineIndex;
 
 use crate::PositionEncoding;
@@ -7,14 +7,21 @@ use super::RangeExt;
 
 pub(crate) type DocumentVersion = i32;
 
+/// The state for an individual document in the server. Stays up-to-date
+/// with changes made by the user, including unsaved changes.
 #[derive(Debug, Clone)]
 pub struct Document {
+    /// The string contents of the document.
     contents: String,
+    /// A computed line index for the document. This should always reflect
+    /// the current version of `contents`. Using a function like [`Self::modify`]
+    /// will re-calculate the line index automatically when the `contents` value is updated.
     index: LineIndex,
+    /// The latest version of the document, set by the LSP client. The server will panic in
+    /// debug mode if we attempt to update the document with an 'older' version.
     version: DocumentVersion,
 }
 
-/* Mutable API */
 impl Document {
     pub fn new(contents: String, version: DocumentVersion) -> Self {
         let index = LineIndex::from_source_text(&contents);
@@ -55,27 +62,17 @@ impl Document {
             return;
         }
 
+        let old_contents = self.contents().to_string();
         let mut new_contents = self.contents().to_string();
         let mut active_index = std::borrow::Cow::Borrowed(self.index());
 
-        let mut last_position = Position {
-            line: u32::MAX,
-            character: u32::MAX,
-        };
-
         for TextDocumentContentChangeEvent {
             range,
             text: change,
             ..
         } in changes
         {
             if let Some(range) = range {
-                if last_position <= range.end {
-                    active_index =
-                        std::borrow::Cow::Owned(LineIndex::from_source_text(&new_contents));
-                }
-
-                last_position = range.start;
                 let range = range.to_text_range(&new_contents, &active_index, encoding);
 
                 new_contents.replace_range(
@@ -84,12 +81,17 @@ impl Document {
                 );
             } else {
                 new_contents = change;
-                last_position = Position::default();
+            }
+
+            if new_contents != old_contents {
+                active_index = std::borrow::Cow::Owned(LineIndex::from_source_text(&new_contents));
             }
         }
 
         self.modify_with_manual_index(|contents, version, index| {
-            *index = LineIndex::from_source_text(&new_contents);
+            if contents != &new_contents {
+                *index = LineIndex::from_source_text(&new_contents);
+            }
             *contents = new_contents;
             *version = new_version;
         });

crates/ruff_server/src/lib.rs

@@ -1,26 +1,21 @@
 //! ## The Ruff Language Server
 
-/* `pub use` statements */
 pub use edit::{Document, PositionEncoding};
 pub use server::Server;
 
-/* modules */
 mod edit;
 mod format;
 mod lint;
 mod server;
 mod session;
 
-/* consts */
 pub(crate) const SERVER_NAME: &str = "ruff";
 pub(crate) const DIAGNOSTIC_NAME: &str = "Ruff";
 
-/* types */
 /// A common result type used in most cases where a
 /// result type is needed.
 pub(crate) type Result<T> = anyhow::Result<T>;
 
-/* functions */
 pub(crate) fn version() -> &'static str {
     ruff_linter::VERSION
 }

crates/ruff_server/src/server.rs

@@ -44,8 +44,11 @@ impl Server {
 
         let workspaces = init_params
             .workspace_folders
-            .map(|f| f.into_iter().map(|w| w.uri).collect())
-            .or_else(|| init_params.root_uri.map(|u| vec![u])).ok_or_else(|| anyhow!("No workspace or root URI was given in the LSP initialization parameters. The server cannot start."))?;
+            .map(|folders| folders.into_iter().map(|folder| folder.uri).collect())
+            .or_else(|| init_params.root_uri.map(|u| vec![u]))
+            .ok_or_else(|| {
+                anyhow!("No workspace or root URI was given in the LSP initialization parameters. The server cannot start.")
+            })?;
 
         let initialize_data = serde_json::json!({
             "capabilities": server_capabilities,

crates/ruff_server/src/server/api.rs

@@ -50,7 +50,7 @@ pub(super) fn request<'a>(req: server::Request) -> Task<'a> {
         }
     }
     .unwrap_or_else(|err| {
-        tracing::error!("Encountered error when routing request: {err}");
+        tracing::error!("Encountered error when routing request with ID {id}: {err}");
         let result: Result<()> = Err(err);
         Task::immediate(id, result)
     })
@@ -154,11 +154,11 @@ where
         .extract(Req::METHOD)
         .map_err(|err| match err {
             json_err @ server::ExtractError::JsonError { .. } => {
-                let err: anyhow::Error = json_err.into();
-                anyhow::anyhow!("JSON parsing failure:\n{err}")
+                anyhow::anyhow!("JSON parsing failure:\n{json_err}")
             }
             server::ExtractError::MethodMismatch(_) => {
-                unreachable!("A method mismatch should not be possible here")
+                unreachable!("A method mismatch should not be possible here unless you've used a different handler (`Req`) \
+                    than the one whose method name was matched against earlier.")
             }
         })
         .with_failure_code(server::ErrorCode::InternalError)
@@ -194,11 +194,11 @@ fn cast_notification<N>(
             .extract(N::METHOD)
             .map_err(|err| match err {
                 json_err @ server::ExtractError::JsonError { .. } => {
-                    let err: anyhow::Error = json_err.into();
-                    anyhow::anyhow!("JSON parsing failure:\n{err}")
+                    anyhow::anyhow!("JSON parsing failure:\n{json_err}")
                 }
                 server::ExtractError::MethodMismatch(_) => {
-                    unreachable!("A method mismatch should not be possible here")
+                    unreachable!("A method mismatch should not be possible here unless you've used a different handler (`N`) \
+                        than the one whose method name was matched against earlier.")
                 }
             })
             .with_failure_code(server::ErrorCode::InternalError)?,

crates/ruff_server/src/session.rs

@@ -15,10 +15,13 @@ use rustc_hash::FxHashMap;
 use crate::edit::{Document, DocumentVersion};
 use crate::PositionEncoding;
 
-/// The global state for the LSP.
+/// The global state for the LSP
 pub(crate) struct Session {
+    /// Workspace folders in the current session, which contain the state of all open files.
     workspaces: Workspaces,
+    /// The global position encoding, negotiated during LSP initialization.
     position_encoding: PositionEncoding,
+    /// Extension-specific settings, set by the client, that apply to all workspace folders.
     #[allow(dead_code)]
     lsp_settings: types::ExtensionSettings,
 }
@@ -53,7 +56,9 @@ pub(crate) struct OpenDocuments {
     documents: FxHashMap<Url, DocumentController>,
 }
 
-/// A handler to an underlying document, with a revision counter.
+/// A mutable handler to an underlying document.
+/// Handles copy-on-write mutation automatically when
+/// calling `deref_mut`.
 pub(crate) struct DocumentController {
     document: Arc<Document>,
 }
@@ -82,18 +87,18 @@ impl Session {
     pub(crate) fn take_snapshot(&self, url: &Url) -> Option<DocumentSnapshot> {
         Some(DocumentSnapshot {
             configuration: self.workspaces.configuration(url)?.clone(),
-            document_ref: self.workspaces.doc_snapshot(url)?,
+            document_ref: self.workspaces.snapshot(url)?,
             position_encoding: self.position_encoding,
             url: url.clone(),
         })
     }
 
     pub(crate) fn open_document(&mut self, url: &Url, contents: String, version: DocumentVersion) {
-        self.workspaces.open_document(url, contents, version);
+        self.workspaces.open(url, contents, version);
     }
 
     pub(crate) fn close_document(&mut self, url: &Url) -> crate::Result<()> {
-        self.workspaces.close_document(url)?;
+        self.workspaces.close(url)?;
         Ok(())
     }
 
@@ -102,7 +107,7 @@ impl Session {
         url: &Url,
     ) -> crate::Result<&mut DocumentController> {
         self.workspaces
-            .doc_controller(url)
+            .controller(url)
             .ok_or_else(|| anyhow!("Tried to open unavailable document `{url}`"))
     }
 
@@ -122,13 +127,13 @@ impl Session {
 }
 
 impl OpenDocuments {
-    fn doc_snapshot(&self, url: &Url) -> Option<DocumentRef> {
+    fn snapshot(&self, url: &Url) -> Option<DocumentRef> {
         Some(self.documents.get(url)?.make_ref())
     }
-    fn doc_controller(&mut self, url: &Url) -> Option<&mut DocumentController> {
+    fn controller(&mut self, url: &Url) -> Option<&mut DocumentController> {
         self.documents.get_mut(url)
     }
-    fn open_document(&mut self, url: &Url, contents: String, version: DocumentVersion) {
+    fn open(&mut self, url: &Url, contents: String, version: DocumentVersion) {
         if self
             .documents
             .insert(url.clone(), DocumentController::new(contents, version))
@@ -137,7 +142,7 @@ impl OpenDocuments {
             tracing::warn!("Opening document `{url}` that is already open!");
         }
     }
-    fn close_document(&mut self, url: &Url) -> crate::Result<()> {
+    fn close(&mut self, url: &Url) -> crate::Result<()> {
         let Some(_) = self.documents.remove(url) else {
             return Err(anyhow!(
                 "Tried to close document `{url}`, which was not open"
@@ -223,51 +228,48 @@ impl Workspaces {
         Ok(())
     }
 
-    fn doc_snapshot(&self, document_url: &Url) -> Option<DocumentRef> {
+    fn snapshot(&self, document_url: &Url) -> Option<DocumentRef> {
         self.workspace_for_url(document_url)
-            .and_then(|w| w.open_documents.doc_snapshot(document_url))
+            .and_then(|w| w.open_documents.snapshot(document_url))
     }
 
-    fn doc_controller(&mut self, document_url: &Url) -> Option<&mut DocumentController> {
+    fn controller(&mut self, document_url: &Url) -> Option<&mut DocumentController> {
         self.workspace_for_url_mut(document_url)
-            .and_then(|w| w.open_documents.doc_controller(document_url))
+            .and_then(|w| w.open_documents.controller(document_url))
     }
 
     fn configuration(&self, document_url: &Url) -> Option<&Arc<RuffConfiguration>> {
         self.workspace_for_url(document_url)
             .map(|w| &w.configuration)
     }
 
-    fn open_document(&mut self, url: &Url, contents: String, version: DocumentVersion) {
+    fn open(&mut self, url: &Url, contents: String, version: DocumentVersion) {
         if let Some(w) = self.workspace_for_url_mut(url) {
-            w.open_documents.open_document(url, contents, version);
+            w.open_documents.open(url, contents, version);
         }
     }
 
-    fn close_document(&mut self, url: &Url) -> crate::Result<()> {
+    fn close(&mut self, url: &Url) -> crate::Result<()> {
         self.workspace_for_url_mut(url)
             .ok_or_else(|| anyhow!("Workspace not found for {url}"))?
             .open_documents
-            .close_document(url)
+            .close(url)
     }
 
     fn workspace_for_url(&self, url: &Url) -> Option<&Workspace> {
         let path = url.to_file_path().ok()?;
         self.0
-            .keys()
-            .filter(|p| path.starts_with(p))
-            .max_by_key(|p| p.as_os_str().len())
-            .and_then(|u| self.0.get(u))
+            .range(..path)
+            .next_back()
+            .map(|(_, workspace)| workspace)
     }
 
     fn workspace_for_url_mut(&mut self, url: &Url) -> Option<&mut Workspace> {
         let path = url.to_file_path().ok()?;
         self.0
-            .keys()
-            .filter(|p| path.starts_with(p))
-            .max_by_key(|p| p.as_os_str().len())
-            .cloned()
-            .and_then(|u| self.0.get_mut(&u))
+            .range_mut(..path)
+            .next_back()
+            .map(|(_, workspace)| workspace)
     }
 }
 
@@ -299,7 +301,7 @@ impl Workspace {
 
 pub(crate) fn find_configuration_from_root(root: &Path) -> crate::Result<RuffConfiguration> {
     let pyproject = ruff_workspace::pyproject::find_settings_toml(root)?
-        .ok_or_else(|| anyhow!("No pyproject.toml/ruff.toml file was found"))?;
+        .ok_or_else(|| anyhow!("No pyproject.toml/ruff.toml/.ruff.toml file was found"))?;
     let settings = ruff_workspace::resolver::resolve_root_settings(
         &pyproject,
         Relativity::Parent,