Merge #10147

10147: fix: don't panic if the client sends invalid request r=matklad a=matklad

bors r+
🤖

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

Author: bors[bot]

crates/rust-analyzer/src/from_proto.rs

@@ -10,7 +10,9 @@ use crate::{
     from_json,
     global_state::GlobalStateSnapshot,
     line_index::{LineIndex, OffsetEncoding},
-    lsp_ext, Result,
+    lsp_ext,
+    lsp_utils::invalid_params_error,
+    Result,
 };
 
 pub(crate) fn abs_path(url: &lsp_types::Url) -> Result<AbsPathBuf> {
@@ -85,7 +87,8 @@ pub(crate) fn annotation(
     snap: &GlobalStateSnapshot,
     code_lens: lsp_types::CodeLens,
 ) -> Result<Annotation> {
-    let data = code_lens.data.unwrap();
+    let data =
+        code_lens.data.ok_or_else(|| invalid_params_error("code lens without data".to_string()))?;
     let resolve = from_json::<lsp_ext::CodeLensResolveData>("CodeLensResolveData", data)?;
 
     match resolve {

crates/rust-analyzer/src/handlers.rs

@@ -40,7 +40,7 @@ use crate::{
         self, InlayHint, InlayHintsParams, PositionOrRange, ViewCrateGraphParams,
         WorkspaceSymbolParams,
     },
-    lsp_utils::all_edits_are_disjoint,
+    lsp_utils::{all_edits_are_disjoint, invalid_params_error},
     to_proto, LspError, Result,
 };
 
@@ -767,9 +767,8 @@ pub(crate) fn handle_completion_resolve(
     let _p = profile::span("handle_completion_resolve");
 
     if !all_edits_are_disjoint(&original_completion, &[]) {
-        return Err(LspError::new(
-            ErrorCode::InvalidParams as i32,
-            "Received a completion with overlapping edits, this is not LSP-compliant".into(),
+        return Err(invalid_params_error(
+            "Received a completion with overlapping edits, this is not LSP-compliant".to_string(),
         )
         .into());
     }
@@ -1038,7 +1037,7 @@ pub(crate) fn handle_code_action_resolve(
     let _p = profile::span("handle_code_action_resolve");
     let params = match code_action.data.take() {
         Some(it) => it,
-        None => Err("can't resolve code action without data")?,
+        None => return Err(invalid_params_error(format!("code action without data")).into()),
     };
 
     let file_id = from_proto::file_id(&snap, &params.code_action_params.text_document.uri)?;
@@ -1056,10 +1055,10 @@ pub(crate) fn handle_code_action_resolve(
     let (assist_index, assist_resolve) = match parse_action_id(&params.id) {
         Ok(parsed_data) => parsed_data,
         Err(e) => {
-            return Err(LspError::new(
-                ErrorCode::InvalidParams as i32,
-                format!("Failed to parse action id string '{}': {}", params.id, e),
-            )
+            return Err(invalid_params_error(format!(
+                "Failed to parse action id string '{}': {}",
+                params.id, e
+            ))
             .into())
         }
     };
@@ -1076,23 +1075,17 @@ pub(crate) fn handle_code_action_resolve(
 
     let assist = match assists.get(assist_index) {
         Some(assist) => assist,
-        None => return Err(LspError::new(
-            ErrorCode::InvalidParams as i32,
-            format!(
-                "Failed to find the assist for index {} provided by the resolve request. Resolve request assist id: {}",
-                assist_index, params.id,
-            ),
-        )
+        None => return Err(invalid_params_error(format!(
+            "Failed to find the assist for index {} provided by the resolve request. Resolve request assist id: {}",
+            assist_index, params.id,
+        ))
         .into())
     };
     if assist.id.0 != expected_assist_id || assist.id.1 != expected_kind {
-        return Err(LspError::new(
-            ErrorCode::InvalidParams as i32,
-            format!(
-                "Mismatching assist at index {} for the resolve parameters given. Resolve request assist id: {}, actual id: {:?}.",
-                assist_index, params.id, assist.id
-            ),
-        )
+        return Err(invalid_params_error(format!(
+            "Mismatching assist at index {} for the resolve parameters given. Resolve request assist id: {}, actual id: {:?}.",
+            assist_index, params.id, assist.id
+        ))
         .into());
     }
     let edit = to_proto::code_action(&snap, assist.clone(), None)?.edit;

crates/rust-analyzer/src/lsp_utils.rs

@@ -8,8 +8,13 @@ use crate::{
     from_proto,
     global_state::GlobalState,
     line_index::{LineEndings, LineIndex, OffsetEncoding},
+    LspError,
 };
 
+pub(crate) fn invalid_params_error(message: String) -> LspError {
+    LspError { code: lsp_server::ErrorCode::InvalidParams as i32, message }
+}
+
 pub(crate) fn is_cancelled(e: &(dyn Error + 'static)) -> bool {
     e.downcast_ref::<Cancelled>().is_some()
 }

crates/rust-analyzer/src/to_proto.rs

@@ -21,7 +21,9 @@ use crate::{
     config::Config,
     global_state::GlobalStateSnapshot,
     line_index::{LineEndings, LineIndex, OffsetEncoding},
-    lsp_ext, semantic_tokens, Result,
+    lsp_ext,
+    lsp_utils::invalid_params_error,
+    semantic_tokens, Result,
 };
 
 pub(crate) fn position(line_index: &LineIndex, offset: TextSize) -> lsp_types::Position {
@@ -1198,7 +1200,9 @@ pub(crate) fn markup_content(markup: Markup) -> lsp_types::MarkupContent {
 }
 
 pub(crate) fn rename_error(err: RenameError) -> crate::LspError {
-    crate::LspError { code: lsp_server::ErrorCode::InvalidParams as i32, message: err.to_string() }
+    // This is wrong, but we don't have a better alternative I suppose?
+    // https://github.com/microsoft/language-server-protocol/issues/1341
+    invalid_params_error(err.to_string())
 }
 
 #[cfg(test)]

docs/dev/style.md

@@ -895,6 +895,29 @@ fn foo() -> Option<Bar> {
 
 **Rationale:** reduce cognitive stack usage.
 
+Use `return Err(err)` to throw an error:
+
+```rust
+// GOOD
+fn f() -> Result<(), ()> {
+    if condition {
+        return Err(());
+    }
+    Ok(())
+}
+
+// BAD
+fn f() -> Result<(), ()> {
+    if condition {
+        Err(())?;
+    }
+    Ok(())
+}
+```
+
+**Rationale:** `return` has type `!`, which allows the compiler to flag dead
+code (`Err(...)?` is of unconstrained generic type `T`).
+
 ## Comparisons
 
 When doing multiple comparisons use `<`/`<=`, avoid `>`/`>=`.