From 86952bf84d2ee47d0a09ef9b423803b2a640daa7 Mon Sep 17 00:00:00 2001 From: Giacomo Cavalieri Date: Sat, 21 Dec 2024 10:37:23 +0100 Subject: [PATCH 1/2] Fix bug with extract variable top level statements --- .../src/language_server/code_action.rs | 54 ++++++++++++++++--- .../src/language_server/tests/action.rs | 24 +++++++++ 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/compiler-core/src/language_server/code_action.rs b/compiler-core/src/language_server/code_action.rs index 3e27bb86773..512bfa44f60 100644 --- a/compiler-core/src/language_server/code_action.rs +++ b/compiler-core/src/language_server/code_action.rs @@ -2605,12 +2605,18 @@ pub struct ExtractVariable<'a> { module: &'a Module, params: &'a CodeActionParams, edits: TextEdits<'a>, - inside_capture_body: bool, + position: Option, selected_expression: Option, statement_before_selected_expression: Option, latest_statement: Option, } +#[derive(PartialEq, Eq, Copy, Clone)] +enum ExtractVariablePosition { + InsideCaptureBody, + TopLevelStatement, +} + impl<'a> ExtractVariable<'a> { pub fn new( module: &'a Module, @@ -2621,7 +2627,7 @@ impl<'a> ExtractVariable<'a> { module, params, edits: TextEdits::new(line_numbers), - inside_capture_body: false, + position: None, selected_expression: None, latest_statement: None, statement_before_selected_expression: None, @@ -2669,21 +2675,50 @@ impl<'ast> ast::visit::Visit<'ast> for ExtractVariable<'ast> { // A capture body is comprised of just a single expression statement // that is inserted by the compiler, we don't really want to put // anything before that; so in this case we avoid tracking it. - if !self.inside_capture_body { + if self.position != Some(ExtractVariablePosition::InsideCaptureBody) { self.latest_statement = Some(stmt.location()); } + + let previous_position = self.position; + self.position = Some(ExtractVariablePosition::TopLevelStatement); ast::visit::visit_typed_statement(self, stmt); + self.position = previous_position; } fn visit_typed_expr(&mut self, expr: &'ast TypedExpr) { let expr_location = expr.location(); let expr_range = self.edits.src_span_to_lsp_range(expr_location); - if within(self.params.range, expr_range) { + + // If the expression is a top level statement we don't want to extract + // it into a variable. It would mean we would turn this: + // + // ```gleam + // pub fn main() { + // let wibble = 1 + // // ^ cursor here + // } + // + // // into: + // + // pub fn main() { + // let value = 1 + // let wibble = value + // } + // ``` + // + // Not all that useful! + // + if self.position != Some(ExtractVariablePosition::TopLevelStatement) + && within(self.params.range, expr_range) + { self.selected_expression = Some(expr_location); self.statement_before_selected_expression = self.latest_statement; } + let previous_position = self.position; + self.position = None; ast::visit::visit_typed_expr(self, expr); + self.position = previous_position; } fn visit_typed_expr_fn( @@ -2695,15 +2730,18 @@ impl<'ast> ast::visit::Visit<'ast> for ExtractVariable<'ast> { body: &'ast [TypedStatement], return_annotation: &'ast Option, ) { - self.inside_capture_body = match kind { + let previous_position = self.position; + self.position = match kind { // If a fn is a capture `int.wibble(1, _)` its body will consist of // just a single expression statement. When visiting we must record // we're inside a capture body. - FunctionLiteralKind::Capture => true, - FunctionLiteralKind::Anonymous { .. } | FunctionLiteralKind::Use { .. } => false, + FunctionLiteralKind::Capture => Some(ExtractVariablePosition::InsideCaptureBody), + FunctionLiteralKind::Anonymous { .. } | FunctionLiteralKind::Use { .. } => { + self.position + } }; ast::visit::visit_typed_expr_fn(self, location, type_, kind, args, body, return_annotation); - self.inside_capture_body = false; + self.position = previous_position; } // We don't want to offer the action if the cursor is over a variable diff --git a/compiler-core/src/language_server/tests/action.rs b/compiler-core/src/language_server/tests/action.rs index 0410ac35270..0dd082402af 100644 --- a/compiler-core/src/language_server/tests/action.rs +++ b/compiler-core/src/language_server/tests/action.rs @@ -3961,6 +3961,30 @@ fn extract_variable_in_block() { ); } +#[test] +fn do_not_extract_top_level_expression_statement() { + assert_no_code_actions!( + EXTRACT_VARIABLE, + r#"pub fn main() { + 1 +} +"#, + find_position_of("1").to_selection() + ); +} + +#[test] +fn do_not_extract_top_level_expression_in_let_statement() { + assert_no_code_actions!( + EXTRACT_VARIABLE, + r#"pub fn main() { + let a = 1 +} +"#, + find_position_of("1").to_selection() + ); +} + #[test] fn expand_function_capture() { assert_code_action!( From 03395ed41768972d6763cba428b536d1d35d6fa0 Mon Sep 17 00:00:00 2001 From: Giacomo Cavalieri Date: Sat, 21 Dec 2024 10:58:32 +0100 Subject: [PATCH 2/2] Fix bug extracting module selects --- .../src/language_server/code_action.rs | 26 +++++++------------ .../src/language_server/tests/action.rs | 15 +++++++++++ 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/compiler-core/src/language_server/code_action.rs b/compiler-core/src/language_server/code_action.rs index 512bfa44f60..9e07b10492a 100644 --- a/compiler-core/src/language_server/code_action.rs +++ b/compiler-core/src/language_server/code_action.rs @@ -2711,8 +2711,16 @@ impl<'ast> ast::visit::Visit<'ast> for ExtractVariable<'ast> { if self.position != Some(ExtractVariablePosition::TopLevelStatement) && within(self.params.range, expr_range) { - self.selected_expression = Some(expr_location); - self.statement_before_selected_expression = self.latest_statement; + match expr { + // We don't extract variables, they're already good. + // And we don't extract module selects by themselves but always + // want to consider those as part of a function call. + TypedExpr::Var { .. } | TypedExpr::ModuleSelect { .. } => (), + _ => { + self.selected_expression = Some(expr_location); + self.statement_before_selected_expression = self.latest_statement; + } + } } let previous_position = self.position; @@ -2744,20 +2752,6 @@ impl<'ast> ast::visit::Visit<'ast> for ExtractVariable<'ast> { self.position = previous_position; } - // We don't want to offer the action if the cursor is over a variable - // already! - fn visit_typed_expr_var( - &mut self, - location: &'ast SrcSpan, - _constructor: &'ast type_::ValueConstructor, - _name: &'ast EcoString, - ) { - let var_range = self.edits.src_span_to_lsp_range(*location); - if within(self.params.range, var_range) { - self.selected_expression = None; - } - } - // We don't want to offer the action if the cursor is over some invalid // piece of code. fn visit_typed_expr_invalid(&mut self, location: &'ast SrcSpan, _type_: &'ast Arc) { diff --git a/compiler-core/src/language_server/tests/action.rs b/compiler-core/src/language_server/tests/action.rs index 0dd082402af..b5fd5f464ac 100644 --- a/compiler-core/src/language_server/tests/action.rs +++ b/compiler-core/src/language_server/tests/action.rs @@ -3985,6 +3985,21 @@ fn do_not_extract_top_level_expression_in_let_statement() { ); } +#[test] +fn do_not_extract_top_level_module_call() { + let src = r#" +import list +pub fn main() { + list.map([1, 2, 3], todo) +}"#; + + assert_no_code_actions!( + EXTRACT_VARIABLE, + TestProject::for_source(src).add_module("list", "pub fn map(l, f) { todo }"), + find_position_of("map").to_selection() + ); +} + #[test] fn expand_function_capture() { assert_code_action!(