Use DefinitionProvider instead of index in RenameProvider #1637

tgodzik · 2020-04-17T13:07:07Z

Previously, we would use GlobalSymbolIndex, which might not always work for Scala 3 in case of non-parsable code. Now, we use the definitionProvider, which will in turn ask the PresentationCompiler if the definition is missing. For Scala 2 it will still work mostly the same.

The test might fail locally due to issue being fixed scalacenter/bloop#1253, which makes the test fail on even tries 😅

…e we always get the definition for Dotty

kpbochenek · 2020-04-23T16:17:38Z

metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala

+        val symbolOccurence =
+          definitionProvider.symbolOccurrence(source, params.getPosition)
+        for {
+          (occurence, _) <- symbolOccurence


rename symbolOccurence -> symbolOccurrence let's fix this once and for all in this file 💪

kpbochenek · 2020-04-23T16:43:07Z

a little unrelated but when I looked at def definition( it contains:

if (result.isEmpty) {
        compilers().definition(params, token)
      } else {
        if (result.isEmpty && fromSemanticdb.isEmpty) {
          warnings.noSemanticdb(path)
        }
        Future.successful(result)
      }

in second if result cannot be empty

kpbochenek · 2020-04-23T16:59:07Z

metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala

  }

-  private def symbolIsLocal(symbol: String): Boolean = {
+  private def symbolIsLocal(


maybe we could rename it to isLocalSymbol ?

also inside it is symbol.startsWith("local") but we can simply reuse symbol.isLocal which is performing exactly this check.

fixed, but renamed isWorkspaceSymbol

kpbochenek · 2020-04-23T17:01:07Z

metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala

-          isLocal
-        }
+    def isFromWorkspace = {
+      val isLocal = definitionPath.isWorkspaceSource(workspace)


for me isLocal has a meaning that symbol is localXXX(anonymous in a given file) and here we talk about isInWorkspace ?

Changed to isInWorkspace

kpbochenek · 2020-04-23T17:16:19Z

metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala

@@ -267,7 +279,11 @@ final class RenameProvider(
    }
  }

-  private def canRenameSymbol(symbol: String, newName: Option[String]) = {
+  private def canRenameSymbol(
+      symbol: String,


I think you try to bind symbol with AbsolutePath and propagate them together through multiple functions, maybe it is worth making case class containing those 2 things and just pass one thing. Also it will be easier to see this path is related to this symbol

Okay actually I would go with symbol <-> List[AbsolutePath] as it looks like there can be multiple definitions of a symbol. This would fix a problem with <- above

And then you can decide in symbolIsLocal if given symbol with all definition locations is local or not.
Previously we did .exists which means at least one location must be in workspace, but maybe .all would make more sense here?
I don't know many multi def locations examples, is it only class/trait + object scenario?

I don't think we have multiple definitions anywhere currently really. And even if we get one, I can't imagine one being inside workspace and the other outside.

I can rework it a bit, but I don't think we need to add a case class just for 2 methods.

Moved isWorkspaceSymbol up, so that we don't propagate the path now.

kpbochenek · 2020-04-23T17:28:25Z

metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala

-            includeSynthetics = includeSynthetic
+        val allReferences = for {
+          (occurence, semanticDb) <- symbolOccurence.toIterable
+          definitionLoc <- definition.locations.asScala


I would not go with <- here as it means if you get more locations all references and will be calculated twice for each location

I should most likely go with headOption, but we don't have really currently a situation with multiple definitions.

kpbochenek · 2020-04-23T18:35:28Z

From what I see you basically replace definitionProvider.fromSymbol(occurence.symbol) with definitionProvider.definition(source, params, token).
Two questions:

Would it make sense to use index and in case of no result fallback to DefProvider?
There are multiple places where definitionProvider.fromSymbol, would it make sense to also change them?

kpbochenek

(Some comments might not be directly related to changes in code in this PR, ignore if you feel like unrelated)

tgodzik · 2020-04-24T07:47:51Z

From what I see you basically replace definitionProvider.fromSymbol(occurence.symbol) with definitionProvider.definition(source, params, token).
Two questions:

Would it make sense to use index and in case of no result fallback to DefProvider?

DefProvider does it already, uses index to locate the SemanticDB with the definition and then searches that SemanticDB. If it's not available there then we fallaback to PC

There are multiple places where definitionProvider.fromSymbol, would it make sense to also change them?

It might make sense, but before Dotty this wasn't needed. We might need to go through all the functionalities and confirm they're working sensibly in all scenarios.

kpbochenek

LGTM 👍

mlachkar

LGTM too

mlachkar · 2020-04-27T09:13:36Z

metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala

+          (occurence, semanticDb) <- symbolOccurrence.toIterable
+          definitionLoc <- definition.locations.asScala.headOption.toIterable
+          definitionPath = definitionLoc.getUri().toAbsolutePath
+          if canRenameSymbol(occurence.symbol, Option(newName)) &&


OPT: same than Some(newName)

Will fix it maybe next time, shouldn't be a big issue.

Use DefinitionProvider instead of index in RenameProvider to make sur…

a44e9a6

…e we always get the definition for Dotty

tgodzik mentioned this pull request Apr 18, 2020

Support for Scala 3 #1367

Closed

tgodzik requested review from kpbochenek and mlachkar April 22, 2020 15:40

kpbochenek reviewed Apr 23, 2020

View reviewed changes

kpbochenek requested changes Apr 23, 2020

View reviewed changes

ckipp01 added the improvement Not a bug or a feature, but something general we can improve label Apr 23, 2020

tgodzik force-pushed the fix-dotty-rename branch from d9f4dd5 to cc52b2d Compare April 27, 2020 09:04

Move Dotty test to slow tests in order to have proper mtags published

fc20da6

tgodzik force-pushed the fix-dotty-rename branch from cc52b2d to fc20da6 Compare April 27, 2020 09:08

tgodzik requested a review from kpbochenek April 27, 2020 09:10

kpbochenek approved these changes Apr 27, 2020

View reviewed changes

mlachkar approved these changes Apr 27, 2020

View reviewed changes

tgodzik merged commit 85aa6c4 into scalameta:master Apr 27, 2020

tgodzik deleted the fix-dotty-rename branch April 27, 2020 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use DefinitionProvider instead of index in RenameProvider #1637

Use DefinitionProvider instead of index in RenameProvider #1637

tgodzik commented Apr 17, 2020

kpbochenek Apr 23, 2020

tgodzik Apr 27, 2020

kpbochenek commented Apr 23, 2020

kpbochenek Apr 23, 2020

tgodzik Apr 27, 2020 •

edited

Loading

kpbochenek Apr 23, 2020

tgodzik Apr 27, 2020 •

edited

Loading

kpbochenek Apr 23, 2020

kpbochenek Apr 23, 2020

kpbochenek Apr 23, 2020

tgodzik Apr 24, 2020

tgodzik Apr 27, 2020

kpbochenek Apr 23, 2020

tgodzik Apr 24, 2020

kpbochenek commented Apr 23, 2020

kpbochenek left a comment

tgodzik commented Apr 24, 2020 •

edited

Loading

kpbochenek left a comment

mlachkar left a comment

mlachkar Apr 27, 2020

tgodzik Apr 27, 2020

Use DefinitionProvider instead of index in RenameProvider #1637

Use DefinitionProvider instead of index in RenameProvider #1637

Conversation

tgodzik commented Apr 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpbochenek commented Apr 23, 2020

Choose a reason for hiding this comment

tgodzik Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik Apr 27, 2020 • 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

Choose a reason for hiding this comment

kpbochenek commented Apr 23, 2020

kpbochenek left a comment

Choose a reason for hiding this comment

tgodzik commented Apr 24, 2020 • edited Loading

kpbochenek left a comment

Choose a reason for hiding this comment

mlachkar 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 Apr 27, 2020 •

edited

Loading

tgodzik Apr 27, 2020 •

edited

Loading

tgodzik commented Apr 24, 2020 •

edited

Loading