diff --git a/CHANGELOG.md b/CHANGELOG.md index 35fec6cd197..727c2d6f2c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,10 @@ used on tuples. ([Giacomo Cavalieri](https://github.com/giacomocavalieri)) +- When renaming, if the new name is invalid, the language server will produce an + error message instead of silently doing nothing. + ([Giacomo Cavalieri](https://github.com/giacomocavalieri)) + ### Formatter ### Bug fixes diff --git a/compiler-core/src/language_server/engine.rs b/compiler-core/src/language_server/engine.rs index b67288f0cb5..7c47f91979c 100644 --- a/compiler-core/src/language_server/engine.rs +++ b/compiler-core/src/language_server/engine.rs @@ -20,6 +20,7 @@ use crate::{ files::FileSystemProxy, progress::ProgressReporter, reference::FindVariableReferences, + rename::RenameOutcome, }, line_numbers::LineNumbers, paths::ProjectPaths, @@ -34,6 +35,7 @@ use camino::Utf8PathBuf; use ecow::EcoString; use itertools::Itertools; use lsp::CodeAction; +use lsp_server::ResponseError; use lsp_types::{ self as lsp, DocumentSymbol, Hover, HoverContents, MarkedString, Position, PrepareRenameResponse, Range, SignatureHelp, SymbolKind, SymbolTag, TextEdit, Url, @@ -698,17 +700,20 @@ where }) } - pub fn rename(&mut self, params: lsp::RenameParams) -> Response> { + pub fn rename( + &mut self, + params: lsp::RenameParams, + ) -> Response, ResponseError>> { self.respond(|this| { let position = ¶ms.text_document_position; let (lines, found) = match this.node_at_position(position) { Some(value) => value, - None => return Ok(None), + None => return Ok(RenameOutcome::NoRenames.into_result()), }; let Some(module) = this.module_for_uri(&position.text_document.uri) else { - return Ok(None); + return Ok(RenameOutcome::NoRenames.into_result()); }; Ok(match reference_for_ast_node(found, &module.name) { @@ -719,7 +724,9 @@ where .. }) => { let rename_kind = match origin.map(|origin| origin.syntax) { - Some(VariableSyntax::Generated) => return Ok(None), + Some(VariableSyntax::Generated) => { + return Ok(RenameOutcome::NoRenames.into_result()); + } Some(VariableSyntax::LabelShorthand(_)) => { VariableReferenceKind::LabelShorthand } @@ -736,6 +743,7 @@ where name, rename_kind, ) + .into_result() } Some(Referenced::ModuleValue { module: module_name, @@ -755,7 +763,9 @@ where target_kind, layer: ast::Layer::Value, }, - ), + ) + .into_result(), + Some(Referenced::ModuleType { module: module_name, target_kind, @@ -773,8 +783,10 @@ where target_kind, layer: ast::Layer::Type, }, - ), - None => None, + ) + .into_result(), + + None => RenameOutcome::NoRenames.into_result(), }) }) } diff --git a/compiler-core/src/language_server/rename.rs b/compiler-core/src/language_server/rename.rs index e6ae12e45b4..4b1a5f21dbc 100644 --- a/compiler-core/src/language_server/rename.rs +++ b/compiler-core/src/language_server/rename.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use ecow::EcoString; +use lsp_server::ResponseError; use lsp_types::{Range, RenameParams, TextEdit, Url, WorkspaceEdit}; use crate::{ @@ -33,6 +34,34 @@ fn workspace_edit(uri: Url, edits: Vec) -> WorkspaceEdit { } } +pub enum RenameOutcome { + InvalidName { name: EcoString }, + NoRenames, + Renamed { edit: WorkspaceEdit }, +} + +/// Error code for when a request has invalid params as described in: +/// https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#errorCodes +/// +const INVALID_PARAMS: i32 = -32602; + +impl RenameOutcome { + /// Turns the outcome of renaming into a value that's suitable to be used as + /// a response in the language server engine. + /// + pub fn into_result(self) -> Result, ResponseError> { + match self { + RenameOutcome::NoRenames => Ok(None), + RenameOutcome::Renamed { edit } => Ok(Some(edit)), + RenameOutcome::InvalidName { name } => Err(ResponseError { + code: INVALID_PARAMS, + message: format!("{name} is not a valid name"), + data: None, + }), + } + } +} + pub fn rename_local_variable( module: &Module, line_numbers: &LineNumbers, @@ -40,15 +69,10 @@ pub fn rename_local_variable( definition_location: SrcSpan, name: EcoString, kind: VariableReferenceKind, -) -> Option { - if name::check_name_case( - Default::default(), - ¶ms.new_name.as_str().into(), - Named::Variable, - ) - .is_err() - { - return None; +) -> RenameOutcome { + let new_name = EcoString::from(¶ms.new_name); + if name::check_name_case(Default::default(), &new_name, Named::Variable).is_err() { + return RenameOutcome::InvalidName { name: new_name }; } let uri = params.text_document_position.text_document.uri.clone(); @@ -77,7 +101,9 @@ pub fn rename_local_variable( } } - Some(workspace_edit(uri, edits.edits)) + RenameOutcome::Renamed { + edit: workspace_edit(uri, edits.edits), + } } pub enum RenameTarget { @@ -100,17 +126,18 @@ pub fn rename_module_entity( modules: &im::HashMap, sources: &HashMap, renamed: Renamed<'_>, -) -> Option { +) -> RenameOutcome { + let new_name = EcoString::from(¶ms.new_name); if name::check_name_case( // We don't care about the actual error here, just whether the name is valid, // so we just use the default span. SrcSpan::default(), - ¶ms.new_name.as_str().into(), + &new_name, renamed.name_kind, ) .is_err() { - return None; + return RenameOutcome::InvalidName { name: new_name }; } match renamed.target_kind { @@ -155,7 +182,9 @@ pub fn rename_module_entity( } } - Some(workspace_edit) + RenameOutcome::Renamed { + edit: workspace_edit, + } } fn rename_references_in_module( @@ -204,13 +233,15 @@ fn alias_references_in_module( module_name: &EcoString, name: &EcoString, layer: ast::Layer, -) -> Option { +) -> RenameOutcome { let reference_map = match layer { ast::Layer::Value => &module.ast.type_info.references.value_references, ast::Layer::Type => &module.ast.type_info.references.type_references, }; - let references = reference_map.get(&(module_name.clone(), name.clone()))?; + let Some(references) = reference_map.get(&(module_name.clone(), name.clone())) else { + return RenameOutcome::NoRenames; + }; let mut edits = TextEdits::new(&module.ast.type_info.line_numbers); let mut found_import = false; @@ -257,10 +288,12 @@ fn alias_references_in_module( } } - Some(workspace_edit( - params.text_document_position.text_document.uri.clone(), - edits.edits, - )) + RenameOutcome::Renamed { + edit: workspace_edit( + params.text_document_position.text_document.uri.clone(), + edits.edits, + ), + } } fn add_import( diff --git a/compiler-core/src/language_server/server.rs b/compiler-core/src/language_server/server.rs index 80fd737bc1b..b74444cd92b 100644 --- a/compiler-core/src/language_server/server.rs +++ b/compiler-core/src/language_server/server.rs @@ -19,6 +19,7 @@ use crate::{ use camino::{Utf8Path, Utf8PathBuf}; use debug_ignore::DebugIgnore; use itertools::Itertools; +use lsp_server::ResponseError; use lsp_types::{ self as lsp, HoverProviderCapability, InitializeParams, Position, PublishDiagnosticsParams, Range, RenameOptions, TextEdit, Url, @@ -98,7 +99,7 @@ where } fn handle_request(&mut self, id: lsp_server::RequestId, request: Request) { - let (payload, feedback) = match request { + let (outcome, feedback) = match request { Request::Format(param) => self.format(param), Request::Hover(param) => self.hover(param), Request::GoToDefinition(param) => self.goto_definition(param), @@ -114,11 +115,19 @@ where self.publish_feedback(feedback); - let response = lsp_server::Response { - id, - error: None, - result: Some(payload), + let response = match outcome { + Ok(payload) => lsp_server::Response { + id, + error: None, + result: Some(payload), + }, + Err(error) => lsp_server::Response { + id, + error: Some(error), + result: None, + }, }; + self.connection .sender .send(lsp_server::Message::Response(response)) @@ -236,12 +245,33 @@ where &mut self, path: Utf8PathBuf, handler: Handler, - ) -> (Json, Feedback) + ) -> (Result, Feedback) where T: serde::Serialize, Handler: FnOnce( &mut LanguageServerEngine>, ) -> engine::Response, + { + self.fallible_respond_with_engine(path, |engine| { + let response = handler(engine); + engine::Response { + result: response.result.map(Ok), + warnings: response.warnings, + compilation: response.compilation, + } + }) + } + + fn fallible_respond_with_engine( + &mut self, + path: Utf8PathBuf, + handler: Handler, + ) -> (Result, Feedback) + where + T: serde::Serialize, + Handler: FnOnce( + &mut LanguageServerEngine>, + ) -> engine::Response>, { match self.router.project_for_path(path) { Ok(Some(project)) => { @@ -251,33 +281,47 @@ where compilation, } = handler(&mut project.engine); match result { - Ok(value) => { + Ok(Ok(value)) => { let feedback = project.feedback.response(compilation, warnings); let json = serde_json::to_value(value).expect("response to json"); - (json, feedback) + (Ok(json), feedback) + } + Ok(Err(error)) => { + let feedback = project.feedback.response(compilation, warnings); + (Err(error), feedback) } Err(e) => { let feedback = project.feedback.build_with_error(e, compilation, warnings); - (Json::Null, feedback) + (Ok(Json::Null), feedback) } } } - Ok(None) => (Json::Null, Feedback::default()), + Ok(None) => (Ok(Json::Null), Feedback::default()), - Err(error) => (Json::Null, self.outside_of_project_feedback.error(error)), + Err(error) => ( + Ok(Json::Null), + self.outside_of_project_feedback.error(error), + ), } } - fn path_error_response(&mut self, path: Utf8PathBuf, error: crate::Error) -> (Json, Feedback) { + fn path_error_response( + &mut self, + path: Utf8PathBuf, + error: crate::Error, + ) -> (Result, Feedback) { let feedback = match self.router.project_for_path(path) { Ok(Some(project)) => project.feedback.error(error), Ok(None) | Err(_) => self.outside_of_project_feedback.error(error), }; - (Json::Null, feedback) + (Ok(Json::Null), feedback) } - fn format(&mut self, params: lsp::DocumentFormattingParams) -> (Json, Feedback) { + fn format( + &mut self, + params: lsp::DocumentFormattingParams, + ) -> (Result, Feedback) { let path = super::path(¶ms.text_document.uri); let mut new_text = String::new(); @@ -298,15 +342,18 @@ where }; let json = serde_json::to_value(vec![edit]).expect("to JSON value"); - (json, Feedback::default()) + (Ok(json), Feedback::default()) } - fn hover(&mut self, params: lsp::HoverParams) -> (Json, Feedback) { + fn hover(&mut self, params: lsp::HoverParams) -> (Result, Feedback) { let path = super::path(¶ms.text_document_position_params.text_document.uri); self.respond_with_engine(path, |engine| engine.hover(params)) } - fn goto_definition(&mut self, params: lsp::GotoDefinitionParams) -> (Json, Feedback) { + fn goto_definition( + &mut self, + params: lsp::GotoDefinitionParams, + ) -> (Result, Feedback) { let path = super::path(¶ms.text_document_position_params.text_document.uri); self.respond_with_engine(path, |engine| engine.goto_definition(params)) } @@ -314,12 +361,15 @@ where fn goto_type_definition( &mut self, params: lsp_types::GotoDefinitionParams, - ) -> (Json, Feedback) { + ) -> (Result, Feedback) { let path = super::path(¶ms.text_document_position_params.text_document.uri); self.respond_with_engine(path, |engine| engine.goto_type_definition(params)) } - fn completion(&mut self, params: lsp::CompletionParams) -> (Json, Feedback) { + fn completion( + &mut self, + params: lsp::CompletionParams, + ) -> (Result, Feedback) { let path = super::path(¶ms.text_document_position.text_document.uri); let src = match self.io.read(&path) { @@ -331,32 +381,52 @@ where }) } - fn signature_help(&mut self, params: lsp_types::SignatureHelpParams) -> (Json, Feedback) { + fn signature_help( + &mut self, + params: lsp_types::SignatureHelpParams, + ) -> (Result, Feedback) { let path = super::path(¶ms.text_document_position_params.text_document.uri); self.respond_with_engine(path, |engine| engine.signature_help(params)) } - fn code_action(&mut self, params: lsp::CodeActionParams) -> (Json, Feedback) { + fn code_action( + &mut self, + params: lsp::CodeActionParams, + ) -> (Result, Feedback) { let path = super::path(¶ms.text_document.uri); self.respond_with_engine(path, |engine| engine.code_actions(params)) } - fn document_symbol(&mut self, params: lsp::DocumentSymbolParams) -> (Json, Feedback) { + fn document_symbol( + &mut self, + params: lsp::DocumentSymbolParams, + ) -> (Result, Feedback) { let path = super::path(¶ms.text_document.uri); self.respond_with_engine(path, |engine| engine.document_symbol(params)) } - fn prepare_rename(&mut self, params: lsp::TextDocumentPositionParams) -> (Json, Feedback) { + fn prepare_rename( + &mut self, + params: lsp::TextDocumentPositionParams, + ) -> (Result, Feedback) { let path = super::path(¶ms.text_document.uri); self.respond_with_engine(path, |engine| engine.prepare_rename(params)) } - fn rename(&mut self, params: lsp::RenameParams) -> (Json, Feedback) { + fn rename(&mut self, params: lsp::RenameParams) -> (Result, Feedback) { let path = super::path(¶ms.text_document_position.text_document.uri); - self.respond_with_engine(path, |engine| engine.rename(params)) + self.fallible_respond_with_engine( + path, + |engine: &mut LanguageServerEngine>| { + engine.rename(params) + }, + ) } - fn find_references(&mut self, params: lsp_types::ReferenceParams) -> (Json, Feedback) { + fn find_references( + &mut self, + params: lsp_types::ReferenceParams, + ) -> (Result, Feedback) { let path = super::path(¶ms.text_document_position.text_document.uri); self.respond_with_engine(path, |engine| engine.find_references(params)) } diff --git a/compiler-core/src/language_server/tests/rename.rs b/compiler-core/src/language_server/tests/rename.rs index 9ea54effbb9..bfece12a9bb 100644 --- a/compiler-core/src/language_server/tests/rename.rs +++ b/compiler-core/src/language_server/tests/rename.rs @@ -34,7 +34,7 @@ fn rename( new_name: new_name.to_string(), work_done_progress_params: WorkDoneProgressParams::default(), }; - engine.rename(params).result.unwrap() + engine.rename(params).result.unwrap().unwrap_or(None) })?; Some((range, edit))