Infer method #6877

ag91 · 2024-10-22T23:15:17Z

This attempts to increase the capabilities of the "Create New Symbol" action to allow new method definition.

Supported only Scala 2 for cases shown in tests: tests/cross/src/test/scala/tests/pc/InsertInferredMethodSuite.scala

Related discussion and feature request: scalameta/metals-feature-requests#298

metals/src/main/scala/scala/meta/internal/metals/codeactions/CreateNewSymbol.scala

tgodzik · 2024-10-23T09:27:28Z

mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java

+	/**
+	 * Return the missing method
+	 */
+	public abstract CompletableFuture<List<TextEdit>> insertInferredMethod(OffsetParams params);


Could we reuse the codeAction interface, it should be universal for all actions

is there an example by any chance? Otherwise I will pick this up at the end, after making sure the cases I handled in the tests work running interactively (not all do yet!)

I was supposed to quickly create an example, but then figured we can refactor the old methods in #6881

It's more complicated for the older action, but for anything new(as soon as my PR is merged) it should be as easy as:

Add a new CodeActionId string

Implement maybeCodeActionId from CodeAction trait

Add handling of new id to codeAction method

Add id to list of supported ones

Use codeAction method from compilers in handleCommand

Feel free to work on the meat of the issue in the compiler, we can get back to it later or I can fix that myself.

right, I seem to have achieved what I wanted! Things are working now 🌞

metals/src/main/scala/scala/meta/internal/metals/codeactions/CreateNewSymbol.scala

ag91 · 2024-10-26T14:12:47Z

@tgodzik I have managed to handle the issues I experienced by running my feature interactively.
I am now tuning the action to allow edits in other documents via WorkspaceEdit (without inferring a method for classes wouldn't be that much useful... ).

Meanwhile, I have a question:
wouldn't make sense to have a logging utility to let user report bugs with the action I am adding?
By that I mean this is what you get when something goes wrong with an action:
SEVERE: Internal error: scala.concurrent.Future$$anon$2: Future.filter predicate is not satisfied java.util.concurrent.CompletionException: scala.concurrent.Future$$anon$2: Future.filter predicate is not satisfied Caused by: scala.concurrent.Future$$anon$2: Future.filter predicate is not satisfied

but for debugging the issue I need the pprint.log of the tree and the position and possibly the original code.
How would a user report an issue like this? Should I look into printing this information out?

In my experience I have had a bunch of issues with inferType and now I see why the metals.log didn't have any hints about what caused them (not necessarily the state of my project, but possibly because not all cases are handled xD)

ag91 · 2024-10-26T23:56:01Z

@tgodzik also:
managed to convert to a WorkspaceEdit. In my testing adding a method to an imported object that exists in another file works ONLY IF the file is already open (I am testing with Emacs).
Not sure if that is a known limitation or there is a trick I have to use to tell the LSP client to open the file and then apply the edits?

And could I get a hint on how to pass the MiMa action? I can fix the scalafix one. What else is needed to merge this in?

metals/src/main/scala/scala/meta/internal/metals/Compilers.scala

tgodzik · 2024-10-28T15:19:15Z

wouldn't make sense to have a logging utility to let user report bugs with the action I am adding?

For sure it's always better to add reports, we do have a utility for that with ReportContext and we can add any data there. Also, we should make sure if is not used in for comprehensions with futures as it is not too clear when the filter fails. Do you know where that happened?

managed to convert to a WorkspaceEdit. In my testing adding a method to an imported object that exists in another file works ONLY IF the file is already open (I am testing with Emacs).
Not sure if that is a known limitation or there is a trick I have to use to tell the LSP client to open the file and then apply the edits?

I would think workspace edit opens a file, but maybe it's a problem with uri? I can check

tgodzik · 2024-10-28T15:41:45Z

And could I get a hint on how to pass the MiMa action? I can fix the scalafix one. What else is needed to merge this in?

We should be ok once we use the codeAction interface. I can update it all before merging

tgodzik · 2024-10-28T15:44:32Z

I can't seem to make it work at all with .method(..) syntax, I can dig in later, but we could implement that later.

ag91 · 2024-10-29T01:17:17Z

I can't seem to make it work at all with .method(..) syntax, I can dig in later, but we could implement that later.

you were right, there was an issue with me not handling NullaryMethodType (not sure why that didn't show in the tests): solved in cc5b6e6

ag91 · 2024-10-29T01:19:14Z

I would think workspace edit opens a file, but maybe it's a problem with uri? I can check

Found out that this issue happens only when I restart metals. Editing a source without an open buffer works fine on a fresh session. Thanks!

ag91 · 2024-10-29T01:20:50Z

And could I get a hint on how to pass the MiMa action? I can fix the scalafix one. What else is needed to merge this in?

We should be ok once we use the codeAction interface. I can update it all before merging

Okay, I am removing the draft from this PR then.

tgodzik · 2024-10-29T11:29:31Z

Is it ok if I push the changes related to the new codeAction inteface? That's now merged to main

ag91 · 2024-10-29T15:36:43Z

Is it ok if I push the changes related to the new codeAction inteface? That's now merged to main

sure, please go ahead: I am planning to only change the codebase for review feedback for now

tgodzik

We should tighten the scope of this PR I think, code action like this should really only work within a single file as otherwise it will become much more complex.

tgodzik · 2024-10-30T12:57:21Z

mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java

@@ -158,211 +159,215 @@ public abstract CompletableFuture<List<AutoImportsResult>> autoImports(String na
 	public abstract CompletableFuture<List<TextEdit>> insertInferredType(OffsetParams params);

 	/**
-	 * Return the text edits for inlining a value.


Any chance not to reformat the file? It makes it difficult to see what actually changed

oh, I think metals github action runs automatically before push (I had to apply to be able to push). Maybe you can try difftastic or ignore whitespaces in PR

It's fine, but I think the formatting was done by the editor actually, but maybe we never run any formatting on it before 🤔 Would probably be cleaner to have that as a separate commit because otherwise any blame in the file will point at you change 😅

tgodzik · 2024-10-30T12:59:57Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+                Nil
+            }
+          // nonExistent(param1, param2)
+          // val a: Int = nonExistent(param1, param2) TODO


Is it still TODO?

tgodzik · 2024-10-30T13:00:18Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+    }
+
+    val uriTextEditPairs = typedTree match {
+      // case errorMethod if errorMethod.toString().contains("lol") =>


Might be worth removing any comments that no longer make sense

sorry, I tired myself out while reiterating over this change.
I will take a little time to address your feedback: thanks for that!

tgodzik · 2024-10-30T13:01:01Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+                Ident(containingMethod),
+                arguments
+              ) :: _ =>
+            val methodSymbol = context.lookupSymbol(containingMethod, _ => true)


Would be able to extract some methods? Might be easier to review

tgodzik · 2024-10-30T13:02:36Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+                        paramsString = argumentsString(arguments).getOrElse(""),
+                        retType = None,
+                        postProcess = method => {
+                          if (


Looks like this could be extracted, isn't it the same as below and above?

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

tgodzik

Great work! I added a few comments.

There also seems to be an unhandled case:

ObjectName.otherMethodWithoutParens we do show the code action but fail to do anything. We should either not show the method (we can use trees for that and not show if we are in a select statement without and apply parent) or handle it in the compiler

metals/src/main/scala/scala/meta/internal/metals/ServerCommands.scala

metals/src/main/scala/scala/meta/internal/metals/codeactions/CreateNewSymbol.scala

tgodzik · 2024-11-05T14:29:09Z

mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java

+	/**
+	 * Return the missing method
+	 */
+	public abstract CompletableFuture<WorkspaceEdit> insertInferredMethod(OffsetParams params);


This should have default implementation for Mima to pass, but I will change it later on to use codeAction, so don't worry about it

yes please, I haven't dived in the codeAction method sure that it would be easier for you to adapt.

tests/cross/src/test/scala/tests/pc/InsertInferredMethodSuite.scala

tgodzik · 2024-11-05T14:32:16Z

tests/cross/src/test/scala/tests/pc/InsertInferredMethodSuite.scala

+
+  // doesn't work currently, User(1) is not being typed
+  checkEdit(
+    "custom-type-advanced".ignore,


TODO -> reminder to create an issue when we merge

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

tgodzik · 2024-11-05T15:20:20Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+             .asInstanceOf[NullaryMethodType]
+             .resultType
+         else containerSymbol.symbol.tpe) match {
+          case TypeRef(_, classSymbol, _) =>


Should we also make sure it's the same file here?

not necessary because the check is in makeClassMethodEdits which I made sure to call for each case that infers a method.

tgodzik · 2024-11-05T15:22:02Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+            // this gives me the position of the class
+            typedTreeAt(containerClass.symbol.pos) match {
+              // class case
+              case ClassDef(


Could we use ImplDef superclass for both ClassDef and ModuleDef?

it seems a good idea but a simple ClassDef -> ImplDef didn't compile.
I will look into the other issues you have highlighted since this feels minor and requires digging time.

tgodzik · 2024-11-05T15:27:02Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+   */
+  private def inferEditPosition(t: Template): Position = {
+    // get text via reflection because Template could be in a different file
+    val text = t.pos.source.content.map(_.toString).mkString


Since we only support single file we can just use text from the OffsetParams

tgodzik

I tested things a bit more and looks a couple of cases are not tested and do not work. I think overall we should reduce the cases handled to 4:

otherMethod(errorMethod(param))
errorMethod(param)
ident.errorMethod(param)
method(errorMethod) with no select

anything else seems very hard to handle and we can make this PR more focused. We can also make sure that this code action only shows up for these cases.

I hope you are not too discouraged. This is a pretty hard one, which is why I probably abandoned this after the initial tries.

Let me know if you need help, I can help rework this PR a bit

tgodzik · 2024-11-08T13:33:45Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+                arguments
+              ) :: _ =>
+            makeEditsForListApply(arguments, containingMethod, errorMethod)
+          // List(1,2,3).map(myFn)


This and the one below does not work unfortunately, it's not really possible to get a type of List(1,2,3).map the current implementation will just produce:

def otherMethod(arg0: Int, arg1: Int, arg2: Int) = ???

which is not valid. We can't even know how many parameters it will have. And also this might be a much more complex expression.

tgodzik · 2024-11-08T13:34:27Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+              position = None
+            )
+          // List(1, 2, 3).map(nonExistent)
+          // List((1, 2)).map{case (x,y) => otherMethod(x,y)}


Is this actually the snippet this handles? Looks like this is a simple apply containingMethod(arguments) The same as the one above

I don't see any check for being inside another method.

yes, this is handling both List cases: both the non argument case, and the unpacking case

ag91 · 2024-11-08T17:47:28Z

I hope you are not too discouraged. This is a pretty hard one, which is why I probably abandoned this after the initial tries.

It is alright: it is a mix of I had more time before because I was on holiday and lost a bit of enthusiasm because the action will not be as useful as I needed it to be.

I am happy to wrap the PR up with your feedback though: I will reduce the cases and polish, possibly during the weekend.
I will get this in so Metals users can benefit ☀️

ag91 · 2024-11-10T01:23:13Z

@tgodzik it turned out that handling the Object.definition was just adding a couple of cases
and the List(1,2,3).map(myFn) also seemed easy to patch up.
Hope we are closer at this point

some tests that I believe are unrelated are failing. Shall look into fixing those once I push to my fork.

It now handles adding methods to class and object, lambda-0 case, map(nonExistent) with and without arguments fix: make action handleCommand with infer method also select action on the ScalacDiagnostic.NotAMember fix issues found by running interactively with scala 2.13.15 move from TextEdit to WorkspaceEdit to allow edit in other files than current when adding a file to a class, we may have the class imported but defined in another file. TextEdit only works for current edits, while we need to add the method stub in the other file. WorkspaceEdit seems to be the way to go here. satisfy scalafix fix issues with action for class methods and extra curly brackets remove unnecessary java formatting

this is to restrict the scope of this action, since it is unclear what would happen for defining method for libraries we are using

Co-authored-by: Tomasz Godzik <[email protected]>

tgodzik · 2024-11-14T18:38:19Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+      logger.warning(
+        s"This case ($c) is not currently supported by the infer-method code action."
+      )
+      throw new RuntimeException()


We can use DisplayableException similar to inlineValue does it.

tgodzik · 2024-11-14T18:43:17Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+            signature(
+              name = errorMethod.name.toString(),
+              paramsString =
+                Option(argumentsString(arguments.take(1)).getOrElse("")),


What if we have a method like:

object DoubleList{ def apply(a : Int) = List( (a,b) ) } class DoubList{ def map(f: (Int, Int) => String) = ??? }

DoubleList(1).map(nonExistent) would create a wrong method. We can't know what is the type used in a method based on the arguments of the preceding apply.

A very hacky way around it would be to create a function that take in everything like
def myErrorFunction(a: Any) : Nothing and with that rerun the presentation compiler to get the actual type of map. We do a similar thing with updating inferred type : https://github.com/scalameta/metals/blob/main/mtags/src/main/scala-2/scala/meta/internal/pc/InferredTypeProvider.scala#L89C9-L89C26

But maybe we could do it later, for now I would remove these cases and maybe document everything unsupported in a separate issue?

ignored the cases, thanks

tgodzik · 2024-11-14T18:43:53Z

mtags/src/main/scala-2/scala/meta/internal/pc/InferredMethodProvider.scala

+            unimplemented("other-than-method")
+        }
+      case _ =>
+        unimplemented("other-than-ident-or-select")


We could add better explanations here and use the DisplayableError which would then show up for the users.

this code is just a reference to find it back in code, I will make the logger into the DisplayableError.
I am unsure what would be a better explanation than saying "the case x is not currently supported for the action".
There could be many cases unsupported for these bits (it would take time to list them and not sure how that would help a user who is likely just disappointed things don't work here)

I think it's enough to make it into a sentence, so that it's clear what is not supported.

okay I applied the changes.

ag91 · 2024-11-15T23:58:56Z

@tgodzik I think I have addressed all the comments. I think the last step is for you to address the MiMa issue and merge it when it is ready, no?
What a journey ☀️

tgodzik · 2024-11-16T19:35:23Z

@tgodzik I think I have addressed all the comments. I think the last step is for you to address the MiMa issue and merge it when it is ready, no? What a journey ☀️

I think we are almost ready, I will add some final touches next week most likely related to the code actions interface. Thanks for your hard work!

ag91 force-pushed the infer-method branch from c768220 to 7a115ad Compare October 22, 2024 23:18

tgodzik reviewed Oct 23, 2024

View reviewed changes

ag91 force-pushed the infer-method branch from 12ec7f8 to adbcadf Compare October 26, 2024 13:49

tgodzik reviewed Oct 28, 2024

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/Compilers.scala Show resolved Hide resolved

ag91 marked this pull request as ready for review October 29, 2024 01:20

ag91 force-pushed the infer-method branch from cc5b6e6 to 8634a7c Compare October 29, 2024 01:24

tgodzik requested changes Oct 30, 2024

View reviewed changes

ag91 force-pushed the infer-method branch from 8634a7c to e9075e3 Compare November 2, 2024 13:37

ag91 requested a review from tgodzik November 2, 2024 13:56

tgodzik requested changes Nov 5, 2024

View reviewed changes

tgodzik requested changes Nov 8, 2024

View reviewed changes

ag91 requested a review from tgodzik November 10, 2024 01:23

tgodzik and others added 6 commits November 10, 2024 01:23

Add a possibility to infer new methods

850bda0

update PR to latest metals changes

f942bd5

some tests that I believe are unrelated are failing. Shall look into fixing those once I push to my fork.

clean comments

f0afe01

extract methods

5a2710f

disallow generation of method in file different from current one

4e786f3

this is to restrict the scope of this action, since it is unclear what would happen for defining method for libraries we are using

ag91 and others added 3 commits November 10, 2024 01:23

Apply suggestions from code review

32c4d0f

Co-authored-by: Tomasz Godzik <[email protected]>

add test and cleanup

674c6a2

handle object definition case

999984c

ag91 force-pushed the infer-method branch from 2d29e70 to 999984c Compare November 10, 2024 01:25

tgodzik reviewed Nov 14, 2024

View reviewed changes

use DisplayableException and skip lambda generic case

db3bbef

ag91 force-pushed the infer-method branch from 01802ae to db3bbef Compare November 14, 2024 23:57

ag91 requested a review from tgodzik November 14, 2024 23:58

rename unimplemented cases

467aed2

Infer method #6877

Are you sure you want to change the base?

Infer method #6877

Conversation

ag91 commented Oct 22, 2024 • 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

ag91 commented Oct 26, 2024

ag91 commented Oct 26, 2024

tgodzik commented Oct 28, 2024

tgodzik commented Oct 28, 2024

tgodzik commented Oct 28, 2024

ag91 commented Oct 29, 2024

ag91 commented Oct 29, 2024 • edited Loading

ag91 commented Oct 29, 2024

tgodzik commented Oct 29, 2024

ag91 commented Oct 29, 2024 • edited Loading

tgodzik 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

tgodzik 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

tgodzik 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

ag91 commented Nov 8, 2024 • edited Loading

ag91 commented Nov 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik Nov 14, 2024 • 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

ag91 commented Nov 15, 2024

tgodzik commented Nov 16, 2024

ag91 commented Oct 22, 2024 •

edited

Loading

ag91 commented Oct 29, 2024 •

edited

Loading

ag91 commented Oct 29, 2024 •

edited

Loading

ag91 commented Nov 8, 2024 •

edited

Loading

tgodzik Nov 14, 2024 •

edited

Loading