feat: OverrideCompletion can handle symbolPrefix #4037

tanishiking · 2022-06-16T05:19:01Z

This PR enables OverrideCompletions and implement-all abstract members' code action for Scala3 to import using preset renames like java.util => ju.

abstract class JUtil {
  def foo: java.util.List[Int]
}
class Main extends JUtil {
  def foo@@
}

completes

import java.{util => ju}
abstract class JUtil {
  def foo: java.util.List[Int]
}
class Main extends JUtil {
  def foo: ju.List[Int] = ${0:???}
}

tanishiking · 2022-06-17T07:56:48Z

mtags/src/main/scala-3/scala/meta/internal/pc/AutoImports.scala

+      config.symbolPrefixes.asScala.flatMap { (from, to) =>
+        val fullName = from.stripSuffix("/").replace("/", ".")
+        // val pkg = requiredPackage(fullName)
+        val pkg = SemanticdbSymbols.inverseSemanticdbSymbol(from)


Using inverseSemanticdbSymbol as I found requiredPackage doesn't create proper symbols.

tanishiking · 2022-06-17T07:57:14Z

mtags/src/main/scala-3/scala/meta/internal/pc/AutoImports.scala

    /**
     * @param symbol A missing symbol to auto-import
     */
    def editsForSymbol(symbol: Symbol): Option[AutoImportEdits] =
-      inferAutoImport(symbol).map { ai =>


Just separate the logic into toEdits method.

mtags/src/main/scala-3/scala/meta/internal/pc/AutoImports.scala

tgodzik · 2022-06-20T16:05:02Z

mtags/src/main/scala-3/scala/meta/internal/pc/printer/DotcPrinter.scala

+      tp match
+        case tp: AppliedType =>
+          tp.tycon match
+            case p: PrettyType =>


Overriding it only for PrettyType wouldn't work?

No, if we have AppliedType(PrettyType("Foo"), Seq(Int, String)), DocPrinter will handle AppliedType using RefinedPrinter's toText and it never call this method for PrettyType recursively, we will end up having Int <none> String instead of Foo[Int, String].

dos65

LGTM

mtags/src/main/scala-3/scala/meta/internal/pc/AutoImports.scala

dos65 · 2022-06-21T15:04:32Z

mtags/src/main/scala-3/scala/meta/internal/pc/AutoImports.scala

@@ -33,6 +34,12 @@ object AutoImports:
     */
    case Renamed(sym: Symbol, ownerRename: String)

+    /**
+     * Rename symbol itself, import only.
+     * `Map -> ("", import java.{util => ju})`


Suggested change

* `Map -> ("", import java.{util => ju})`

* `Map -> ("ju.Map", no-import)`

No, SelfRenamed is for import only, and it won't complete anything (like ju.Map)

dos65 · 2022-06-21T15:29:50Z

mtags/src/main/scala-3/scala/meta/internal/pc/AutoImports.scala

+          case AutoImport.SelfRenamed(sym, rename) =>
+            s"${importName(sym.owner)}.{${sym.nameBackticked} => $rename}"


Shouldn't it be unreachable?

I don't think so, SelfRenamed will be caught here https://github.com/tanishiking/metals/blob/9cef97a78e0811aa7ff217a3d5ff1d8bbdd44f79/mtags/src/main/scala-3/scala/meta/internal/pc/AutoImports.scala#L178-L179 and then we will hit here in importEdit

oh. now I see that SelfRenamed is a bit different from what I expected))
It's kind of auto import for import 🤔

Now, we import `java.{util => ju}`

tgodzik

LGTM!

tgodzik · 2022-06-22T07:41:50Z

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

                |import org.eclipse.lsp4j.WorkDoneProgressCreateParams
+                |import java.{util => ju}


Interesting, should we make it work the same for Scala 2 ?

fixed 6889236
I realized Scala2 renames only if the direct owner symbol (of the symbol we're completing) is the java.util.

mtags/src/main/scala-3/scala/meta/internal/pc/AutoImports.scala

Previously in Scala3, if we have a rename config `<java.util> -> "ju"`, Metals always rename `<java.util>` to "ju" For, the following code ```scala import org.eclipse.lsp4j.services.LanguageClient trait Client extends LanguageClient{ over@@ } ``` we get // ... import java.{util => ju} trait Client extends LanguageClient{ override def createProgress(x$0: WorkDoneProgressCreateParams): ju.concurrent.CompletableFuture[Void] = ${0:???} } ``` Now, Metals rename the import only if `java.util` is the direct owner of the symbol we're completing (here, `CompletableFuture`'s direct owner is `java.util.concurrent`, so Metals doesn't rename it). If we complete `java.util.Map`, Metals will complete `ju.Map` and import `import java.{util => ju}`.

tgodzik

Ship it! 🚢

This comment was marked as resolved.

Sign in to view

tanishiking force-pushed the rename-override-completion branch from 880efd3 to 064c3a5 Compare June 17, 2022 06:47

This comment was marked as resolved.

Sign in to view

tanishiking force-pushed the rename-override-completion branch 2 times, most recently from 916a709 to fd31e37 Compare June 17, 2022 07:51

tanishiking marked this pull request as ready for review June 17, 2022 07:55

tanishiking commented Jun 17, 2022

View reviewed changes

tanishiking force-pushed the rename-override-completion branch from fd31e37 to 9cef97a Compare June 20, 2022 09:02

tgodzik reviewed Jun 20, 2022

View reviewed changes

tgodzik mentioned this pull request Jun 20, 2022

bugfix: Fix wrong type shown when type alias is used #4046

Merged

dos65 approved these changes Jun 21, 2022

View reviewed changes

tanishiking added 3 commits June 22, 2022 15:10

feat: OverrideCompletion can handle symbolPrefix

cc813c7

docs: add docstrings on AutoImports

fff3f06

tests: Fix CompletionIssueSuite

1950738

Now, we import `java.{util => ju}`

tanishiking force-pushed the rename-override-completion branch from a884232 to 1950738 Compare June 22, 2022 06:15

tanishiking mentioned this pull request Jun 22, 2022

[Scala3] Better OverrideCompletions #3928

Closed

tgodzik approved these changes Jun 22, 2022

View reviewed changes

dos65 reviewed Jun 22, 2022

View reviewed changes

mtags/src/main/scala-3/scala/meta/internal/pc/AutoImports.scala Outdated Show resolved Hide resolved

tanishiking added 2 commits June 23, 2022 11:44

refactor: Remove unused value definition

152944f

tanishiking requested a review from tgodzik June 23, 2022 02:56

tgodzik approved these changes Jun 23, 2022

View reviewed changes

tanishiking merged commit eab3df8 into scalameta:main Jun 23, 2022

tanishiking deleted the rename-override-completion branch June 23, 2022 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: OverrideCompletion can handle symbolPrefix #4037

feat: OverrideCompletion can handle symbolPrefix #4037

tanishiking commented Jun 16, 2022 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

tanishiking Jun 17, 2022

tanishiking Jun 17, 2022

tgodzik Jun 20, 2022

tanishiking Jun 21, 2022

dos65 left a comment

dos65 Jun 21, 2022

tanishiking Jun 22, 2022

dos65 Jun 21, 2022

tanishiking Jun 22, 2022

dos65 Jun 22, 2022

tgodzik left a comment

tgodzik Jun 22, 2022

tanishiking Jun 23, 2022

tgodzik left a comment

	* `Map -> ("", import java.{util => ju})`
	* `Map -> ("ju.Map", no-import)`

		case AutoImport.SelfRenamed(sym, rename) =>
		s"${importName(sym.owner)}.{${sym.nameBackticked} => $rename}"

		\|import org.eclipse.lsp4j.WorkDoneProgressCreateParams
		\|import java.{util => ju}

feat: OverrideCompletion can handle symbolPrefix #4037

feat: OverrideCompletion can handle symbolPrefix #4037

Conversation

tanishiking commented Jun 16, 2022 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

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

dos65 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

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

tgodzik left a comment

Choose a reason for hiding this comment

tanishiking commented Jun 16, 2022 •

edited

Loading