diff options
author | bors[bot] <26634292+bors[bot]@users.noreply.github.com> | 2020-10-10 12:59:25 +0100 |
---|---|---|
committer | GitHub <[email protected]> | 2020-10-10 12:59:25 +0100 |
commit | b7596d248382aa88b81da4dd6c7598993bc4b787 (patch) | |
tree | 0ef92bcd580446c4ea44902f0f959d547a698943 /crates/ide | |
parent | 05261f5aeb1dcf01c5dfcf3e01826e57b1249ca9 (diff) | |
parent | e90931a2047e6e38f173e2e2f2a24ad648e92e9d (diff) |
Merge #5858
5858: Draft: Show reason for failed rename refactoring r=rherrmann a=rherrmann
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.
Screenshot of a rename error showing in VS Code
![invalid-rename-ui](https://user-images.githubusercontent.com/607182/91059560-2c08a380-e62a-11ea-9297-f092db935a3b.png)
I would kindly ask to get feedback from the maintainers if they can spare the time:
* Is the general direction of the proposed changes acceptable?
* I'm new to Rust. The code feels clumsy and redundant, please suggest improvements
if you find the time for. E.g. is there a simple replacement for `RenameError`?
* Should presenting the error with proper severity (i.e. not as a programming error) be part of this change or in a followup change?
See https://github.com/rust-analyzer/rust-analyzer/issues/3981
Co-authored-by: RĂ¼diger Herrmann <[email protected]>
Diffstat (limited to 'crates/ide')
-rw-r--r-- | crates/ide/src/lib.rs | 6 | ||||
-rw-r--r-- | crates/ide/src/references.rs | 1 | ||||
-rw-r--r-- | crates/ide/src/references/rename.rs | 175 |
3 files changed, 134 insertions, 48 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::{ | |||
77 | hover::{HoverAction, HoverConfig, HoverGotoTypeData, HoverResult}, | 77 | hover::{HoverAction, HoverConfig, HoverGotoTypeData, HoverResult}, |
78 | inlay_hints::{InlayHint, InlayHintsConfig, InlayKind}, | 78 | inlay_hints::{InlayHint, InlayHintsConfig, InlayKind}, |
79 | markup::Markup, | 79 | markup::Markup, |
80 | references::{Declaration, Reference, ReferenceAccess, ReferenceKind, ReferenceSearchResult}, | 80 | references::{ |
81 | Declaration, Reference, ReferenceAccess, ReferenceKind, ReferenceSearchResult, RenameError, | ||
82 | }, | ||
81 | runnables::{Runnable, RunnableKind, TestId}, | 83 | runnables::{Runnable, RunnableKind, TestId}, |
82 | syntax_highlighting::{ | 84 | syntax_highlighting::{ |
83 | Highlight, HighlightModifier, HighlightModifiers, HighlightTag, HighlightedRange, | 85 | Highlight, HighlightModifier, HighlightModifiers, HighlightTag, HighlightedRange, |
@@ -490,7 +492,7 @@ impl Analysis { | |||
490 | &self, | 492 | &self, |
491 | position: FilePosition, | 493 | position: FilePosition, |
492 | new_name: &str, | 494 | new_name: &str, |
493 | ) -> Cancelable<Option<RangeInfo<SourceChange>>> { | 495 | ) -> Cancelable<Result<RangeInfo<SourceChange>, RenameError>> { |
494 | self.with_db(|db| references::rename(db, position, new_name)) | 496 | self.with_db(|db| references::rename(db, position, new_name)) |
495 | } | 497 | } |
496 | 498 | ||
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::{ | |||
26 | use crate::{display::TryToNav, FilePosition, FileRange, NavigationTarget, RangeInfo}; | 26 | use crate::{display::TryToNav, FilePosition, FileRange, NavigationTarget, RangeInfo}; |
27 | 27 | ||
28 | pub(crate) use self::rename::rename; | 28 | pub(crate) use self::rename::rename; |
29 | pub use self::rename::RenameError; | ||
29 | 30 | ||
30 | pub use ide_db::search::{Reference, ReferenceAccess, ReferenceKind}; | 31 | pub use ide_db::search::{Reference, ReferenceAccess, ReferenceKind}; |
31 | 32 | ||
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::{ | |||
6 | defs::{classify_name, classify_name_ref, Definition, NameClass, NameRefClass}, | 6 | defs::{classify_name, classify_name_ref, Definition, NameClass, NameRefClass}, |
7 | RootDatabase, | 7 | RootDatabase, |
8 | }; | 8 | }; |
9 | use std::convert::TryInto; | 9 | |
10 | use std::{ | ||
11 | convert::TryInto, | ||
12 | error::Error, | ||
13 | fmt::{self, Display}, | ||
14 | }; | ||
10 | use syntax::{ | 15 | use syntax::{ |
11 | algo::find_node_at_offset, | 16 | algo::find_node_at_offset, |
12 | ast::{self, NameOwner}, | 17 | ast::{self, NameOwner}, |
13 | lex_single_valid_syntax_kind, match_ast, AstNode, SyntaxKind, SyntaxNode, SyntaxToken, | 18 | lex_single_syntax_kind, match_ast, AstNode, SyntaxKind, SyntaxNode, SyntaxToken, |
14 | }; | 19 | }; |
15 | use test_utils::mark; | 20 | use test_utils::mark; |
16 | use text_edit::TextEdit; | 21 | use text_edit::TextEdit; |
@@ -20,17 +25,37 @@ use crate::{ | |||
20 | SourceChange, SourceFileEdit, TextRange, TextSize, | 25 | SourceChange, SourceFileEdit, TextRange, TextSize, |
21 | }; | 26 | }; |
22 | 27 | ||
28 | #[derive(Debug)] | ||
29 | pub struct RenameError(pub(crate) String); | ||
30 | |||
31 | impl fmt::Display for RenameError { | ||
32 | fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
33 | Display::fmt(&self.0, f) | ||
34 | } | ||
35 | } | ||
36 | |||
37 | impl Error for RenameError {} | ||
38 | |||
23 | pub(crate) fn rename( | 39 | pub(crate) fn rename( |
24 | db: &RootDatabase, | 40 | db: &RootDatabase, |
25 | position: FilePosition, | 41 | position: FilePosition, |
26 | new_name: &str, | 42 | new_name: &str, |
27 | ) -> Option<RangeInfo<SourceChange>> { | 43 | ) -> Result<RangeInfo<SourceChange>, RenameError> { |
28 | let sema = Semantics::new(db); | 44 | let sema = Semantics::new(db); |
29 | 45 | ||
30 | match lex_single_valid_syntax_kind(new_name)? { | 46 | match lex_single_syntax_kind(new_name) { |
31 | SyntaxKind::IDENT | SyntaxKind::UNDERSCORE => (), | 47 | Some(res) => match res { |
32 | SyntaxKind::SELF_KW => return rename_to_self(&sema, position), | 48 | (SyntaxKind::IDENT, _) => (), |
33 | _ => return None, | 49 | (SyntaxKind::UNDERSCORE, _) => (), |
50 | (SyntaxKind::SELF_KW, _) => return rename_to_self(&sema, position), | ||
51 | (_, Some(syntax_error)) => { | ||
52 | return Err(RenameError(format!("Invalid name `{}`: {}", new_name, syntax_error))) | ||
53 | } | ||
54 | (_, None) => { | ||
55 | return Err(RenameError(format!("Invalid name `{}`: not an identifier", new_name))) | ||
56 | } | ||
57 | }, | ||
58 | None => return Err(RenameError(format!("Invalid name `{}`: not an identifier", new_name))), | ||
34 | } | 59 | } |
35 | 60 | ||
36 | let source_file = sema.parse(position.file_id); | 61 | let source_file = sema.parse(position.file_id); |
@@ -103,7 +128,7 @@ fn rename_mod( | |||
103 | position: FilePosition, | 128 | position: FilePosition, |
104 | module: Module, | 129 | module: Module, |
105 | new_name: &str, | 130 | new_name: &str, |
106 | ) -> Option<RangeInfo<SourceChange>> { | 131 | ) -> Result<RangeInfo<SourceChange>, RenameError> { |
107 | let mut source_file_edits = Vec::new(); | 132 | let mut source_file_edits = Vec::new(); |
108 | let mut file_system_edits = Vec::new(); | 133 | let mut file_system_edits = Vec::new(); |
109 | 134 | ||
@@ -125,7 +150,7 @@ fn rename_mod( | |||
125 | 150 | ||
126 | if let Some(src) = module.declaration_source(sema.db) { | 151 | if let Some(src) = module.declaration_source(sema.db) { |
127 | let file_id = src.file_id.original_file(sema.db); | 152 | let file_id = src.file_id.original_file(sema.db); |
128 | let name = src.value.name()?; | 153 | let name = src.value.name().unwrap(); |
129 | let edit = SourceFileEdit { | 154 | let edit = SourceFileEdit { |
130 | file_id, | 155 | file_id, |
131 | edit: TextEdit::replace(name.syntax().text_range(), new_name.into()), | 156 | edit: TextEdit::replace(name.syntax().text_range(), new_name.into()), |
@@ -133,35 +158,40 @@ fn rename_mod( | |||
133 | source_file_edits.push(edit); | 158 | source_file_edits.push(edit); |
134 | } | 159 | } |
135 | 160 | ||
136 | let RangeInfo { range, info: refs } = find_all_refs(sema, position, None)?; | 161 | let RangeInfo { range, info: refs } = find_all_refs(sema, position, None) |
162 | .ok_or_else(|| RenameError("No references found at position".to_string()))?; | ||
137 | let ref_edits = refs | 163 | let ref_edits = refs |
138 | .references | 164 | .references |
139 | .into_iter() | 165 | .into_iter() |
140 | .map(|reference| source_edit_from_reference(reference, new_name)); | 166 | .map(|reference| source_edit_from_reference(reference, new_name)); |
141 | source_file_edits.extend(ref_edits); | 167 | source_file_edits.extend(ref_edits); |
142 | 168 | ||
143 | Some(RangeInfo::new(range, SourceChange::from_edits(source_file_edits, file_system_edits))) | 169 | Ok(RangeInfo::new(range, SourceChange::from_edits(source_file_edits, file_system_edits))) |
144 | } | 170 | } |
145 | 171 | ||
146 | fn rename_to_self( | 172 | fn rename_to_self( |
147 | sema: &Semantics<RootDatabase>, | 173 | sema: &Semantics<RootDatabase>, |
148 | position: FilePosition, | 174 | position: FilePosition, |
149 | ) -> Option<RangeInfo<SourceChange>> { | 175 | ) -> Result<RangeInfo<SourceChange>, RenameError> { |
150 | let source_file = sema.parse(position.file_id); | 176 | let source_file = sema.parse(position.file_id); |
151 | let syn = source_file.syntax(); | 177 | let syn = source_file.syntax(); |
152 | 178 | ||
153 | let fn_def = find_node_at_offset::<ast::Fn>(syn, position.offset)?; | 179 | let fn_def = find_node_at_offset::<ast::Fn>(syn, position.offset) |
154 | let params = fn_def.param_list()?; | 180 | .ok_or_else(|| RenameError("No surrounding method declaration found".to_string()))?; |
181 | let params = | ||
182 | fn_def.param_list().ok_or_else(|| RenameError("Method has no parameters".to_string()))?; | ||
155 | if params.self_param().is_some() { | 183 | if params.self_param().is_some() { |
156 | return None; // method already has self param | 184 | return Err(RenameError("Method already has a self parameter".to_string())); |
157 | } | 185 | } |
158 | let first_param = params.params().next()?; | 186 | let first_param = |
187 | params.params().next().ok_or_else(|| RenameError("Method has no parameters".into()))?; | ||
159 | let mutable = match first_param.ty() { | 188 | let mutable = match first_param.ty() { |
160 | Some(ast::Type::RefType(rt)) => rt.mut_token().is_some(), | 189 | Some(ast::Type::RefType(rt)) => rt.mut_token().is_some(), |
161 | _ => return None, // not renaming other types | 190 | _ => return Err(RenameError("Not renaming other types".to_string())), |
162 | }; | 191 | }; |
163 | 192 | ||
164 | let RangeInfo { range, info: refs } = find_all_refs(sema, position, None)?; | 193 | let RangeInfo { range, info: refs } = find_all_refs(sema, position, None) |
194 | .ok_or_else(|| RenameError("No reference found at position".to_string()))?; | ||
165 | 195 | ||
166 | let param_range = first_param.syntax().text_range(); | 196 | let param_range = first_param.syntax().text_range(); |
167 | let (param_ref, usages): (Vec<Reference>, Vec<Reference>) = refs | 197 | let (param_ref, usages): (Vec<Reference>, Vec<Reference>) = refs |
@@ -169,7 +199,7 @@ fn rename_to_self( | |||
169 | .partition(|reference| param_range.intersect(reference.file_range.range).is_some()); | 199 | .partition(|reference| param_range.intersect(reference.file_range.range).is_some()); |
170 | 200 | ||
171 | if param_ref.is_empty() { | 201 | if param_ref.is_empty() { |
172 | return None; | 202 | return Err(RenameError("Parameter to rename not found".to_string())); |
173 | } | 203 | } |
174 | 204 | ||
175 | let mut edits = usages | 205 | let mut edits = usages |
@@ -185,7 +215,7 @@ fn rename_to_self( | |||
185 | ), | 215 | ), |
186 | }); | 216 | }); |
187 | 217 | ||
188 | Some(RangeInfo::new(range, SourceChange::from(edits))) | 218 | Ok(RangeInfo::new(range, SourceChange::from(edits))) |
189 | } | 219 | } |
190 | 220 | ||
191 | fn text_edit_from_self_param( | 221 | fn text_edit_from_self_param( |
@@ -216,12 +246,13 @@ fn rename_self_to_param( | |||
216 | position: FilePosition, | 246 | position: FilePosition, |
217 | self_token: SyntaxToken, | 247 | self_token: SyntaxToken, |
218 | new_name: &str, | 248 | new_name: &str, |
219 | ) -> Option<RangeInfo<SourceChange>> { | 249 | ) -> Result<RangeInfo<SourceChange>, RenameError> { |
220 | let source_file = sema.parse(position.file_id); | 250 | let source_file = sema.parse(position.file_id); |
221 | let syn = source_file.syntax(); | 251 | let syn = source_file.syntax(); |
222 | 252 | ||
223 | let text = sema.db.file_text(position.file_id); | 253 | let text = sema.db.file_text(position.file_id); |
224 | let fn_def = find_node_at_offset::<ast::Fn>(syn, position.offset)?; | 254 | let fn_def = find_node_at_offset::<ast::Fn>(syn, position.offset) |
255 | .ok_or_else(|| RenameError("No surrounding method declaration found".to_string()))?; | ||
225 | let search_range = fn_def.syntax().text_range(); | 256 | let search_range = fn_def.syntax().text_range(); |
226 | 257 | ||
227 | let mut edits: Vec<SourceFileEdit> = vec![]; | 258 | let mut edits: Vec<SourceFileEdit> = vec![]; |
@@ -235,7 +266,8 @@ fn rename_self_to_param( | |||
235 | syn.token_at_offset(offset).find(|t| t.kind() == SyntaxKind::SELF_KW) | 266 | syn.token_at_offset(offset).find(|t| t.kind() == SyntaxKind::SELF_KW) |
236 | { | 267 | { |
237 | let edit = if let Some(ref self_param) = ast::SelfParam::cast(usage.parent()) { | 268 | let edit = if let Some(ref self_param) = ast::SelfParam::cast(usage.parent()) { |
238 | text_edit_from_self_param(syn, self_param, new_name)? | 269 | text_edit_from_self_param(syn, self_param, new_name) |
270 | .ok_or_else(|| RenameError("No target type found".to_string()))? | ||
239 | } else { | 271 | } else { |
240 | TextEdit::replace(usage.text_range(), String::from(new_name)) | 272 | TextEdit::replace(usage.text_range(), String::from(new_name)) |
241 | }; | 273 | }; |
@@ -246,15 +278,18 @@ fn rename_self_to_param( | |||
246 | let range = ast::SelfParam::cast(self_token.parent()) | 278 | let range = ast::SelfParam::cast(self_token.parent()) |
247 | .map_or(self_token.text_range(), |p| p.syntax().text_range()); | 279 | .map_or(self_token.text_range(), |p| p.syntax().text_range()); |
248 | 280 | ||
249 | Some(RangeInfo::new(range, SourceChange::from(edits))) | 281 | Ok(RangeInfo::new(range, SourceChange::from(edits))) |
250 | } | 282 | } |
251 | 283 | ||
252 | fn rename_reference( | 284 | fn rename_reference( |
253 | sema: &Semantics<RootDatabase>, | 285 | sema: &Semantics<RootDatabase>, |
254 | position: FilePosition, | 286 | position: FilePosition, |
255 | new_name: &str, | 287 | new_name: &str, |
256 | ) -> Option<RangeInfo<SourceChange>> { | 288 | ) -> Result<RangeInfo<SourceChange>, RenameError> { |
257 | let RangeInfo { range, info: refs } = find_all_refs(sema, position, None)?; | 289 | let RangeInfo { range, info: refs } = match find_all_refs(sema, position, None) { |
290 | Some(range_info) => range_info, | ||
291 | None => return Err(RenameError("No references found at position".to_string())), | ||
292 | }; | ||
258 | 293 | ||
259 | let edit = refs | 294 | let edit = refs |
260 | .into_iter() | 295 | .into_iter() |
@@ -262,10 +297,10 @@ fn rename_reference( | |||
262 | .collect::<Vec<_>>(); | 297 | .collect::<Vec<_>>(); |
263 | 298 | ||
264 | if edit.is_empty() { | 299 | if edit.is_empty() { |
265 | return None; | 300 | return Err(RenameError("No references found at position".to_string())); |
266 | } | 301 | } |
267 | 302 | ||
268 | Some(RangeInfo::new(range, SourceChange::from(edit))) | 303 | Ok(RangeInfo::new(range, SourceChange::from(edit))) |
269 | } | 304 | } |
270 | 305 | ||
271 | #[cfg(test)] | 306 | #[cfg(test)] |
@@ -280,25 +315,45 @@ mod tests { | |||
280 | fn check(new_name: &str, ra_fixture_before: &str, ra_fixture_after: &str) { | 315 | fn check(new_name: &str, ra_fixture_before: &str, ra_fixture_after: &str) { |
281 | let ra_fixture_after = &trim_indent(ra_fixture_after); | 316 | let ra_fixture_after = &trim_indent(ra_fixture_after); |
282 | let (analysis, position) = fixture::position(ra_fixture_before); | 317 | let (analysis, position) = fixture::position(ra_fixture_before); |
283 | let source_change = analysis.rename(position, new_name).unwrap(); | 318 | let rename_result = analysis |
284 | let mut text_edit_builder = TextEdit::builder(); | 319 | .rename(position, new_name) |
285 | let mut file_id: Option<FileId> = None; | 320 | .unwrap_or_else(|err| panic!("Rename to '{}' was cancelled: {}", new_name, err)); |
286 | if let Some(change) = source_change { | 321 | match rename_result { |
287 | for edit in change.info.source_file_edits { | 322 | Ok(source_change) => { |
288 | file_id = Some(edit.file_id); | 323 | let mut text_edit_builder = TextEdit::builder(); |
289 | for indel in edit.edit.into_iter() { | 324 | let mut file_id: Option<FileId> = None; |
290 | text_edit_builder.replace(indel.delete, indel.insert); | 325 | for edit in source_change.info.source_file_edits { |
326 | file_id = Some(edit.file_id); | ||
327 | for indel in edit.edit.into_iter() { | ||
328 | text_edit_builder.replace(indel.delete, indel.insert); | ||
329 | } | ||
291 | } | 330 | } |
331 | let mut result = analysis.file_text(file_id.unwrap()).unwrap().to_string(); | ||
332 | text_edit_builder.finish().apply(&mut result); | ||
333 | assert_eq_text!(ra_fixture_after, &*result); | ||
292 | } | 334 | } |
293 | } | 335 | Err(err) => { |
294 | let mut result = analysis.file_text(file_id.unwrap()).unwrap().to_string(); | 336 | if ra_fixture_after.starts_with("error:") { |
295 | text_edit_builder.finish().apply(&mut result); | 337 | let error_message = ra_fixture_after |
296 | assert_eq_text!(ra_fixture_after, &*result); | 338 | .chars() |
339 | .into_iter() | ||
340 | .skip("error:".len()) | ||
341 | .collect::<String>(); | ||
342 | assert_eq!(error_message.trim(), err.to_string()); | ||
343 | return; | ||
344 | } else { | ||
345 | panic!("Rename to '{}' failed unexpectedly: {}", new_name, err) | ||
346 | } | ||
347 | } | ||
348 | }; | ||
297 | } | 349 | } |
298 | 350 | ||
299 | fn check_expect(new_name: &str, ra_fixture: &str, expect: Expect) { | 351 | fn check_expect(new_name: &str, ra_fixture: &str, expect: Expect) { |
300 | let (analysis, position) = fixture::position(ra_fixture); | 352 | let (analysis, position) = fixture::position(ra_fixture); |
301 | let source_change = analysis.rename(position, new_name).unwrap().unwrap(); | 353 | let source_change = analysis |
354 | .rename(position, new_name) | ||
355 | .unwrap() | ||
356 | .expect("Expect returned RangeInfo to be Some, but was None"); | ||
302 | expect.assert_debug_eq(&source_change) | 357 | expect.assert_debug_eq(&source_change) |
303 | } | 358 | } |
304 | 359 | ||
@@ -313,11 +368,30 @@ mod tests { | |||
313 | } | 368 | } |
314 | 369 | ||
315 | #[test] | 370 | #[test] |
316 | fn test_rename_to_invalid_identifier() { | 371 | fn test_rename_to_invalid_identifier1() { |
317 | let (analysis, position) = fixture::position(r#"fn main() { let i<|> = 1; }"#); | 372 | check( |
318 | let new_name = "invalid!"; | 373 | "invalid!", |
319 | let source_change = analysis.rename(position, new_name).unwrap(); | 374 | r#"fn main() { let i<|> = 1; }"#, |
320 | assert!(source_change.is_none()); | 375 | "error: Invalid name `invalid!`: not an identifier", |
376 | ); | ||
377 | } | ||
378 | |||
379 | #[test] | ||
380 | fn test_rename_to_invalid_identifier2() { | ||
381 | check( | ||
382 | "multiple tokens", | ||
383 | r#"fn main() { let i<|> = 1; }"#, | ||
384 | "error: Invalid name `multiple tokens`: not an identifier", | ||
385 | ); | ||
386 | } | ||
387 | |||
388 | #[test] | ||
389 | fn test_rename_to_invalid_identifier3() { | ||
390 | check( | ||
391 | "let", | ||
392 | r#"fn main() { let i<|> = 1; }"#, | ||
393 | "error: Invalid name `let`: not an identifier", | ||
394 | ); | ||
321 | } | 395 | } |
322 | 396 | ||
323 | #[test] | 397 | #[test] |
@@ -350,6 +424,15 @@ fn main() { | |||
350 | } | 424 | } |
351 | 425 | ||
352 | #[test] | 426 | #[test] |
427 | fn test_rename_unresolved_reference() { | ||
428 | check( | ||
429 | "new_name", | ||
430 | r#"fn main() { let _ = unresolved_ref<|>; }"#, | ||
431 | "error: No references found at position", | ||
432 | ); | ||
433 | } | ||
434 | |||
435 | #[test] | ||
353 | fn test_rename_for_macro_args() { | 436 | fn test_rename_for_macro_args() { |
354 | check( | 437 | check( |
355 | "b", | 438 | "b", |