Added extract constant code action #4228

matiascr · 2025-02-10T21:20:49Z

Addresses #3936

GearsDatapacks · 2025-02-11T21:48:03Z

FYI before this can be merged, you'll need to add tests. Those can be found in the language_server/tests/action.rs file. (It also helps us review changes as the snapshots show the output format of the action!)
Also, you will need to update CHANGELOG.md to include this change.

lpil · 2025-02-14T13:46:34Z

Thank you both!

lpil

Thank you!! Looking very nice.

Could you add tests for these please:

Expressions with function calls
Expressions using local variables
Expressions using records
Expressions use constants
Expression with bool operators
Expressions with logical operators
When the name is taken by a const
When the name is taken by a function

It would also be good to take local variables into account for the naming.

lpil · 2025-02-17T17:22:13Z

compiler-core/src/language_server/code_action.rs

+                })
+                .collect(),
+        };
+        name_generator.reserve_variable_names(module_constant_names);


Functions would also want to be taken into account here. Perhaps this would fit best in the walker? I've not looked at how we do it in the others.

lpil · 2025-02-17T17:30:35Z

compiler-core/src/language_server/code_action.rs

+        if let Some(function_start) = container_function_start {
+            self.edits.insert(
+                function_start,
+                format!("const {variable_name} = {content}\n\n"),
+            );
+
+            self.edits
+                .replace(expression_span, String::from(variable_name));
+        }
+
+        let mut action = Vec::with_capacity(1);
+        CodeActionBuilder::new("Extract constant")
+            .kind(CodeActionKind::REFACTOR_EXTRACT)
+            .changes(self.params.text_document.uri.clone(), self.edits.edits)
+            .preferred(false)
+            .push_to(&mut action);


When is it possible for this to ever be None? It looks like in that case it would add a refactoring that doesn't contain any edits?

This code is now only reachable if the edits are actually going to take place

lpil · 2025-02-17T17:31:48Z

compiler-core/src/language_server/code_action.rs

+            }
+        }
+
+        ast::visit::visit_typed_expr(self, expr);


Don't we want to stop walking the tree if the expression has already been found?

Not quite, it's required to continue in case we want to extract a literal that's inside, say, a function call or record constructor. Maybe it won't be needed after applying the rest of the changes, right now it's necessary.

lpil · 2025-02-17T17:32:13Z

compiler-core/src/language_server/code_action.rs

+        let container_function_start = self
+            .module
+            .ast
+            .definitions
+            .iter()
+            .filter(|definition| definition.is_function())
+            .map(|function| function.location().start)
+            .filter(|function_start| function_start < &expression_span.start)
+            .max();


Could we do this during the tree visiting?

Moved it to visit_typed_function, much cleaner now.

lpil · 2025-02-23T12:16:08Z

If you are ready for a review please un-draft the PR 🙏

lpil

Fab work! Thank you!

I've left some comments inline.

Could you add these tests also:

Extracting results
Extracting Nil
Extracting bit arrays
Extracting a custom type that has fields (a record)
Extracting expressions which reference constant variables
Extracting expressions from assignments that use complex patterns (the assignment can stay and just the value is replaced)
Refusing to extract expressions which reference non-constant variables
Refusing to extract expressions which call functions

And anything else you can think of. I may have missed something 😁

Thank you!

lpil · 2025-02-23T17:13:09Z

compiler-core/src/language_server/code_action.rs

+            constructor.variant,
+            type_::ValueConstructorVariant::Record { arity: 0, .. }
+        ),
+        _ => false,


No catch-all pattern please! They cause bugs as you don't have to update the code when new ones are added.

The nicest way to do this will be to move all the guards into the body of the clause, to avoid repeating each variant twice (once with a guard, once without).

lpil · 2025-02-23T17:13:39Z

compiler-core/src/language_server/code_action.rs

+    Assignment,
+}
+
+fn is_literal_or_made_of_literals(expr: &TypedExpr) -> bool {


A list of literals is a literal also.

Suggested change

fn is_literal_or_made_of_literals(expr: &TypedExpr) -> bool {

fn is_literal(expr: &TypedExpr) -> bool {

Though I think it is really trying to work out is if the expression is valid constant expression syntax, which you can't tell by looking at just the AST. You'd need to know if variables point to constants or not, and if all the calls are to record constructors.

Good point, I'll update it and rename it to can_be_constant for clarity

lpil · 2025-02-23T17:14:09Z

compiler-core/src/language_server/code_action.rs

+            true
+        }
+        // Attempt to extract whole bit array as long as it's made up of literals
+        TypedExpr::BitArray { segments, .. }


You would need to check the options also to check for any variable usage there

compiler-core/src/language_server/code_action.rs

lpil · 2025-02-23T22:35:08Z

compiler-core/src/language_server/code_action.rs

+        if matches!(
+            self.type_of_extractable,
+            None | Some(ExtractableToConstant::Collection)
+        ) && is_literal_or_made_of_literals(expr)


What is this bit doing? Would be useful to have documentation comments explaining the intent

lpil · 2025-02-23T22:36:17Z

compiler-core/src/language_server/code_action.rs

+                TypedExpr::Int { .. } | TypedExpr::Float { .. } | TypedExpr::String { .. } => {
+                    ExtractableToConstant::Value
+                }
+                _ => ExtractableToConstant::Collection,


This mapping of types to ExtractableToConstant isn't quite right. They're all values, and there are things that are not containers being marked as containers here.

lpil · 2025-02-23T22:38:24Z

compiler-core/src/language_server/code_action.rs

+    container_function_start: Option<u32>,
+    type_of_extractable: Option<ExtractableToConstant>,
+    name_to_use: Option<EcoString>,
+    value_to_use: Option<EcoString>,


Would be useful to have these documented as it's a little unclear what they are.

What's the different between "value to use" and "selected expression"?

"type of extractable" seems to not be a type.

Added explanation to the comments, let me know if that clears it up

matiascr · 2025-03-06T18:54:51Z

Support for records is still missing and proving to be a bit tricky

lpil · 2025-03-11T19:48:27Z

Hello! Did you see my comments above?

matiascr · 2025-03-13T19:33:45Z

Hi @lpil ,
What would tests look like for the following?

Extracting expressions which reference constant variables
Extracting expressions from assignments that use complex patterns (the assignment can stay and just the value is replaced)

Could you give an example? Do you mean bracketed expressions or just patterns for pattern matching?

GearsDatapacks · 2025-03-14T08:29:38Z

I think like this is what he meant:

const x = 1
...
let y = [x, 2, 3]

And

let #(a, b, c) = #(1, 2, 3)

matiascr · 2025-03-14T09:01:05Z

Both those cases are already taken into account then. I've updated a test case to cover when the left-hand side is a pattern.
See:

Selecting the patttern (let x = ...) will extract the whole assignment, if possible. Selecting the value will keep the variable assignment and extract only the value.

lpil

Beautiful work! Very impressive! Thank you so much!

…ests and reworked them to cover more cases.

…ray options. Also added: - Exhaustive matches - Documentation

matiascr force-pushed the extract_const_code_action branch from 3ebaaa4 to 3ccbe21 Compare February 10, 2025 21:23

matiascr mentioned this pull request Feb 10, 2025

Extract const code action #3936

Closed

matiascr force-pushed the extract_const_code_action branch 2 times, most recently from bd8e305 to 7f8aef4 Compare February 11, 2025 17:40

matiascr marked this pull request as ready for review February 11, 2025 17:41

matiascr force-pushed the extract_const_code_action branch from d193b39 to 11ba75b Compare February 11, 2025 22:39

matiascr marked this pull request as draft February 12, 2025 20:25

matiascr marked this pull request as ready for review February 12, 2025 22:52

matiascr force-pushed the extract_const_code_action branch from fbdf675 to 5398646 Compare February 14, 2025 13:37

lpil reviewed Feb 17, 2025

View reviewed changes

lpil marked this pull request as draft February 17, 2025 17:40

matiascr force-pushed the extract_const_code_action branch 2 times, most recently from a00559b to 4e8e361 Compare February 20, 2025 09:11

matiascr force-pushed the extract_const_code_action branch from 83753c0 to 10d41e2 Compare February 23, 2025 12:45

matiascr marked this pull request as ready for review February 23, 2025 12:45

lpil reviewed Feb 23, 2025

View reviewed changes

lpil marked this pull request as draft February 23, 2025 22:43

matiascr force-pushed the extract_const_code_action branch 2 times, most recently from 573b4d9 to 4e1217b Compare March 6, 2025 09:32

matiascr marked this pull request as ready for review March 6, 2025 09:32

matiascr force-pushed the extract_const_code_action branch from 0d71d9d to 510aea3 Compare March 9, 2025 11:42

lpil marked this pull request as draft March 11, 2025 19:48

matiascr force-pushed the extract_const_code_action branch from 510aea3 to e9285f2 Compare March 12, 2025 17:03

matiascr force-pushed the extract_const_code_action branch from e26d242 to aa087e9 Compare March 13, 2025 20:55

matiascr marked this pull request as ready for review March 13, 2025 20:55

matiascr force-pushed the extract_const_code_action branch from f22570c to 9e46f2b Compare March 16, 2025 15:35

lpil approved these changes Mar 17, 2025

View reviewed changes

matiascr added 17 commits March 17, 2025 12:57

Added extract constant code action

58d56f5

Removed redundancies

7aacc0d

Added logic to avoid duplicate const names

e97342b

Updated CHANGELOG.md

ece226c

Added tests for extract constant code action

9ec2f6e

Added test for "extract constant" code action on nested lists and tuples

ed49efa

Add extracting declarations. Refactored getting function location.

8108230

Support for records without arguments in "extract constant" code action

e8850ef

Fixed recursion in checking literals. Added more "extract constant" t…

481f724

…ests and reworked them to cover more cases.

Added support for collections with constants. Fixed extracting bit ar…

70cfaac

…ray options. Also added: - Exhaustive matches - Documentation

Fixes from rebase

14d9bb3

Updated with support for concat binary operator

1b91feb

renamed type_of_extractable to variant_of_extractable

a49cf21

Added extract nil test. Added DONT extract function call test.

488b03e

Support for extract constant from records. Added tests

7239b5d

Updated extract constant test for tuples when assigned to a pattern

cd931d9

Added extract constant tests for block, case and use

c38025e

lpil force-pushed the extract_const_code_action branch from 1603e99 to c38025e Compare March 17, 2025 12:58

Update post-rebase

97687f9

lpil merged commit d611687 into gleam-lang:main Mar 18, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added extract constant code action #4228

Added extract constant code action #4228

matiascr commented Feb 10, 2025 •

edited

Loading

GearsDatapacks commented Feb 11, 2025

lpil commented Feb 14, 2025

lpil left a comment •

edited

Loading

lpil Feb 17, 2025 •

edited

Loading

lpil Feb 17, 2025

matiascr Mar 12, 2025

lpil Feb 17, 2025

matiascr Feb 17, 2025

lpil Feb 17, 2025

matiascr Mar 12, 2025

lpil commented Feb 23, 2025

lpil left a comment

lpil Feb 23, 2025

lpil Feb 23, 2025

matiascr Feb 24, 2025

lpil Feb 23, 2025

lpil Feb 23, 2025

lpil Feb 23, 2025

lpil Feb 23, 2025

matiascr Mar 12, 2025

matiascr commented Mar 6, 2025

lpil commented Mar 11, 2025

matiascr commented Mar 13, 2025

GearsDatapacks commented Mar 14, 2025

matiascr commented Mar 14, 2025

lpil left a comment

	fn is_literal_or_made_of_literals(expr: &TypedExpr) -> bool {
	fn is_literal(expr: &TypedExpr) -> bool {

Added extract constant code action #4228

Added extract constant code action #4228

Conversation

matiascr commented Feb 10, 2025 • edited Loading

GearsDatapacks commented Feb 11, 2025

lpil commented Feb 14, 2025

lpil left a comment • edited Loading

Choose a reason for hiding this comment

lpil Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil commented Feb 23, 2025

lpil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matiascr commented Mar 6, 2025

lpil commented Mar 11, 2025

matiascr commented Mar 13, 2025

GearsDatapacks commented Mar 14, 2025

matiascr commented Mar 14, 2025

lpil left a comment

Choose a reason for hiding this comment

matiascr commented Feb 10, 2025 •

edited

Loading

lpil left a comment •

edited

Loading

lpil Feb 17, 2025 •

edited

Loading