Code actions to add and remove anonymous functions #4789

treuherz · 2025-07-17T10:50:47Z

I've had a go at implementing new code actions discussed in #4614, one to wrap a reference to a function in an anonymous function, and one to unwrap a trivial anonymous function into a bare reference.

These seemed pretty straightforward to implement so I'm a bit concerned I've missed a whole slew of edge cases. Any pointers as to more test cases I should add would be great

I know the issue was inconclusive about the names. These ones seemed straightforward but bikeshedding is welcome

Closes #4614

compiler-core/src/language_server/code_action.rs

lpil

Thank you! Looking really good! I've left a bunch of comments inline, let me know if anything is unclear.

Please un-draft the PR and tag me when you are ready for a review. Thank you

lpil · 2025-07-17T12:07:15Z

compiler-core/src/language_server/code_action.rs

        variables
    }
+
+    fn from_expr(expr: &TypedExpr) -> Self {


No abbreviations please 🙏 expr -> expression

It's not very clear to me from the name what this does. A doc comment would help a lot, and maybe a more descriptive name

lpil · 2025-07-17T12:07:27Z

compiler-core/src/language_server/code_action.rs

    }
 }
+
+struct FunctionToWrap {


Document this please

compiler-core/src/language_server/code_action.rs

lpil · 2025-07-17T12:14:09Z

compiler-core/src/language_server/code_action.rs

+    }
+}
+
+impl<'ast> ast::visit::Visit<'ast> for WrapInAnonymousFunction<'ast> {


I think this is computing the code action for all non-nested function references that are assigned to variables or are passed as arguments, but we want only the ones that are currently within the range specified by the code action parameters.

I think it would ideally be any non-called function rather than specifically these two positions in the AST, and think the non-nested restriction should be removed.

the range check makes sense, I'd missed it before bc I was looking at RemoveUnusedImports which obviously doesn't need it 🤦

Lots of the other actions store their TextEdits in the struct but only need it there for the range check, which can be done with just a LineNumbers. Storing LineNumbers seems more natural to me but I'm splitting hairs and will defer to consistency if you like

The non-nested restriction was inadvertent, if I'm reading it right I need to call back into the visitor after I'm done?

Using line numbers seems good to me!

compiler-core/src/language_server/code_action.rs

lpil · 2025-07-17T12:19:29Z

compiler-core/src/language_server/code_action.rs

+
+            if call_arguments != expected_arguments {
+                return;
+            }


Document the logic in this section please

lpil · 2025-07-17T12:20:10Z

compiler-core/src/language_server/code_action.rs

+            }
+        }
+
+        // match fn bodies with only a single function call


Capital letters at the start of comments please, and explain why something is done rather than what is done. Thank you

lpil · 2025-07-17T12:20:53Z

compiler-core/src/language_server/tests/action.rs

 const GENERATE_VARIANT: &str = "Generate variant";
 const REMOVE_BLOCK: &str = "Remove block";
+const WRAP_IN_ANONYMOUS_FUNCTION: &str = "Wrap in anonymous function";
+const UNWRAP_ANONYMOUS_FUNCTION: &str = "Unwrap anonymous function";


To me this sounds like the anonymous function remains but its wrapper will be removed. "Remove anonymous function wrapper" perhaps?

lpil · 2025-07-17T12:21:24Z

compiler-core/src/language_server/tests/action.rs

+        "import gleam/list
+
+pub fn main() {
+    list.map([1, 2, 3], op)


2 space indentation for Gleam please 🙏

lpil · 2025-07-17T12:23:27Z

compiler-core/src/language_server/tests/action.rs

+",
+        find_position_of("fn(int)").to_selection()
+    );
+}


Let's add more tests to cover all the possible scenarios. We'll want to cover:

Labelled arguments

Pipes

Qualified functions from other modules

Records

Comments

And anything else you can think of!

treuherz · 2025-07-18T09:43:49Z

Ok, there was more to do here than I thought but the code is better than before 🎉

I've copped out and not offered the action on functions with comments in. I think it's doable, but found an issue I wanted some advice on while I was on my way there. The first_comment_between helper has a binary search which will return any comment in a given line-span, not necessarily the first. We could guarantee it finds the first by recursing back into the list with smaller boundaries, but I haven't written recursive algorithms in Rust before and I don't know how to tell if I'm going to blow up the stack.

llakala · 2025-07-18T15:58:38Z

Probably also want a changelog entry, right?

Thanks for doing this!

treuherz · 2025-07-19T12:05:37Z

Ok, I've fixed first_comment_between without recursion but actually unwrapping functions with comments is a bit of a minefield because unless I re-indent them afterwards it looks rubbish. How would you feel about keeping this as-is (i.e. not offering the action on functions with comments) and improving that in a follow-up?

lpil · 2025-07-21T11:03:10Z

Comments shouldn't change the behaviour of the language server, so we would want to provide code actions regardless of whether or not there are comments.

Having imperfect formatting afterwards when there's comments doesn't sound too bad, the programmer can run the formatter. Do you have some examples of what the output looks like?

I've less context than you, but it seems like comments inside the body of the function shouldn't be much of an issue as they can be left as-is:

fn(x, y) {
  // comment
  thingy(x, y)
}

  // comment
  thingy

For comments inside the parameters I think we can discard them happily.

fn(
  // Hello
  x,
  y,
) {
  thingy(x, y)
}

  thingy

treuherz · 2025-07-21T17:37:11Z

It's not really anything that the autoformatter wouldn't tidy up. There are some snapshots in this commit, the worst it gets is probably things like this one where the comment has moved lines but that's not too big a deal. If that's acceptable (or close enough to work on) I'll include it in this PR

lpil

Awesome, really nice work!! I've left a few small notes inline.

When you are ready for a review please un-draft this PR and tag me for a review. Thank you!

lpil · 2025-08-07T11:56:52Z

compiler-core/src/parse/extra.rs

+            best = self.comments.get(index).copied();
+            search_list = search_list.get(0..index).unwrap_or(&[]);
+        }
+        best


Why are we now traversing the comments and copying their locations multiple times?

I've moved the copy out of the loop but I think the multiple-traversal is necessary to ensure we return the first comment in the range rather than just a comment in the range. I added some failing tests for the previous implementation in this commit, I can comment this new one more to explain the method properly

lpil · 2025-08-07T11:59:57Z

compiler-core/src/language_server/code_action.rs

+/// Helper struct, a target for [WrapInAnonymousFunction].
+struct FunctionToWrap {
+    location: SrcSpan,
+    arguments: Vec<Arc<Type>>,


This could be borrowed I think

Suggested change

arguments: Vec<Arc<Type>>,

arguments: &'a [Arc<Type>],

I can't figure out how to get that past borrowck 😕. The arguments are pulled from a field of an Arc<Type> here. If we want to avoid cloning the argument list then I could clone the whole Arc<Fn> for the function instead and pull the arguments out later

lpil · 2025-08-07T12:09:37Z

...eam_core__language_server__tests__action__wrap_anonymous_function_in_anonymous_function.snap

+---
+----- BEFORE ACTION
+pub fn main() {
+  let f = fn(in) { ception(in) }


I don't think we should be offering to wrap an anonymous function in another anonymous function

lpil · 2025-08-07T12:11:48Z

...ts/gleam_core__language_server__tests__action__wrap_pipeline_step_in_anonymous_function.snap

+---
+----- BEFORE ACTION
+pub fn main() {
+  1 |> wibble |> wobble


Could we have a test for wibble taking 2 arguments please

lpil · 2025-08-07T12:14:42Z

compiler-core/src/language_server/code_action.rs

+
+            // We need to delete the anonymous function's head but preserve
+            // comments between it and the inner function call.
+            edits.delete(self.span_until_comment(SrcSpan {


Rather than seek through the comments multiple times I think we could use the location stored in the FunctionLiteralKind::Anonymous variant.

I think by pulling more data through from the parser we can eliminate both comment-checks, but I'm not sure we can currently eliminate either.

Here's what I think we get from the parser currently:

fn(arg_1: T1, arg_2: T2) -> T3 { [comments] inner_function(arg_1, arg_2) [comments] } └─────────────head───────────┘ └────inner───┘ └─────────────────────────────────────────outer──────────────────────────────────────┘

So means if I pull the opening brace through the parser I can eliminate this first comment-seek, and if I pull the span of the inner argument-list through I can eliminate the second. Without either I think we risk mangling things if the input isn't well-formatted (e.g. too much space before the opening brace)

I've pulled more locations through the parser, and have followed this approach so we now don't care about comments. I've left the bug-fix in though!

lpil · 2025-08-07T12:15:57Z

compiler-core/src/language_server/code_action.rs

+
+            // Now we need to delete the inner function call's arguments,
+            // preserving comments before the outer function tail.
+            edits.delete(self.span_until_comment(SrcSpan {


In the TypedExpr::Call variant we could store the byte index of the ( to make it easier to get that position.

see previous

lpil · 2025-08-24T08:25:41Z

Do remember to undraft this PR and tag me when you're ready for a review!

plus a bunch more tests

previously this could have returned any comment in the span rather than guaranteeing the first

The indentation of comments when we're done with them might be a bit off, but it's nothing an autoformat shouldn't be able to fix

This increases the blast radius of the change a bit, but means we don't need to worry about comments any more.

treuherz · 2025-09-09T13:45:29Z

Sorry for the delay, I wanted some advice on which route to take to fix the problems, and then life got in the way. I think the new approach is better, and I've rebased on top of main. Ready for review @lpil

treuherz commented Jul 17, 2025

View reviewed changes

compiler-core/src/language_server/code_action.rs Outdated Show resolved Hide resolved

lpil reviewed Jul 17, 2025

View reviewed changes

lpil marked this pull request as draft July 17, 2025 12:25

treuherz marked this pull request as ready for review July 18, 2025 09:44

treuherz requested a review from lpil July 19, 2025 12:05

lpil reviewed Aug 7, 2025

View reviewed changes

lpil marked this pull request as draft August 7, 2025 12:18

treuherz added 17 commits September 9, 2025 14:29

add action to wrap a function ref in its own anonymous fn

50cbf19

add action to unwrap a trivial anonymous function

0d3af2d

tidy imports

c20b981

remove chained if-let

b9d6754

better naming, more docs and comments

1bbf8ed

add range checks for new code actions

aed14a6

correctly indent gleam in tests

e6ee64c

make unwrapping work within nested function literals

cb1bb1c

anonymous functions can't have labelled arguments!

8b43e4d

make function-wrapping action wrap any uncalled function

f3eb218

plus a bunch more tests

add tests for unwrapping functions with labelled arguments

5c6aa88

don't try to unwrap functions with comments in

5e926a4

add test for wrapping record constructor

6e01d2c

use iterators for comparing arguments instead of vecs

3ec71f7

add failing tests for first_comment_between

9c69083

fix first_comment_between

9a951b0

previously this could have returned any comment in the span rather than guaranteeing the first

unwrap anonymous functions with comments in

c68ea9b

The indentation of comments when we're done with them might be a bit off, but it's nothing an autoformat shouldn't be able to fix

treuherz added 4 commits September 9, 2025 14:30

add changelog entry for new code actions

631d821

don't offer to re-wrap anonymous functions

8884ab1

add tests for wrapping multi-argument pipeline steps

a4fd477

don't copy comment location in a loop

6809596

treuherz force-pushed the anon-fn-code-actions branch from a0c331f to 203a523 Compare September 9, 2025 13:40

treuherz marked this pull request as ready for review September 9, 2025 13:41

get fn locations from parser

58626bf

This increases the blast radius of the change a bit, but means we don't need to worry about comments any more.

treuherz force-pushed the anon-fn-code-actions branch from 203a523 to 58626bf Compare September 9, 2025 13:42

Uh oh!

Code actions to add and remove anonymous functions #4789

Are you sure you want to change the base?

Code actions to add and remove anonymous functions #4789

Uh oh!

Conversation

treuherz commented Jul 17, 2025

Uh oh!

Uh oh!

lpil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

treuherz commented Jul 18, 2025

Uh oh!

llakala commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

treuherz commented Jul 19, 2025

Uh oh!

lpil commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

treuherz commented Jul 21, 2025

Uh oh!

lpil left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpil commented Aug 24, 2025

Uh oh!

llakala commented Jul 18, 2025 •

edited

Loading

lpil commented Jul 21, 2025 •

edited

Loading

lpil left a comment •

edited

Loading