From 79b07d53ddac1be1c4a22378da6ff9db22568688 Mon Sep 17 00:00:00 2001 From: Tsuf Cohen <39455181+tsuf239@users.noreply.github.com> Date: Wed, 29 May 2024 00:09:01 +0300 Subject: [PATCH] fix(vscode): symbols that cannot be renamed seem to silently fail (#6573) ## Checklist fixes: #6564 listening to the `textDocument/prepareRename` request and allowing rename only for symbols https://www.loom.com/share/7e5d68e50029474f8c23b65ba2cf20aa?sid=5160bb05-b969-4e88-8b60-74422556c673 - [ ] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted) - [ ] Description explains motivation and solution - [ ] Tests added (always) - [ ] Docs updated (only required for features) - [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing *By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*. --- apps/wing/src/commands/lsp.ts | 5 +- libs/wingc/src/lsp/mod.rs | 1 + libs/wingc/src/lsp/rename_prepare.rs | 345 +++++++++++++++++++++++++++ libs/wingc/src/lsp/rename_visitor.rs | 40 +++- libs/wingcompiler/src/wingc.ts | 1 + 5 files changed, 390 insertions(+), 2 deletions(-) create mode 100644 libs/wingc/src/lsp/rename_prepare.rs diff --git a/apps/wing/src/commands/lsp.ts b/apps/wing/src/commands/lsp.ts index 9a4edd1a025..56056a7ace2 100644 --- a/apps/wing/src/commands/lsp.ts +++ b/apps/wing/src/commands/lsp.ts @@ -100,7 +100,7 @@ export async function lsp() { hoverProvider: true, documentSymbolProvider: true, definitionProvider: true, - renameProvider: true, + renameProvider: { prepareProvider: true }, }, }; return result; @@ -219,6 +219,9 @@ export async function lsp() { connection.onRenameRequest(async (params) => { return callWing("wingc_on_rename", params); }); + connection.onPrepareRename(async (params) => { + return callWing("wingc_on_prepare_rename", params); + }); connection.onHover(async (params) => { return callWing("wingc_on_hover", params); }); diff --git a/libs/wingc/src/lsp/mod.rs b/libs/wingc/src/lsp/mod.rs index 03d62253cdb..2753bef8c73 100644 --- a/libs/wingc/src/lsp/mod.rs +++ b/libs/wingc/src/lsp/mod.rs @@ -3,6 +3,7 @@ mod completions; mod document_symbols; mod goto_definition; mod hover; +mod rename_prepare; mod rename_request; mod rename_visitor; mod signature; diff --git a/libs/wingc/src/lsp/rename_prepare.rs b/libs/wingc/src/lsp/rename_prepare.rs new file mode 100644 index 00000000000..8c2ec5cd370 --- /dev/null +++ b/libs/wingc/src/lsp/rename_prepare.rs @@ -0,0 +1,345 @@ +use crate::lsp::sync::PROJECT_DATA; +use crate::visit::Visit; +use crate::wasm_util::extern_json_fn; +use lsp_types::{PrepareRenameResponse, TextDocumentPositionParams}; + +use super::rename_visitor::RenameVisitor; +use super::sync::{check_utf8, WING_TYPES}; + +#[no_mangle] +pub unsafe extern "C" fn wingc_on_prepare_rename(ptr: u32, len: u32) -> u64 { + extern_json_fn(ptr, len, on_prepare_rename) +} + +pub fn on_prepare_rename(params: TextDocumentPositionParams) -> PrepareRenameResponse { + WING_TYPES.with(|types| { + let types = types.borrow(); + PROJECT_DATA.with(|project_data| -> PrepareRenameResponse { + let project_data = project_data.borrow(); + let uri = params.text_document.uri; + let file = check_utf8(uri.to_file_path().expect("LSP only works on real filesystems")); + let scope = project_data.asts.get(&file).unwrap(); + + let position = params.position; + + let mut reference_visitor = RenameVisitor::new(&types); + reference_visitor.visit_scope(scope); + + reference_visitor.prepare_rename(position) + }) + }) +} + +#[cfg(test)] +mod tests { + use crate::lsp::rename_prepare::*; + use crate::lsp::sync::test_utils::*; + + /// Creates a snapshot test for a given wing program's rename_prepare at a given position + /// In the wing program, place a comment "//^" into the text where the "^" is pointing to the desired character position + /// To assert on the target range of the result, place a comment "//-" below the target, with additional "-" characters to extend the range + /// + /// First parameter will be the name of the tests, as well as the identifier to use for the list of completion in the asserts (see last parameter) + /// Second parameter is the wing code block as a string literal + /// After the first two parameters, additional statements can optionally be provided to assert on the returned data. + /// + /// Result is a PrepareRenameResponse + macro_rules! test_rename_prepare { + ($name:ident, $code:literal) => { + test_rename_prepare!($name, $code,); + }; + ($name:ident, $code:literal, $($assertion:stmt)*) => { + #[test] + fn $name() { + // NOTE: this is needed for debugging to work regardless of where you run the test + std::env::set_current_dir(env!("CARGO_MANIFEST_DIR")).unwrap(); + + let text_document_position_params = load_file_with_contents($code); + + let $name = on_prepare_rename(TextDocumentPositionParams { + text_document: text_document_position_params.text_document.clone(), + position: text_document_position_params.position.clone(), + }); + + let ranges = get_ranges($code); + + match $name { + PrepareRenameResponse::DefaultBehavior{default_behavior} => { + assert_eq!( + ranges.len(), + 0, + "target_range did not match" + ); + assert_eq!( + default_behavior, + false, + "target_range did not match" + ); + } + PrepareRenameResponse::Range(range) => { + assert_eq!( + ranges[0], + range, + "target_range did not match" + ); + } + PrepareRenameResponse::RangeWithPlaceholder{range, ..} => { + assert_eq!( + ranges[0], + range, + "target_range did not match" + ); + } + } + + $($assertion)* + } + }; + } + + test_rename_prepare!( + cant_rename_equal_sign, + r#" +let thing = "thing"; +let thing2 = thing; + //^ + "# + ); + + test_rename_prepare!( + cant_rename_let, + r#" +let thing = "thing"; +//^ +let thing2 = thing; + "# + ); + + test_rename_prepare!( + cant_rename_str, + r#" +let thing = "thing"; + //^ +let thing2 = thing; + "# + ); + + test_rename_prepare!( + cant_rename_semicolon, + r#" +let thing = "thing"; + //^ +let thing2 = thing; + "# + ); + // + + test_rename_prepare!( + cant_rename_numbers, + r#" +let var thing = 6; + //^ + "# + ); + + test_rename_prepare!( + cant_rename_log, + r#" + log("hi!") + //^ + + "# + ); + + test_rename_prepare!( + can_rename_symbol, + r#" + let var thing = 6; + if (thing > 8) { + thing = 6; + + } else { + if (thing < 0) { + let thing = 0; + log(thing); + //----^ + } + log(thing) + } + "# + ); + + test_rename_prepare!( + cant_rename_class_keyword, + r#" +class User { +//^ + name: str; +} + "# + ); + + test_rename_prepare!( + cant_rename_for, + r#" + bring cloud; + + let buckets = [new cloud.Bucket()]; + for bucket in buckets { + //^ + bucket.addObject("bucket","bucket"); + log(bucket.toString()); + } + "# + ); + + test_rename_prepare!( + cant_rename_in, + r#" + bring cloud; + + let buckets = [new cloud.Bucket()]; + for bucket in buckets { + //^ + bucket.addObject("bucket","bucket"); + log(bucket.toString()); + } + "# + ); + + test_rename_prepare!( + can_rename_vars, + r#" + bring cloud; + + let buckets = [new cloud.Bucket()]; + //------^ + "# + ); + + // TODO: not supported yet - therefore disabled + test_rename_prepare!( + cant_rename_namespace, + r#" + bring cloud; + //^ + let bucket = new cloud.Bucket(); + "# + ); + + // TODO: not supported yet - therefore disabled + test_rename_prepare!( + cant_rename_namespace_from_property, + r#" + bring cloud; + let bucket = new cloud.Bucket(); + //^ + "# + ); + + test_rename_prepare!( + cant_rename_comments, + r#" + bring cloud; + // let bucket = new cloud.Bucket(); + //^ + "# + ); + + test_rename_prepare!( + cant_rename_tests, + r#" + test "this is a test" { + //^ + assert(true); + } + "# + ); + + // TODO: not supported yet- therefore disabled + test_rename_prepare!( + cant_rename_struct_key, + r#" + struct user { + name: str; + //^ + } + + let a: user = {name: "a"}; + "# + ); + + test_rename_prepare!( + cant_rename_struct_keyword, + r#" + struct user { + //^ + name: str; + } + + let a: user = {name: "a"}; + "# + ); + // TODO: not supported yet - therefore disabled + test_rename_prepare!( + cant_rename_interface_key, + r#" + interface ICat { + inflight stepOnKeyboard(): str; + //^ + } + + class Cat impl ICat { + pub inflight stepOnKeyboard(): str { + + return "/..,.o0e"; + } + } + + let hershy = new Cat(); + + test "cat misbehaves" { + hershy.stepOnKeyboard(); + } + "# + ); + + test_rename_prepare!( + cant_rename_interface_keyword, + r#" + interface ICat { + //^ + inflight stepOnKeyboard(): str; + } + + class Cat impl ICat { + pub inflight stepOnKeyboard(): str { + + return "/..,.o0e"; + } + } + + let hershy = new Cat(); + + test "cat misbehaves" { + hershy.stepOnKeyboard(); + } + "# + ); + + test_rename_prepare!( + cant_rename_types, + r#" + let a: str = "hi"; + //^ + "# + ); + + test_rename_prepare!( + cant_rename_json_val, + r#" + let j = {a: "hi"}; + //^ + "# + ); +} diff --git a/libs/wingc/src/lsp/rename_visitor.rs b/libs/wingc/src/lsp/rename_visitor.rs index 5e87e1f2abe..5d5160a1a1f 100644 --- a/libs/wingc/src/lsp/rename_visitor.rs +++ b/libs/wingc/src/lsp/rename_visitor.rs @@ -1,4 +1,4 @@ -use lsp_types::{Position, Range, TextEdit}; +use lsp_types::{Position, PrepareRenameResponse, Range, TextEdit}; use crate::diagnostic::WingLocation; use crate::type_check::symbol_env::LookupResult; @@ -100,6 +100,44 @@ impl<'a> RenameVisitor<'a> { } vec![] } + + fn prepare_symbol_rename(&self, symbol: &Symbol) -> PrepareRenameResponse { + return PrepareRenameResponse::RangeWithPlaceholder { + range: Range { + start: Position { + line: symbol.span.start.line, + character: symbol.span.start.col, + }, + end: Position { + line: symbol.span.end.line, + character: symbol.span.end.col, + }, + }, + placeholder: symbol.name.clone(), + }; + } + + pub fn prepare_rename(&mut self, position: Position) -> PrepareRenameResponse { + let location = WingLocation { + line: position.line, + col: position.character, + }; + for symbol in &self.linked_symbols { + if symbol.symbol.span.contains_location(&location) { + return self.prepare_symbol_rename(&symbol.symbol); + } + + // to remove the lock we must get out of the for loop + for child in symbol.references.iter() { + if child.span.contains_location(&location) { + return self.prepare_symbol_rename(child); + } + } + } + PrepareRenameResponse::DefaultBehavior { + default_behavior: false, + } + } } impl<'a> VisitorWithContext for RenameVisitor<'a> { diff --git a/libs/wingcompiler/src/wingc.ts b/libs/wingcompiler/src/wingc.ts index 899a0c48828..72badc4ffde 100644 --- a/libs/wingcompiler/src/wingc.ts +++ b/libs/wingcompiler/src/wingc.ts @@ -14,6 +14,7 @@ export type WingCompilerFunction = | "wingc_on_goto_definition" | "wingc_on_document_symbol" | "wingc_on_rename" + | "wingc_on_prepare_rename" | "wingc_on_semantic_tokens" | "wingc_on_hover" | "wingc_on_code_action";