feat: Import missing extension method #4141

tanishiking · 2022-07-12T16:20:46Z

refer scalameta/metals-feature-requests#141

This PR enables Metals to auto-import missing extension method from the workspace (it doesn't support import from third-party libraries).

This is done by

Index extension methods by ScalaToplevelMtags
Show AutoImport code action for value xxx is not a member of yyy.

What I didn't in this PR

Auto import given instances
Auto import implicit classes
completion for extension method and auto-import
- I'll open an feature-request-issue

tanishiking · 2022-07-14T04:28:00Z

~~Realized it doesn't work with toplevel extensions~~ fixed

tanishiking · 2022-07-14T10:46:13Z

mtags/src/main/scala/scala/meta/internal/mtags/MtagsIndexer.scala

@@ -142,7 +142,7 @@ trait MtagsIndexer {
 definition: m.Position,
 kind: s.SymbolInformation.Kind,
 properties: Int
- ): Unit = {
+ ): String = {


I changed addSignature to return the added symbol so we can use those symbols as a next owner.

tanishiking · 2022-07-14T10:51:24Z

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala

@@ -415,6 +460,7 @@ object ScalaToplevelMtags {
 def owner: String
 def acceptMembers: Boolean
 def produceSourceToplevel: Boolean
+ def isExtension: Boolean = false


Added isExntension flag to ExpectTemplate (and Region).

if ExpectTemplate.isExtension is true on newline or lbrace, as needsToParseExtension says, enter to new Region where extension is true.

In those regions, we will index the symbol of DEFs.

tanishiking · 2022-07-14T10:52:33Z

Now, ready for review 👍

tgodzik

Looks good! TopLevels should also be run on dependency sources, so we should suggest extension methods from dependencies also not only from the workspace.

tgodzik · 2022-07-15T11:06:58Z

tests/slow/src/test/scala/tests/feature/ImportMissingSymbolCrossLspSuite.scala

+ path,
+ s"""|package x
+ |def main =
+ | println(1.<<incr>>)


Could we maybe add it to the code actions Scala 3 suite? Or at least it would be nice to see the code action applied to test if it works correctly

Tested check edit 6d49fb9 👍

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala

tanishiking · 2022-07-19T08:26:19Z

TopLevels should also be run on dependency sources, so we should suggest extension methods from dependencies also not only from the workspace.

@tgodzik
Year, it would be awesome to have! Could you give me some pointers on where to code, and where we're testing them? I'm unfamiliar with those areas. FYI @dos65

dos65 · 2022-07-19T08:55:24Z

@tanishiking I took a look at TopLevelSuite. It requires some work to make it work for Scala3 .At least, I see that BaseExpectSuite should be parametrized by an input - currently it uses only scala2 input classpath.
I think, it should be ok not to touch this suite in this PR.

tgodzik · 2022-07-19T14:51:19Z

TopLevels should also be run on dependency sources, so we should suggest extension methods from dependencies also not only from the workspace.

@tgodzik Year, it would be awesome to have! Could you give me some pointers on where to code, and where we're testing them? I'm unfamiliar with those areas. FYI @dos65

Wait, don't we already do it? We should be caching toplevels in ScalaToplevelMtags.scala, so esentially extension methods should currently work from dependencies in this PR, or did I totally forget something? Wait, we are talking just about tests, aren't we? 😅

tanishiking · 2022-07-20T07:14:02Z

Wait, we are talking just about tests, aren't we? 😅

Ahhh, I just mistook Toplevels for something like JarToplevelMtags (whatever), but you meant TopLevelSuite.

So, you mean TopLevelSuite (or maybe another suite that extends BaseExpectSuite) should generate expect files by indexing the symbols using PackageIndex against third-party libraries such as scala-library to verify there's no regression in dependencies symbol indexing?

While @dos65 talks about TopLevelSuite to work on Scala3.
However, it looks like TopLevelSuite already works against Scala3.

In TopSevelSuite,
forInput(..., Scala213) runs Mtags.toplevelsagainsttests/input/src/main/scala/example/*.scala andforInput(..., Scala3) runs Mtags.toplevels against tests/input2/src/main/scala/example/*.scala.
ToplevelSuite concat the result of both Scala213 and Scala3 and then write it to toplevel.expect file.

metals/tests/unit/src/test/scala/tests/ToplevelSuite.scala

Lines 39 to 40 in a163e22

 forInput(InputProperties.scala2(), Scala213) 

 forInput(InputProperties.scala3(), Scala3)

dos65 · 2022-07-20T10:34:36Z

While @dos65 talks about TopLevelSuite to work on Scala3.
However, it looks like TopLevelSuite already works against Scala3.

There is suspicious clause and comment but I'm not sure how it affects this test.

metals/tests/unit/src/test/scala/tests/ToplevelSuite.scala

Lines 30 to 31 in f9660d5

 // do not check symtab for Scala 3 since it's not possible currently 

 if (symtab.info(toplevel).isEmpty && dialect != Scala3) {

tgodzik

Added super minor comments, which we can fix later on. LGTM! Great feature to have, we can comment on the issue to document what is added and what still needs to be done.

tests/unit/src/test/scala/tests/ScalaToplevelSuite.scala

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala

Co-authored-by: Tomasz Godzik <tgodzik@users.noreply.github.com>

tanishiking · 2022-07-20T11:40:11Z

There is suspicious clause and comment but I'm not sure how it affects this test.

Hmm, right I'll take a look in another PR 👀

close scalameta#4212 In scalameta#4141 we started indexing the extension methods (for auto-import missing extension methods) in `workspaceSymbolIndex` where we should index only top-level symbols. Later in scalameta#4183 we decided to index those extension method symbols in a separate index (see more details around here: scalameta#4183 (review)). This PR refactor import missing extension method to use the index only for extension method symbols. This change slightly improve the performance of symbol search and reduce the memory footprint for symbol index.

close #4212 In #4141 we started indexing the extension methods (for auto-import missing extension methods) in `workspaceSymbolIndex` where we should index only top-level symbols. Later in #4183 we decided to index those extension method symbols in a separate index (see more details around here: #4183 (review)). This PR refactor import missing extension method to use the index only for extension method symbols. This change slightly improve the performance of symbol search and reduce the memory footprint for symbol index.

tanishiking changed the title ~~feat: Import missing extension method~~ [WIP] feat: Import missing extension method Jul 14, 2022

tanishiking force-pushed the import-extension branch from e9d7b19 to c67ec06 Compare July 14, 2022 10:25

tanishiking changed the title ~~[WIP] feat: Import missing extension method~~ feat: Import missing extension method Jul 14, 2022

tanishiking commented Jul 14, 2022

View reviewed changes

tanishiking requested review from dos65 and tgodzik and removed request for dos65 July 14, 2022 10:52

tanishiking mentioned this pull request Jul 14, 2022

Auto complete (missing) extension methods scalameta/metals-feature-requests#282

Closed

tgodzik requested changes Jul 15, 2022

View reviewed changes

tanishiking added 2 commits July 19, 2022 15:08

feat: Import missing extension method

7215f5e

feat: Import missing (toplevel) extension methods

4d388b8

tanishiking force-pushed the import-extension branch from a3980aa to 8dcad16 Compare July 19, 2022 06:27

fix: Do not index inner def inside of extension method

91a3903

tanishiking force-pushed the import-extension branch from 8dcad16 to 91a3903 Compare July 19, 2022 06:43

tests: test import missing extension method code action edit

6d49fb9

tanishiking requested a review from tgodzik July 19, 2022 08:26

tgodzik approved these changes Jul 20, 2022

View reviewed changes

tests/unit/src/test/scala/tests/ScalaToplevelSuite.scala Outdated Show resolved Hide resolved

tests/unit/src/test/scala/tests/ScalaToplevelSuite.scala Outdated Show resolved Hide resolved

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala Show resolved Hide resolved

Apply suggestions from code review

6527693

Co-authored-by: Tomasz Godzik <tgodzik@users.noreply.github.com>

tanishiking merged commit 29176c4 into scalameta:main Jul 20, 2022

tanishiking deleted the import-extension branch July 20, 2022 15:12

This was referenced Jul 21, 2022

Auto-import missing implicits scalameta/metals-feature-requests#141

Open

feat: Auto complete (missing) extension methods #4183

Merged

tanishiking mentioned this pull request Aug 1, 2022

refactor: Do not index extension methods in workspaceSymbolIndex #4212

Closed

tanishiking mentioned this pull request Aug 26, 2022

refactor: Do not index extension methods in workspaceSymbolIndex #4300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Import missing extension method #4141

feat: Import missing extension method #4141

tanishiking commented Jul 12, 2022 •

edited

Loading

tanishiking commented Jul 14, 2022 •

edited

Loading

tanishiking Jul 14, 2022

tanishiking Jul 14, 2022 •

edited

Loading

tanishiking commented Jul 14, 2022

tgodzik left a comment

tgodzik Jul 15, 2022

tanishiking Jul 19, 2022

tanishiking commented Jul 19, 2022

dos65 commented Jul 19, 2022 •

edited

Loading

tgodzik commented Jul 19, 2022 •

edited

Loading

tanishiking commented Jul 20, 2022

dos65 commented Jul 20, 2022

tgodzik left a comment

tanishiking commented Jul 20, 2022 •

edited

Loading

feat: Import missing extension method #4141

feat: Import missing extension method #4141

Conversation

tanishiking commented Jul 12, 2022 • edited Loading

What I didn't in this PR

tanishiking commented Jul 14, 2022 • edited Loading

tanishiking Jul 14, 2022

Choose a reason for hiding this comment

tanishiking Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

tanishiking commented Jul 14, 2022

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik Jul 15, 2022

Choose a reason for hiding this comment

tanishiking Jul 19, 2022

Choose a reason for hiding this comment

tanishiking commented Jul 19, 2022

dos65 commented Jul 19, 2022 • edited Loading

tgodzik commented Jul 19, 2022 • edited Loading

tanishiking commented Jul 20, 2022

dos65 commented Jul 20, 2022

tgodzik left a comment

Choose a reason for hiding this comment

tanishiking commented Jul 20, 2022 • edited Loading

tanishiking commented Jul 12, 2022 •

edited

Loading

tanishiking commented Jul 14, 2022 •

edited

Loading

tanishiking Jul 14, 2022 •

edited

Loading

dos65 commented Jul 19, 2022 •

edited

Loading

tgodzik commented Jul 19, 2022 •

edited

Loading

tanishiking commented Jul 20, 2022 •

edited

Loading