Create new symbol action should allow you to create a method

[ckipp01]

Is your feature request related to a problem? Please describe.

Sometimes when I'm writing code I'm in this situation:

def doThing(a: Int, b: Int) = {
  doOtherStuff(a) // this will error because `doOtherStuff` doesn't exist
}

In this situations you can trigger a code action for doOtherStuff and it will show you the "Create new symbol 'doOtherStuff'" action. However, if you select that all the options are creating another file or a companion object, whereas I want doOtherStuff to be a method.

Describe the solution you'd like

I'd like another option to be offered, maybe just "method". And if I select it, the code I'd end up with is:

def doOtherStuff(a: Int) = ???

def doThing(a: Int, b: Int) = {
  doOtherStuff(a)
}

Describe alternatives you've considered

Doing this by hand

Additional context

No response

Search terms

create method

[tgodzik]

Thanks for reporting! I've actually been working on it in https://github.com/tgodzik/metals/tree/infer-method 😅

[ag91]

@tgodzik it seems your PR was at a good point: did you find a major obstacle to complete it?
I was tempted to give a go at recovering it (just not very familiar with metals development, so I may need some guidance).

And a minor question: would it be much more effort to allow the following case as well?

before action:

class X {}

def doThing(a: Int, b: Int, x: X) = {
  x.doOtherStuff(a) // this will error because `doOtherStuff` doesn't exist
}

after action:

class X {
  def doOtherStuff(a: Int) = ???
}

def doThing(a: Int, b: Int, x: X) = {
  x.doOtherStuff(a)
}

[tgodzik]

I think some of the test might have not been passing, but don't remember why I never finished 🤔

One thing that can better to do now is to use the codeAction method instead of adding a new one to the presentation compiler.

Feel, free to pick it up, the way I would go around it is:

• check out the branch
• rebase on the most recent main
• try to run the tests via sbt cross/testOnly tests.pc.InsertInferredMethodSuite

You can add more tests there, they are pretty quick compared to full unit tests and offer quickest way to experiment. You can add println statements via pprint.log(...) and run a single test in watch mode to see what is going on sbt ~cross/testOnly tests.pc.InsertInferredMethodSuite -- name-of-the-test``

[tgodzik]

Let me know if you have any more questions, I will try to answer them as soon as possible. I am also active on our discord.

[ag91]

right, recovered the branch at https://github.com/ag91/metals/commits/infer-method. (BTW is it okay unrelated Hover tests are failing on main - at least on my local version)

It seems the scala 3 presentation compiler method is unimplemented: would using the codeAction method (do you mean this?) help with avoiding the repeated definition for each presentation compiler?

Also where could I get a gist of the architecture relevant to this change? (So I understand better what I am doing)

I had to comment out this test because it was failing (which is a good point to start feature work). Thanks for all the tests!

[tgodzik]

right, recovered the branch at https://github.com/ag91/metals/commits/infer-method. (BTW is it okay unrelated Hover tests are failing on main - at least on my local version)

This might be related to a JDK version, we test on JDK 17

It seems the scala 3 presentation compiler method is unimplemented: would using the codeAction method (do you mean this?) help with avoiding the repeated definition for each presentation compiler?

I don't think I ever managed to work on the Scala 3 support, it can be done separately. That's the codeAction method I mean. It's mostly to avoid having too many methods especially that some of them do exactly the same. And we don't have to later update interfaces in the Scala 3 compiler.

Also where could I get a gist of the architecture relevant to this change? (So I understand better what I am doing)

Code actions are implementations of CodeAction trait and they really have two phases. One is to identify that a code action is really possible there and then resolving it. Resolve is doing all the more intensive stuff and it's the gist of the change:

• use an appropriate method in the Compilers which resolve the exact version of the presentation compiler we need
• then it will use the specific method in the implementation of PresentationCompiler, where we need to put most of the logic and it's already partly done with the class I added.

code action request -> return all code action -> user chooses a code action -> resolve code action -> use the presentation compiler if needed -> return an edit to the editor to be applied

I hope it clears things a bit.

[ag91]

Thanks for your comment @tgodzik !

Right, with this I feel initiated to the wonderful world of code actions XD

A couple of questions:

1. what is a satisfactory scope to get the PR merged in?
   I mean after adding a case, I see that I could manage many cases. I would be happy handling a few initially and then reiterate over time. How does it work usually with these actions?
2. can the context.lookupSymbol search for a class not defined in the same file?
   The use case I would like this action to support is

   class X {}
def doThing(a: Int, b: Int, x: X) = {
  x.doOtherStuff(a) // this will error because `doOtherStuff` doesn't exist
}

where X is in another file. Is it possible to look that up?
3. sbt cross/testOnly tests.pc.InsertInferredMethodSuite keeps running all tests for me! Is there a way to run only a single test?

[tgodzik]

what is a satisfactory scope to get the PR merged in?

It's ok to merge anything with a more narrow scope, just make sure other things do not break. So best to make sure that any unsupported cases are not shown as a code action.

can the context.lookupSymbol search for a class not defined in the same file?

If it's imported I think it should be fine. With anything that is not yet imported you might take a look at how inferred type does it.

3. sbt cross/testOnly tests.pc.InsertInferredMethodSuite keeps running all tests for me! Is there a way to run only a single test?

You can add -- *name-of-the-test* after it. That's the glob syntax I think where * matches 0+ characters

[ag91]

@tgodzik how do you find the position of the empty body of a class like

class X () {^}

The best I could do is the +4 here ag91/metals@c8f53ab#diff-e34c24f618da246200888c78de43be3e8d92e005c47f86fa2dcfe7d390d02457R278

I could find the right spot if it has at least a value like

class X () {^ val x = 1}

but I couldn't find the right way without the body.
Now got to type the parameters, handle a few more cases and probably some wrap up work: it will take some time!

[tgodzik]

inferEditPosition from OverrideCompletions seems to be the same logic that you need.

[ag91]

@tgodzik I felt somewhat satisfied with the cases handled by unit tests in the action.
So I tried to build the snapshot to test locally, but apparently things don't work:
the action gets picked up only for List(1).map(nonExistent) (I guess because of this predicate?)
and even then metals.log displays:

2024.10.20 17:04:15 ERROR Expected 'Object with `document` and `position`

Example:

{
  document: "file:///home/dev/foo/Bar.scala",
  position: {line: 5, character: 12}
}

', but got '[{"textDocument":{"uri":"file:///home/andrea/workspace/test/src/main/scala/ag91/test/HelloWorld.scala"},"position":{"line":7,"character":16}}]'�[0m
2024.10.20 17:05:18 ERROR Expected 'Object with `document` and `position`

Any chance this rings a bell?

[tgodzik]

We might need to have:

override type CommandData = l.TextDocumentPositionParams
def command: Option[ActionCommand] = Some(ServerCommands.InsertInferredMethod)

which defines what command might be sent to the code action provider.

The message itself is probably wrong, since textDocument should be the name.

[ag91]

mmm, do you mean here?
We are reusing CreateNewSymbol, so I think that .setCommand should do for the action, not sure about the override though (since that would change things also for the other actions of CreateNewSymbol).

Or are you suggesting we need a separate action?

[tgodzik]

For clarity we can always create a separate action, but in this case I think it's enough to have CreateNewSymbol action implement those two methods a mentioned. The other action uses an existing command, so both should not interfere