Metals: Automatic implementation for abstract methods in concrete classes

[dgalichet]

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

Not related to a problem

Describe the solution you'd like

This could take place automatically or with quick action.

  trait Foo {
    def foo(i: Int): String
  }
// before
  val foo = new Foo {}

// after
  val foo = new Foo {
    def foo(i: Int): String = ???
  }

Describe alternatives you've considered

This could be triggered using completion (same way that for the exhaustive pattern match), or using a quick action.

Additional context

Search terms:

[olafurpg]

Thanks for reporting! Have you tried using "override def" completions?

It would be nice to expose this functionality as a code action, which are currently blocked by scalameta/metals-feature-requests#18

[dgalichet]

yes actually I'm using it completion. But it could be nice to have all abstract methods to be appended when you define a concrete class. An alternative, it could be to have a quick action that append all these methods in case of error because of missing implementations.

[tanishiking]

I tried to implement this feature and found that there's an obstacle for realizing this.

Background

In my plan, metals will provide the automatic implementation on the response of Code Action Request whose CodeActionParams#context.kind is CodeActionKind.QuickFix, and the metals server needs to route the Code Action Request to appropriate code action based on its CodeActionParams.

The best (in robustness) way is routing based on Diagnostic#code, which is a unique identifier of the diagnostics.

• Actually, each diagnostic in TSServer (TypeScript) has a unique identifier, and typescript language server-client tell is there available quick fixes or not, based on the diagnostic identifier.
  
  
  
  • FYI: list of diagnostics and codes: https://github.com/microsoft/TypeScript/blob/master/src/compiler/diagnosticMessages.json
  
  
  • typescript language server-client requests available quick fixes for the diagnostics, based on diagnostic code
  
  
  • (If there are no codeActions available for the diagnostic, vscode will show the message "No quick fixes available")
  
  

Problem

Although Diagnostic in BSP support a diagnostic code, bloop returns null for the code field of Diagnostic because scalac (and even dotty) doesn't have unique identifier for error messages.
As a result, metals have no clue about what the errors are published by the build server, and about what quick fix should metals provide.

Solution

Assign unique identifiers to all errors in scalac / dotty

As discussed in https://contributors.scala-lang.org/t/towards-better-error-messages-in-scalac/1470/31, if the compiler errors have a unique identifier, metals can search the error and available codeActions robustly.
However, it may take quite a long time for implementing this.

Workarounds

Specify the available codeActions based on the error message

Diagnostic in LSP offers some other information other than code, like range, source, and message. Maybe we can decide what is the appropriate codeAction for the diagnostic, based on the error message.

For instance, if a class extends a trait and it lacks some required implementations, scala 2.12.9 publish the following error message,

object creation impossible, since method ${method} in trait ${trait} of type ... is not defined

In this case, we can tell the proper quick fix for this diagnostic is implementing the missing method automatically, because the error message starts with "object creation impossible" and ends with "is not defined".
However, this way depends heavily on the compiler implementation and VERY vulnerable to the changes in error messages.

[tgodzik]

The other solution could be checking if the error is in a class/object and then finding out if there is something unimplemented. It should be possible to go through parent and collect all abstract methods to compare them with the ones inside the newly created class.

Might be easier todo after I finish with goto implementation, since some utility methods might be available.

[olafurpg]

I think it would be a good idea to start by implementing this as a completion. We can additionally support this feature as a code action.

The label of this completion could be something like implement all methods which works similar to match (exchaustive).

A good place to start looking at to implement this feature would be here https://github.com/scalameta/metals/blob/a5050d7c17d011183dad832ba786a7a7edfa9fc0/mtags/src/main/scala/scala/meta/internal/pc/Completions.scala#L1183-L1194

We can create a new CompletionOverrideAllSuite that works similarly as https://github.com/scalameta/metals/blob/a5050d7c17d011183dad832ba786a7a7edfa9fc0/tests/cross/src/test/scala/tests/pc/CompletionOverrideSuite.scala

I'm not sure in what order this completion should appear 🤔 Should it appear at the top of the completion results or should it appear below the indvidual abstract methods?

[olafurpg]

One tricky situation that we should try to handle is when different methods introduce different symbol in scope with the same name. For example

package a { class Foo }
package b { class Foo }
trait Foo {
  def one: a.Foo
  def two: b.Foo
}
class Impl extends Foo {
  // implement all methods: ensure we don't insert conflicting imports
}

We can do this by using the same ShortenedNames instance when rendering the signatures of all methods https://github.com/scalameta/metals/blob/a5050d7c17d011183dad832ba786a7a7edfa9fc0/mtags/src/main/scala/scala/meta/internal/pc/Signatures.scala#L104

After rendering all signatures, the ShortenedNames instance make sure we don't introduce conflicting imports.

[ckipp01]

I'm not sure in what order this completion should appear 🤔 Should it appear at the top of the completion results or should it appear below the individual abstract methods?

My initial thought is at the top?

[olafurpg]

I think the top makes sense, the user can filter out the "implement all methods" completion by writing the name of the specific method to override.

[ckipp01]

Couple quick questions

1) Thoughts on the label?

implement all remaining abstract methods (3 methods)
2) I implemented it so that if you have for example 4 abstract methods, but 2 implemented, you still get the option to implement the remaining ones. Is that alright, or do we only want it offered if non of the abstract members are implemented? If we do want it, if there is only one left to implement, there is no need to show it correct?

[olafurpg]

@ckipp01 that looks great!

Thoughts on the label?

I think we can use something similar as IntelliJ, how about "Implement all methods (3 total)"? The completion list usually has limited horizontal space so it's good to be compact.

implemented it so that if you have for example 4 abstract methods, but 2 implemented, you still get the option to implement the remaining ones. Is that alright, or do we only want it offered if non of the abstract members are implemented?

This sounds great, I would expect it to show this completion as long as there are more than one method left to complete (regardless how many are already implemented).

if there is only one left to implement, there is no need to show it correct?

Correct, this completion item should only appear when there are more than one method to implement.

[ckipp01]

@olafurpg I noticed in intelliJ that when you have the below:

trait Foo {
    def one: String
    def two: String
    val three: String
  }

  val foo = new Foo {
     // hover in here to see message
  }

and you click _Implement methods_ if it gives you the option to implement all _members_, not just the methods. I don't want to assume one way or the other, but do we want to do it the same way?

If so, if the user starts typing def obviously targeting the methods, should the _Implement all methods_ still put in all abstract members or just the abstract methods?

EDIT: Add a var four: String into that party as well. Now I noted that if there was a var four: String the completion will show it as a completion option of def four: String. Would we also want that included in the _Implement all methods_.

[olafurpg]

I don't want to assume one way or the other, but do we want to do it the same way?

Yeah, I think it's OK to implement val/def/var even with the label "Implement all methods". We could use the label "Implement all members" instead if you think that would help, but mirroring the IntelliJ behavior is fine as well.

If so, if the user starts typing def obviously targeting the methods, should the Implement all methods still put in all abstract members or just the abstract methods?

I think it's OK to complete val/var even if the user typed def and selected "implement all methods". I'm not sure how useful it would be to have a completion that only overrides the def members while leaving abstract val/var members unimplemented.

[ckipp01]

Now that #1031 is merged, do we want to close this, or is there still another use case to implement. For example in the initial example, if you were to trigger a completion at the following position

  trait Foo {
    def foo(i: Int): String
  }

  val foo = new Fo@@

I know IntelliJ would provide a complete option to finish creating Foo and also have the members included. I guess this would be like a "create class with members" completion. Is that something we also want to offer?

[olafurpg]

This issue can be closed!

I think the current solution is fine. I'm not sure we should offer to complete all members when writing new Foo@@. As a general rule of thumb, I think completions should not try to guess too many steps in advance (the parameter autofill completion violates this principle). The more steps you try to guess ahead of time the harder it gets to match the intuition of the user. It's better IMO to try and complete only simple steps one at a time.

Down the road, I think it would be nice to provide "implement all members" as a code action when there's a compile error about unimplemented members. However, that can be tracked in a separate feature request.