feature: go to implementations for dependency sources #5623

kasiaMarek · 2023-09-06T16:08:49Z

Go to implementation for dependency sources scalameta/metals-feature-requests#119.
resolves: #1287
We index the type hierarchy using ScalaTopLevelMtags, only saving the name of the overridden symbol and its usage position, the actual symbol is resolved lazily.

Shortcomings:

won't work for renamed import

Benchmark                        Mode  Cnt  Score   Error  Units
MetalsBench.toplevelsScalaIndex    ss   10  0.332 ± 0.007   s/op
MetalsBench.typeHierarchyIndex     ss   10  0.331 ± 0.004   s/op

follow up: provide solution for handling renamed symbols

filipwiech · 2023-09-08T10:02:47Z

This is awesome! Thank you for working on this, it's something I was missing when coming from the IntelliJ. 👍

jkciesluk

That’s an amazing feature! I have few nitpicks

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

metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala

jkciesluk · 2023-09-14T13:26:26Z

metals/src/main/scala/scala/meta/internal/implementation/InheritanceContext.scala

+      symbol: String
+  )(implicit ec: ExecutionContext): Future[Set[ClassLocation]] = {
+    val workspaceImplementations = getWorkspaceLocations(symbol)
+    val enumCasesImplementations =


Could you add a comment explaining why we have enumCases here? I feel like I will soon forget it's because they are resolved during indexing

tgodzik

Great work! Sorry for taking so long to review.

metals/src/main/scala/scala/meta/internal/implementation/ClassLocation.scala

metals/src/main/resources/db/migration/V5__Jar_type_hierarchy.sql

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

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

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

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

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

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

tests/unit/src/test/scala/tests/ImplementationLspSuite.scala

tests/unit/src/test/scala/tests/JarTopLevelsSuite.scala

… in dependencies

tgodzik

Should this work for Scala standard library? I am trying to find implementations of Seq but nothing is returned. Maybe you could add a test in ImplementationLspSuite?

tgodzik · 2023-11-22T15:23:52Z

Ok, it seems when I try it in the Metals repository it works sometimes, but each time it actually generates heaps of errors such as:

jar:file://<HOME>/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.17/scala-library-2.12.17-sources.jar!/scala/collection/immutable/MapProxy.scala
### jar%3Afile%3A%2F%2F%2Fhome%2Ftgodzik%2F.cache%2Fcoursier%2Fv1%2Fhttps%2Frepo1.maven.org%2Fmaven2%2Forg%2Fscala-lang%2Fscala-library%2F2.12.17%2Fscala-library-2.12.17-sources.jar%21%2Fscala%2Fcollection%2Fimmutable%2FMapProxy.scala:20: error: ; expected but package found
package collection
^

occurred in the presentation compiler.

action parameters:
uri: jar:file://<HOME>/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.17/scala-library-2.12.17-sources.jar!/scala/collection/immutable/MapProxy.scala

och after a while I get an OOM.

It seems to have work on very basic Set, on Set in Predef, but not the Set in immutable package.

Could you take a look?

Z1kkurat · 2024-01-10T09:14:36Z

This is amazing, I'm so excited about this feature, thanks a million for your effort! 🎉

kasiaMarek · 2024-01-16T15:26:30Z

tests/unit/src/test/scala/tests/ImplementationLspSuite.scala

  check(
-    "java-implementation",
+    "java-implementation".ignore,


I ignored tests with java files. We need to implement info in java pc. Alternatively, we can force Compilers to use scala pc for info (at least for now), this should make go to impl work in java files that belong to scala build target.

Let's note what still need to be done in the feature request

kasiaMarek · 2024-01-16T15:29:55Z

tests/unit/src/test/scala/tests/ImplementationLspSuite.scala

  check(
-    "java-classes",
+    "java-classes".ignore,


To make this work we need JavaMtags (or JavaTopLevelMatgs) to return information about overridden symbols. Now we don't know that RuntimeException implements Exception, and only MyException will be found.

kasiaMarek · 2024-01-16T15:38:50Z

metals/src/main/scala/scala/meta/internal/search/SymbolHierarchyOps.scala

+
+import org.eclipse.lsp4j.Location
+
+class SymbolHierarchyOps(


All those things are moved from ImplementationProvider. Those are not needed for go to implementation anymore but are still used by Supermethods and RenameProvider. I didn't want to try and remove those in this PR.

tgodzik

I will still check out it locally to see if we didn't miss anything.

metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala

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

mtags-shared/src/main/scala/scala/meta/internal/pc/PcSymbolInformation.scala

tgodzik · 2024-01-20T12:57:35Z

tests/unit/src/test/scala/tests/ImplementationLspSuite.scala

  check(
-    "java-implementation",
+    "java-implementation".ignore,


Let's note what still need to be done in the feature request

metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala

…-dependecy-sources

…ndecy-sources

tgodzik

Looks good, just a couple of comments

tgodzik · 2024-02-16T17:17:57Z

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

@@ -840,7 +859,8 @@ class Compilers(
  }

  def loadCompiler(
-      path: AbsolutePath
+      path: AbsolutePath,
+      forceScala: Boolean = false,


Could you add docs saying why this is needed?

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

tgodzik · 2024-02-21T13:18:54Z

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

@@ -179,6 +179,10 @@ public CompletableFuture<List<SyntheticDecoration>> syntheticDecorations(Synthet
 		return CompletableFuture.completedFuture(Collections.emptyList());
 	}

+	public CompletableFuture<List<PcSymbolInformation>> info(String symbol) {


When can it return multiple? Is there a known case?

overloaded methods

Though usually they have a different symbol

method()
method(+1)
etc.

so this should not repeat.

Yes, but I want it to return all. When we search for implementations of methods, we find implementations of owner class/trait and only then implementations of method, needs to be this way since we index only class hierarchy. E.g. if class A extends B and we're looking for implementations of B.method(+1), the searched method will be A.method but we don't know the disambiguator so I get them all to find the one I need. In Compilers.scala there is info and infoAll, the first one does exactly what you'd expect and returns only info about that exact symbol.

If this is too odd, we can add overloadedSymbols to PcSymbolInformation to learn all the overloaded and get info about each one by one.

@tgodzik, would you prefer I change this?

Could we get a full method symbol ? This should be possible using Mtags class. 🤔

tgodzik

LGTM! Amazing work!

kasiaMarek mentioned this pull request Sep 6, 2023

feature: collect overridden symbols in Scala toplevel mtags #5607

Closed

kasiaMarek marked this pull request as ready for review September 7, 2023 16:20

jkciesluk requested changes Sep 14, 2023

View reviewed changes

kasiaMarek requested review from jkciesluk and tgodzik September 21, 2023 13:33

kasiaMarek force-pushed the implementations-for-dependecy-sources branch from 7b716d1 to 37ebd1b Compare October 2, 2023 14:04

tgodzik requested changes Oct 26, 2023

View reviewed changes

kasiaMarek added 10 commits October 27, 2023 15:59

feature: index overridden symbols in top level

ebf5157

feature: collect overridden symbols in Scala toplevel mtags

7b95aba

feature: index overriden symbols

950aa04

feature: use overridden index information for finding implementations…

1d4dada

… in dependencies

store type hierarchy information

5fa2a66

bugfix: cover class A extends b.B when indexing type hierarchy

57694a8

tests

8a6c7b6

add benchmark

ed822f6

review fixes

97bca82

review fixes

46fb233

kasiaMarek force-pushed the implementations-for-dependecy-sources branch from 37ebd1b to 9b91b06 Compare October 27, 2023 16:11

kasiaMarek added 2 commits October 30, 2023 18:16

get rid of enriched document

ede5b9a

use toAbsolutePath in JarTopLevels

25bf321

kasiaMarek force-pushed the implementations-for-dependecy-sources branch from 9b91b06 to 25bf321 Compare October 30, 2023 17:17

kasiaMarek added 2 commits November 21, 2023 21:20

Merge branch 'dev' into implementations-for-dependecy-sources

93afe40

fixes after merge

83436ad

kasiaMarek force-pushed the implementations-for-dependecy-sources branch from 352fa37 to 83436ad Compare November 21, 2023 20:26

kasiaMarek requested a review from tgodzik November 22, 2023 09:01

tgodzik requested changes Nov 22, 2023

View reviewed changes

fix resolving if symbol is defined in the workspace

357b3c0

kasiaMarek requested a review from tgodzik November 28, 2023 12:25

jkciesluk approved these changes Jan 12, 2024

View reviewed changes

kasiaMarek added 4 commits January 16, 2024 12:40

remove usage of global symbol table from go to implementation

c0299e5

move unused methods from implementation provider

22b41d3

don't use scala pc for java

ecdc166

make sure found implementations are within build target

1d86b8e

kasiaMarek commented Jan 16, 2024

View reviewed changes

kasiaMarek force-pushed the implementations-for-dependecy-sources branch from 688e897 to f285962 Compare January 16, 2024 15:33

kasiaMarek commented Jan 16, 2024

View reviewed changes

fix oom + format

8c6fdbc

kasiaMarek force-pushed the implementations-for-dependecy-sources branch from 0dd0fea to 8c6fdbc Compare January 17, 2024 14:12

kasiaMarek requested review from tgodzik and jkciesluk January 17, 2024 15:33

tgodzik reviewed Jan 20, 2024

View reviewed changes

kasiaMarek mentioned this pull request Feb 1, 2024

bugfix: handle nested java jars #6066

Merged

kasiaMarek added 4 commits February 9, 2024 11:22

Merge remote-tracking branch 'upstream/main' into implementations-for…

e4b2833

…-dependecy-sources

review changes

3542435

use scala pc for java files

0c22ffc

Merge remote-tracking branch 'upstream' into implementations-for-depe…

2de93af

…ndecy-sources

tgodzik approved these changes Feb 21, 2024

View reviewed changes

kasiaMarek added the backport-scala3-pc label Feb 22, 2024

make info return result exactly for searched symbol

80d64f9

kasiaMarek requested a review from tgodzik February 23, 2024 16:05

tgodzik approved these changes Feb 25, 2024

View reviewed changes

kasiaMarek merged commit 2d1738a into scalameta:main Feb 26, 2024
25 of 26 checks passed

This was referenced Feb 28, 2024

add info implementation to pc scala/scala3#19812

Merged

feat: index type hierarchy in java files #6189

Merged

tgodzik removed the backport-scala3-pc label Mar 11, 2024

tgodzik mentioned this pull request Aug 8, 2024

Allow auto imports of type members scalameta/metals-feature-requests#378

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: go to implementations for dependency sources #5623

feature: go to implementations for dependency sources #5623

kasiaMarek commented Sep 6, 2023 •

edited

Loading

filipwiech commented Sep 8, 2023

jkciesluk left a comment

jkciesluk Sep 14, 2023

tgodzik left a comment

tgodzik left a comment

tgodzik commented Nov 22, 2023

Z1kkurat commented Jan 10, 2024

kasiaMarek Jan 16, 2024

tgodzik Jan 20, 2024

kasiaMarek Jan 16, 2024

kasiaMarek Jan 16, 2024

tgodzik left a comment

tgodzik Jan 20, 2024

tgodzik left a comment

tgodzik Feb 16, 2024

tgodzik Feb 21, 2024

kasiaMarek Feb 21, 2024

tgodzik Feb 21, 2024

kasiaMarek Feb 21, 2024

kasiaMarek Feb 23, 2024

tgodzik Feb 23, 2024

tgodzik left a comment

feature: go to implementations for dependency sources #5623

feature: go to implementations for dependency sources #5623

Conversation

kasiaMarek commented Sep 6, 2023 • edited Loading

filipwiech commented Sep 8, 2023

jkciesluk left a 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

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik commented Nov 22, 2023

Z1kkurat commented Jan 10, 2024

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

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

tgodzik left a comment

Choose a reason for hiding this comment

kasiaMarek commented Sep 6, 2023 •

edited

Loading