From b7b3ef5cffbd91416146d845fba4eaa96b57334e Mon Sep 17 00:00:00 2001 From: Tomasz Godzik Date: Thu, 8 Aug 2024 18:36:32 +0200 Subject: [PATCH] improvement: Further heuristic improvements Couple of related improvements: - escape operators, they are escaped for semanticdb symbols ( otherwise ??? will not be found) - add standard Scala imports (needed for scala.Predef.`???`().) - also try and guess method with (). ending - check if the returned symbol is exactly the same as we were looking for (could not reproduce it 100% the same as I encountered in the wild, but it's close enough) --- .../metals/FallbackDefinitionProvider.scala | 22 +++++-- .../meta/internal/mtags/KeywordWrapper.scala | 12 ++-- .../scala/meta/internal/mtags/Symbol.scala | 3 +- .../feature/DefinitionCrossLspSuite.scala | 64 +++++++++++++++++++ 4 files changed, 91 insertions(+), 10 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala index 177c5c39f09..b797f505825 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala @@ -56,7 +56,7 @@ class FallbackDefinitionProvider( .value if (isInSelectPosition) List(symbolPrefix + ".") else if (isInTypePosition) List(symbolPrefix + "#") - else List(".", "#").map(ending => symbolPrefix + ending) + else List(".", "#", "().").map(ending => symbolPrefix + ending) } // Get all select parts to build symbol from it later @@ -114,16 +114,26 @@ class FallbackDefinitionProvider( }.flatten }.flatten + val standardScalaImports = guessObjectOrClass( + List("scala", "Predef") ++ proposedNameParts + ) val fullyScopedName = guessObjectOrClass(proposedNameParts) + def findInIndex(proposedSymbol: String) = { + index + .definition(mtags.Symbol(proposedSymbol)) + // Make sure we don't return unrelated definitions + .filter { _.definitionSymbol.value == proposedSymbol } + } val nonLocalGuesses = - (proposedImportedSymbols ++ fullyScopedName).distinct + (proposedImportedSymbols ++ fullyScopedName ++ standardScalaImports).distinct .flatMap { proposedSymbol => - index.definition(mtags.Symbol(proposedSymbol)) + findInIndex(proposedSymbol) } def toDefinition(guesses: List[mtags.SymbolDefinition]) = { + DefinitionResult( guesses .flatMap(guess => @@ -143,8 +153,10 @@ class FallbackDefinitionProvider( } else { // otherwise might be symbol in a local package, starting from enclosing proposedCurrentPackageSymbols.reverse - .map(proposedSymbol => index.definition(mtags.Symbol(proposedSymbol))) - .collectFirst { case Some(dfn) => toDefinition(List(dfn)) } + .map(proposedSymbol => findInIndex(proposedSymbol)) + .collectFirst { case Some(dfn) => + toDefinition(List(dfn)) + } } result.foreach { _ => diff --git a/mtags-shared/src/main/scala/scala/meta/internal/mtags/KeywordWrapper.scala b/mtags-shared/src/main/scala/scala/meta/internal/mtags/KeywordWrapper.scala index da675ad749f..5e86e5916a9 100644 --- a/mtags-shared/src/main/scala/scala/meta/internal/mtags/KeywordWrapper.scala +++ b/mtags-shared/src/main/scala/scala/meta/internal/mtags/KeywordWrapper.scala @@ -11,9 +11,12 @@ trait KeywordWrapper { def keywords: Set[String] - final def needsBacktick(s: String): Boolean = { + final def needsBacktick( + s: String, + wrapOperators: Boolean = false + ): Boolean = { val chunks = s.split("_", -1) - def validOperator(c: Char) = { + def validOperator(c: Char) = !wrapOperators && { c.getType == Character.MATH_SYMBOL || c.getType == Character.OTHER_SYMBOL || "!#%&*+-/:<=>?@\\^|~".contains(c) @@ -45,12 +48,13 @@ trait KeywordWrapper { final def backtickWrap( s: String, - exclusions: Set[String] = Set.empty + exclusions: Set[String] = Set.empty, + wrapOperators: Boolean = false ): String = { if (exclusions.contains(s)) s else if (s.isEmpty) "``" else if (s(0) == '`' && s.last == '`') s - else if (needsBacktick(s)) "" + ('`') + s + '`' + else if (needsBacktick(s, wrapOperators)) "" + ('`') + s + '`' else s } } diff --git a/mtags/src/main/scala/scala/meta/internal/mtags/Symbol.scala b/mtags/src/main/scala/scala/meta/internal/mtags/Symbol.scala index 85aa46b26b4..51d432b1939 100644 --- a/mtags/src/main/scala/scala/meta/internal/mtags/Symbol.scala +++ b/mtags/src/main/scala/scala/meta/internal/mtags/Symbol.scala @@ -161,7 +161,8 @@ object Symbol { case ((prefix, insideObject), next) => val name = keywordWrapper.backtickWrap( next.replace("\\", ""), - Set("this", "package") + Set("this", "package"), + wrapOperators = true ) if (prefix.isEmpty()) (name, /*insideObject =*/ name.find(_.isLetter).exists(_.isUpper)) diff --git a/tests/slow/src/test/scala/tests/feature/DefinitionCrossLspSuite.scala b/tests/slow/src/test/scala/tests/feature/DefinitionCrossLspSuite.scala index d6ff76f2d56..bf7fa4b1aea 100644 --- a/tests/slow/src/test/scala/tests/feature/DefinitionCrossLspSuite.scala +++ b/tests/slow/src/test/scala/tests/feature/DefinitionCrossLspSuite.scala @@ -638,5 +638,69 @@ class DefinitionCrossLspSuite ) } yield () } + + test(s"fallback-object-imports-$version") { + cleanWorkspace() + for { + _ <- initialize( + s""" + |/metals.json + |{ + | "a": { "scalaVersion" : "${version}" } + |} + |/a/src/main/scala/a/Main.scala + |package a + |import b.Foo._ + | + |object Main { + | val name: Version = ??? + |} + | + |/a/src/main/scala/a/b/Foo.scala + |package a.b + | + |trait Foo + |object Foo extends Foo{ + | val a = 123 + |} + | + |""".stripMargin + ) + _ <- server.didOpen("a/src/main/scala/a/Main.scala") + _ <- server.didOpen("a/src/main/scala/a/b/Foo.scala") + _ = assertNoDiff( + client.workspaceDiagnostics, + if (version == BuildInfo.scala3) + """|a/src/main/scala/a/Main.scala:5:13: error: Not found: type Version + | val name: Version = ??? + | ^^^^^^^ + |""".stripMargin + else + """|a/src/main/scala/a/Main.scala:5:13: error: not found: type Version + | val name: Version = ??? + | ^^^^^^^ + |""".stripMargin, + ) + _ = assertNoDiff( + server.workspaceDefinitions, + """|/a/src/main/scala/a/Main.scala + |package a + |import b/**/.Foo/*Foo.scala:3*/._ + | + |object Main/*L3*/ { + | val name/*L4*/: Version/**/ = ???/*Predef.scala*/ + |} + | + |/a/src/main/scala/a/b/Foo.scala + |package a.b + | + |trait Foo/*L2*/ + |object Foo/*L3*/ extends Foo/*L2*/{ + | val a/*L4*/ = 123 + |} + |""".stripMargin, + ) + } yield () + } } }