From e90931a2047e6e38f173e2e2f2a24ad648e92e9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=BCdiger=20Herrmann?= Date: Mon, 24 Aug 2020 16:37:45 +0200 Subject: Show reason for failed rename refactoring Return an error with a meaningful message for requests to `textDocument/rename` if the operation cannot be performed. Pass errors raised by rename handling code to the LSP runtime. As a consequence, the VS Code client shows and logs the request as if a server-side programming error occured. Resolves https://github.com/rust-analyzer/rust-analyzer/issues/3981 --- crates/ide/src/lib.rs | 6 +- crates/ide/src/references.rs | 1 + crates/ide/src/references/rename.rs | 175 ++++++++++++++++++++++++++--------- crates/rust-analyzer/src/handlers.rs | 17 +--- 4 files changed, 138 insertions(+), 61 deletions(-) diff --git a/crates/ide/src/lib.rs b/crates/ide/src/lib.rs index 57f3581b6..d54c06b14 100644 --- a/crates/ide/src/lib.rs +++ b/crates/ide/src/lib.rs @@ -77,7 +77,9 @@ pub use crate::{ hover::{HoverAction, HoverConfig, HoverGotoTypeData, HoverResult}, inlay_hints::{InlayHint, InlayHintsConfig, InlayKind}, markup::Markup, - references::{Declaration, Reference, ReferenceAccess, ReferenceKind, ReferenceSearchResult}, + references::{ + Declaration, Reference, ReferenceAccess, ReferenceKind, ReferenceSearchResult, RenameError, + }, runnables::{Runnable, RunnableKind, TestId}, syntax_highlighting::{ Highlight, HighlightModifier, HighlightModifiers, HighlightTag, HighlightedRange, @@ -490,7 +492,7 @@ impl Analysis { &self, position: FilePosition, new_name: &str, - ) -> Cancelable>> { + ) -> Cancelable, RenameError>> { self.with_db(|db| references::rename(db, position, new_name)) } diff --git a/crates/ide/src/references.rs b/crates/ide/src/references.rs index 9315f7354..f65a05ea3 100644 --- a/crates/ide/src/references.rs +++ b/crates/ide/src/references.rs @@ -26,6 +26,7 @@ use syntax::{ use crate::{display::TryToNav, FilePosition, FileRange, NavigationTarget, RangeInfo}; pub(crate) use self::rename::rename; +pub use self::rename::RenameError; pub use ide_db::search::{Reference, ReferenceAccess, ReferenceKind}; diff --git a/crates/ide/src/references/rename.rs b/crates/ide/src/references/rename.rs index 8cbe1ae5a..f3b5cfc8c 100644 --- a/crates/ide/src/references/rename.rs +++ b/crates/ide/src/references/rename.rs @@ -6,11 +6,16 @@ use ide_db::{ defs::{classify_name, classify_name_ref, Definition, NameClass, NameRefClass}, RootDatabase, }; -use std::convert::TryInto; + +use std::{ + convert::TryInto, + error::Error, + fmt::{self, Display}, +}; use syntax::{ algo::find_node_at_offset, ast::{self, NameOwner}, - lex_single_valid_syntax_kind, match_ast, AstNode, SyntaxKind, SyntaxNode, SyntaxToken, + lex_single_syntax_kind, match_ast, AstNode, SyntaxKind, SyntaxNode, SyntaxToken, }; use test_utils::mark; use text_edit::TextEdit; @@ -20,17 +25,37 @@ use crate::{ SourceChange, SourceFileEdit, TextRange, TextSize, }; +#[derive(Debug)] +pub struct RenameError(pub(crate) String); + +impl fmt::Display for RenameError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + Display::fmt(&self.0, f) + } +} + +impl Error for RenameError {} + pub(crate) fn rename( db: &RootDatabase, position: FilePosition, new_name: &str, -) -> Option> { +) -> Result, RenameError> { let sema = Semantics::new(db); - match lex_single_valid_syntax_kind(new_name)? { - SyntaxKind::IDENT | SyntaxKind::UNDERSCORE => (), - SyntaxKind::SELF_KW => return rename_to_self(&sema, position), - _ => return None, + match lex_single_syntax_kind(new_name) { + Some(res) => match res { + (SyntaxKind::IDENT, _) => (), + (SyntaxKind::UNDERSCORE, _) => (), + (SyntaxKind::SELF_KW, _) => return rename_to_self(&sema, position), + (_, Some(syntax_error)) => { + return Err(RenameError(format!("Invalid name `{}`: {}", new_name, syntax_error))) + } + (_, None) => { + return Err(RenameError(format!("Invalid name `{}`: not an identifier", new_name))) + } + }, + None => return Err(RenameError(format!("Invalid name `{}`: not an identifier", new_name))), } let source_file = sema.parse(position.file_id); @@ -103,7 +128,7 @@ fn rename_mod( position: FilePosition, module: Module, new_name: &str, -) -> Option> { +) -> Result, RenameError> { let mut source_file_edits = Vec::new(); let mut file_system_edits = Vec::new(); @@ -125,7 +150,7 @@ fn rename_mod( if let Some(src) = module.declaration_source(sema.db) { let file_id = src.file_id.original_file(sema.db); - let name = src.value.name()?; + let name = src.value.name().unwrap(); let edit = SourceFileEdit { file_id, edit: TextEdit::replace(name.syntax().text_range(), new_name.into()), @@ -133,35 +158,40 @@ fn rename_mod( source_file_edits.push(edit); } - let RangeInfo { range, info: refs } = find_all_refs(sema, position, None)?; + let RangeInfo { range, info: refs } = find_all_refs(sema, position, None) + .ok_or_else(|| RenameError("No references found at position".to_string()))?; let ref_edits = refs .references .into_iter() .map(|reference| source_edit_from_reference(reference, new_name)); source_file_edits.extend(ref_edits); - Some(RangeInfo::new(range, SourceChange::from_edits(source_file_edits, file_system_edits))) + Ok(RangeInfo::new(range, SourceChange::from_edits(source_file_edits, file_system_edits))) } fn rename_to_self( sema: &Semantics, position: FilePosition, -) -> Option> { +) -> Result, RenameError> { let source_file = sema.parse(position.file_id); let syn = source_file.syntax(); - let fn_def = find_node_at_offset::(syn, position.offset)?; - let params = fn_def.param_list()?; + let fn_def = find_node_at_offset::(syn, position.offset) + .ok_or_else(|| RenameError("No surrounding method declaration found".to_string()))?; + let params = + fn_def.param_list().ok_or_else(|| RenameError("Method has no parameters".to_string()))?; if params.self_param().is_some() { - return None; // method already has self param + return Err(RenameError("Method already has a self parameter".to_string())); } - let first_param = params.params().next()?; + let first_param = + params.params().next().ok_or_else(|| RenameError("Method has no parameters".into()))?; let mutable = match first_param.ty() { Some(ast::Type::RefType(rt)) => rt.mut_token().is_some(), - _ => return None, // not renaming other types + _ => return Err(RenameError("Not renaming other types".to_string())), }; - let RangeInfo { range, info: refs } = find_all_refs(sema, position, None)?; + let RangeInfo { range, info: refs } = find_all_refs(sema, position, None) + .ok_or_else(|| RenameError("No reference found at position".to_string()))?; let param_range = first_param.syntax().text_range(); let (param_ref, usages): (Vec, Vec) = refs @@ -169,7 +199,7 @@ fn rename_to_self( .partition(|reference| param_range.intersect(reference.file_range.range).is_some()); if param_ref.is_empty() { - return None; + return Err(RenameError("Parameter to rename not found".to_string())); } let mut edits = usages @@ -185,7 +215,7 @@ fn rename_to_self( ), }); - Some(RangeInfo::new(range, SourceChange::from(edits))) + Ok(RangeInfo::new(range, SourceChange::from(edits))) } fn text_edit_from_self_param( @@ -216,12 +246,13 @@ fn rename_self_to_param( position: FilePosition, self_token: SyntaxToken, new_name: &str, -) -> Option> { +) -> Result, RenameError> { let source_file = sema.parse(position.file_id); let syn = source_file.syntax(); let text = sema.db.file_text(position.file_id); - let fn_def = find_node_at_offset::(syn, position.offset)?; + let fn_def = find_node_at_offset::(syn, position.offset) + .ok_or_else(|| RenameError("No surrounding method declaration found".to_string()))?; let search_range = fn_def.syntax().text_range(); let mut edits: Vec = vec![]; @@ -235,7 +266,8 @@ fn rename_self_to_param( syn.token_at_offset(offset).find(|t| t.kind() == SyntaxKind::SELF_KW) { let edit = if let Some(ref self_param) = ast::SelfParam::cast(usage.parent()) { - text_edit_from_self_param(syn, self_param, new_name)? + text_edit_from_self_param(syn, self_param, new_name) + .ok_or_else(|| RenameError("No target type found".to_string()))? } else { TextEdit::replace(usage.text_range(), String::from(new_name)) }; @@ -246,15 +278,18 @@ fn rename_self_to_param( let range = ast::SelfParam::cast(self_token.parent()) .map_or(self_token.text_range(), |p| p.syntax().text_range()); - Some(RangeInfo::new(range, SourceChange::from(edits))) + Ok(RangeInfo::new(range, SourceChange::from(edits))) } fn rename_reference( sema: &Semantics, position: FilePosition, new_name: &str, -) -> Option> { - let RangeInfo { range, info: refs } = find_all_refs(sema, position, None)?; +) -> Result, RenameError> { + let RangeInfo { range, info: refs } = match find_all_refs(sema, position, None) { + Some(range_info) => range_info, + None => return Err(RenameError("No references found at position".to_string())), + }; let edit = refs .into_iter() @@ -262,10 +297,10 @@ fn rename_reference( .collect::>(); if edit.is_empty() { - return None; + return Err(RenameError("No references found at position".to_string())); } - Some(RangeInfo::new(range, SourceChange::from(edit))) + Ok(RangeInfo::new(range, SourceChange::from(edit))) } #[cfg(test)] @@ -280,25 +315,45 @@ mod tests { fn check(new_name: &str, ra_fixture_before: &str, ra_fixture_after: &str) { let ra_fixture_after = &trim_indent(ra_fixture_after); let (analysis, position) = fixture::position(ra_fixture_before); - let source_change = analysis.rename(position, new_name).unwrap(); - let mut text_edit_builder = TextEdit::builder(); - let mut file_id: Option = None; - if let Some(change) = source_change { - for edit in change.info.source_file_edits { - file_id = Some(edit.file_id); - for indel in edit.edit.into_iter() { - text_edit_builder.replace(indel.delete, indel.insert); + let rename_result = analysis + .rename(position, new_name) + .unwrap_or_else(|err| panic!("Rename to '{}' was cancelled: {}", new_name, err)); + match rename_result { + Ok(source_change) => { + let mut text_edit_builder = TextEdit::builder(); + let mut file_id: Option = None; + for edit in source_change.info.source_file_edits { + file_id = Some(edit.file_id); + for indel in edit.edit.into_iter() { + text_edit_builder.replace(indel.delete, indel.insert); + } } + let mut result = analysis.file_text(file_id.unwrap()).unwrap().to_string(); + text_edit_builder.finish().apply(&mut result); + assert_eq_text!(ra_fixture_after, &*result); } - } - let mut result = analysis.file_text(file_id.unwrap()).unwrap().to_string(); - text_edit_builder.finish().apply(&mut result); - assert_eq_text!(ra_fixture_after, &*result); + Err(err) => { + if ra_fixture_after.starts_with("error:") { + let error_message = ra_fixture_after + .chars() + .into_iter() + .skip("error:".len()) + .collect::(); + assert_eq!(error_message.trim(), err.to_string()); + return; + } else { + panic!("Rename to '{}' failed unexpectedly: {}", new_name, err) + } + } + }; } fn check_expect(new_name: &str, ra_fixture: &str, expect: Expect) { let (analysis, position) = fixture::position(ra_fixture); - let source_change = analysis.rename(position, new_name).unwrap().unwrap(); + let source_change = analysis + .rename(position, new_name) + .unwrap() + .expect("Expect returned RangeInfo to be Some, but was None"); expect.assert_debug_eq(&source_change) } @@ -313,11 +368,30 @@ mod tests { } #[test] - fn test_rename_to_invalid_identifier() { - let (analysis, position) = fixture::position(r#"fn main() { let i<|> = 1; }"#); - let new_name = "invalid!"; - let source_change = analysis.rename(position, new_name).unwrap(); - assert!(source_change.is_none()); + fn test_rename_to_invalid_identifier1() { + check( + "invalid!", + r#"fn main() { let i<|> = 1; }"#, + "error: Invalid name `invalid!`: not an identifier", + ); + } + + #[test] + fn test_rename_to_invalid_identifier2() { + check( + "multiple tokens", + r#"fn main() { let i<|> = 1; }"#, + "error: Invalid name `multiple tokens`: not an identifier", + ); + } + + #[test] + fn test_rename_to_invalid_identifier3() { + check( + "let", + r#"fn main() { let i<|> = 1; }"#, + "error: Invalid name `let`: not an identifier", + ); } #[test] @@ -349,6 +423,15 @@ fn main() { ); } + #[test] + fn test_rename_unresolved_reference() { + check( + "new_name", + r#"fn main() { let _ = unresolved_ref<|>; }"#, + "error: No references found at position", + ); + } + #[test] fn test_rename_for_macro_args() { check( diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index 468655f9c..4e3340b0d 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -646,14 +646,9 @@ pub(crate) fn handle_prepare_rename( let _p = profile::span("handle_prepare_rename"); let position = from_proto::file_position(&snap, params)?; - let optional_change = snap.analysis.rename(position, "dummy")?; - let range = match optional_change { - None => return Ok(None), - Some(it) => it.range, - }; - + let change = snap.analysis.rename(position, "dummy")??; let line_index = snap.analysis.file_line_index(position.file_id)?; - let range = to_proto::range(&line_index, range); + let range = to_proto::range(&line_index, change.range); Ok(Some(PrepareRenameResponse::Range(range))) } @@ -672,12 +667,8 @@ pub(crate) fn handle_rename( .into()); } - let optional_change = snap.analysis.rename(position, &*params.new_name)?; - let source_change = match optional_change { - None => return Ok(None), - Some(it) => it.info, - }; - let workspace_edit = to_proto::workspace_edit(&snap, source_change)?; + let change = snap.analysis.rename(position, &*params.new_name)??; + let workspace_edit = to_proto::workspace_edit(&snap, change.info)?; Ok(Some(workspace_edit)) } -- cgit v1.2.3