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?

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?

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

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

…ests and reworked them to cover more cases.

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.

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

lpil · 2025-02-23T17:19:47Z

compiler-core/src/language_server/code_action.rs

+    value_to_use: Option<EcoString>,
+}
+
+enum ExtractableToConstant {


Could you document what this is and how it is used please, including each variant.

lpil · 2025-02-23T17:20:27Z

compiler-core/src/language_server/code_action.rs

+                self.edits.delete(expr_span_with_new_line);
+            }
+
+            ExtractableToConstant::Collection | ExtractableToConstant::Value => {


Are these two exactly the same then?

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.

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

Open

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 added 8 commits February 23, 2025 13:44

Added extract constant code action

feec5f3

Removed redundancies

6880d8c

Added logic to avoid duplicate const names

2d4632c

Updated CHANGELOG.md

1f613da

Added tests for extract constant code action

d258858

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

6bb323e

Add extracting declarations. Refactored getting function location.

91a5cfa

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

10d41e2

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

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

14051ab

…ests and reworked them to cover more cases.

lpil reviewed Feb 23, 2025

View reviewed changes

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

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

lpil Feb 17, 2025

matiascr Feb 17, 2025

lpil Feb 17, 2025

lpil commented Feb 23, 2025

lpil left a comment

lpil Feb 23, 2025

lpil Feb 23, 2025

lpil Feb 23, 2025

lpil Feb 23, 2025

lpil Feb 23, 2025

lpil Feb 23, 2025

lpil Feb 23, 2025

lpil Feb 23, 2025

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

Added extract constant code action #4228

Are you sure you want to change the base?

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

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 Feb 10, 2025 •

edited

Loading

lpil left a comment •

edited

Loading

lpil Feb 17, 2025 •

edited

Loading