From ac01eef9ca6bc265568c3d9629473317e66f6578 Mon Sep 17 00:00:00 2001 From: Katarzyna Marek Date: Mon, 9 Dec 2024 10:12:22 +0100 Subject: [PATCH] fix: use alternative with stripped synthetic `filename$package.` for go to def and docs (#7000) --- .../metals/ProxySymbolDocumentation.scala | 18 ++++++++ .../internal/pc/CompletionItemResolver.scala | 2 +- .../internal/mtags/MtagsEnrichments.scala | 7 +-- .../internal/pc/CompletionItemResolver.scala | 2 +- .../meta/internal/metals/Docstrings.scala | 43 +++++++++++++++---- .../mtags/DefinitionAlternatives.scala | 16 +++++++ .../mtags/ScalametaCommonEnrichments.scala | 3 ++ .../internal/mtags/SymbolIndexBucket.scala | 3 +- .../main/scala/tests/pc/BaseHoverSuite.scala | 16 ++++--- .../tests/hover/HoverScala3TypeSuite.scala | 17 ++++++++ .../scala/tests/pc/SignatureHelpSuite.scala | 2 +- 11 files changed, 107 insertions(+), 22 deletions(-) create mode 100644 mtags-shared/src/main/scala/scala/meta/internal/metals/ProxySymbolDocumentation.scala diff --git a/mtags-shared/src/main/scala/scala/meta/internal/metals/ProxySymbolDocumentation.scala b/mtags-shared/src/main/scala/scala/meta/internal/metals/ProxySymbolDocumentation.scala new file mode 100644 index 00000000000..ec0fc6f99f1 --- /dev/null +++ b/mtags-shared/src/main/scala/scala/meta/internal/metals/ProxySymbolDocumentation.scala @@ -0,0 +1,18 @@ +package scala.meta.internal.metals + +import java.util +import java.util.Collections + +import scala.meta.pc.SymbolDocumentation + +case class ProxySymbolDocumentation(orginalSymbol: String) + extends SymbolDocumentation { + override def symbol(): String = "" + override def displayName(): String = "" + override def docstring(): String = "" + override def defaultValue(): String = "" + override def typeParameters(): util.List[SymbolDocumentation] = + Collections.emptyList() + override def parameters(): util.List[SymbolDocumentation] = + Collections.emptyList() +} diff --git a/mtags/src/main/scala-2/scala/meta/internal/pc/CompletionItemResolver.scala b/mtags/src/main/scala-2/scala/meta/internal/pc/CompletionItemResolver.scala index 6227a027ed8..8dcb6d355db 100644 --- a/mtags/src/main/scala-2/scala/meta/internal/pc/CompletionItemResolver.scala +++ b/mtags/src/main/scala-2/scala/meta/internal/pc/CompletionItemResolver.scala @@ -75,7 +75,7 @@ class CompletionItemResolver( } } else { val companionDoc = docs(companion) - if (companionDoc.isEmpty) gsymDoc + if (companionDoc.isEmpty || companionDoc == gsymDoc) gsymDoc else if (gsymDoc.isEmpty) companionDoc else { List( diff --git a/mtags/src/main/scala-3/scala/meta/internal/mtags/MtagsEnrichments.scala b/mtags/src/main/scala-3/scala/meta/internal/mtags/MtagsEnrichments.scala index 316aef28277..4431ade44fe 100644 --- a/mtags/src/main/scala-3/scala/meta/internal/mtags/MtagsEnrichments.scala +++ b/mtags/src/main/scala-3/scala/meta/internal/mtags/MtagsEnrichments.scala @@ -230,9 +230,10 @@ object MtagsEnrichments extends ScalametaCommonEnrichments: def stripBackticks: String = s.stripPrefix("`").stripSuffix("`") extension (search: SymbolSearch) - def symbolDocumentation(symbol: Symbol, contentType: ContentType = ContentType.MARKDOWN)(using - Context - ): Option[SymbolDocumentation] = + def symbolDocumentation( + symbol: Symbol, + contentType: ContentType = ContentType.MARKDOWN + )(using Context): Option[SymbolDocumentation] = def toSemanticdbSymbol(symbol: Symbol) = SemanticdbSymbols.symbolName( if !symbol.is(JavaDefined) && symbol.isPrimaryConstructor then diff --git a/mtags/src/main/scala-3/scala/meta/internal/pc/CompletionItemResolver.scala b/mtags/src/main/scala-3/scala/meta/internal/pc/CompletionItemResolver.scala index 40152b8c6e6..4bc400c5823 100644 --- a/mtags/src/main/scala-3/scala/meta/internal/pc/CompletionItemResolver.scala +++ b/mtags/src/main/scala-3/scala/meta/internal/pc/CompletionItemResolver.scala @@ -71,7 +71,7 @@ object CompletionItemResolver extends ItemResolver: else gsymDoc else val companionDoc = docs(companion) - if companionDoc.isEmpty then gsymDoc + if companionDoc.isEmpty || companionDoc == gsymDoc then gsymDoc else if gsymDoc.isEmpty then companionDoc else List( diff --git a/mtags/src/main/scala/scala/meta/internal/metals/Docstrings.scala b/mtags/src/main/scala/scala/meta/internal/metals/Docstrings.scala index 434a511c1d6..04b053dcea2 100644 --- a/mtags/src/main/scala/scala/meta/internal/metals/Docstrings.scala +++ b/mtags/src/main/scala/scala/meta/internal/metals/Docstrings.scala @@ -40,16 +40,15 @@ class Docstrings(index: GlobalSymbolIndex) { parents: ParentSymbols, contentType: ContentType ): Optional[SymbolDocumentation] = { - val content = Content.from(symbol, contentType) - val result = cache.get(content) match { + val result = getFromCacheWithProxy(symbol, contentType) match { case Some(value) => if (value == EmptySymbolDocumentation) None else Some(value) case None => indexSymbol(symbol, contentType) - val result = cache.get(content) + val result = getFromCacheWithProxy(symbol, contentType) if (result.isEmpty) - cache(content) = EmptySymbolDocumentation + cache(Content.from(symbol, contentType)) = EmptySymbolDocumentation result } /* Fall back to parent javadocs/scaladocs if nothing is specified for the current symbol @@ -83,11 +82,9 @@ class Docstrings(index: GlobalSymbolIndex) { .parents() .asScala .flatMap { s => - val content = Content.from(s, contentType) - if (cache.contains(content)) cache.get(content) - else { + getFromCacheWithProxy(s, contentType).orElse { indexSymbol(s, contentType) - cache.get(content) + getFromCacheWithProxy(s, contentType) } } .find(_.docstring().nonEmpty) @@ -100,6 +97,17 @@ class Docstrings(index: GlobalSymbolIndex) { } } + private def getFromCacheWithProxy( + symbol: String, + contentType: ContentType + ): Option[SymbolDocumentation] = { + cache.get(Content.from(symbol, contentType)) match { + case Some(ProxySymbolDocumentation(alternativeSymbol)) => + cache.get(Content.from(alternativeSymbol, contentType)) + case res => res + } + } + /** * Expire all symbols showed in the given scala source file. * @@ -130,6 +138,7 @@ class Docstrings(index: GlobalSymbolIndex) { case Some(defn) => try { indexSymbolDefinition(defn, contentType) + maybeCacheAlternative(defn, contentType) } catch { case NonFatal(e) => logger.log(Level.SEVERE, defn.path.toURI.toString, e) @@ -138,6 +147,22 @@ class Docstrings(index: GlobalSymbolIndex) { } } + private def maybeCacheAlternative( + defn: SymbolDefinition, + contentType: ContentType + ) = { + val defSymbol = defn.definitionSymbol.value + val querySymbol = defn.querySymbol.value + lazy val queryContent = Content.from(querySymbol, contentType) + + if ( + defSymbol != querySymbol && cache.get(queryContent).forall { + case EmptySymbolDocumentation | _: ProxySymbolDocumentation => true + case _ => false + } + ) cache.put(queryContent, new ProxySymbolDocumentation(defSymbol)) + } + private def indexSymbolDefinition( defn: SymbolDefinition, contentType: ContentType @@ -166,7 +191,7 @@ class Docstrings(index: GlobalSymbolIndex) { ): Unit = { for { contentType <- ContentType.values() - } cache.remove(Content.from(occ.symbol, contentType)) + } cache.remove(Content.from(sinfo.symbol, contentType)) } } diff --git a/mtags/src/main/scala/scala/meta/internal/mtags/DefinitionAlternatives.scala b/mtags/src/main/scala/scala/meta/internal/mtags/DefinitionAlternatives.scala index c8e54f2b915..b9568843c2d 100644 --- a/mtags/src/main/scala/scala/meta/internal/mtags/DefinitionAlternatives.scala +++ b/mtags/src/main/scala/scala/meta/internal/mtags/DefinitionAlternatives.scala @@ -9,6 +9,7 @@ object DefinitionAlternatives { */ def apply(symbol: Symbol): List[Symbol] = { List( + stripSyntheticPackageObjectFromObject(symbol), caseClassCompanionToType(symbol), caseClassApplyOrCopy(symbol), caseClassApplyOrCopyParams(symbol), @@ -26,6 +27,21 @@ object DefinitionAlternatives { Some(sym.owner -> sym.value.desc) } + private def stripSyntheticPackageObjectFromObject( + symbol: Symbol + ): Option[Symbol] = { + val toplevel = symbol.toplevel + Option(toplevel).flatMap { + case GlobalSymbol(owner, Descriptor.Term(pkgName)) + if pkgName.endsWith("$package") => + val newSymbol = + Symbol(s"${owner.value}${symbol.value.stripPrefix(toplevel.value)}") + if (newSymbol.toplevel.isTerm) Some(newSymbol) + else None + case _ => None + } + } + /** * If `case class A(a: Int)` and there is no companion object, resolve * `A` in `A(1)` to the class definition. diff --git a/mtags/src/main/scala/scala/meta/internal/mtags/ScalametaCommonEnrichments.scala b/mtags/src/main/scala/scala/meta/internal/mtags/ScalametaCommonEnrichments.scala index cfb11bad68d..ee139e1b6a5 100644 --- a/mtags/src/main/scala/scala/meta/internal/mtags/ScalametaCommonEnrichments.scala +++ b/mtags/src/main/scala/scala/meta/internal/mtags/ScalametaCommonEnrichments.scala @@ -363,6 +363,9 @@ trait ScalametaCommonEnrichments extends CommonMtagsEnrichments { def filename: String = path.toNIO.filename + def scalaFileName: String = + path.filename.stripSuffix(".scala").stripSuffix(".sc") + def toIdeallyRelativeURI(sourceItemOpt: Option[AbsolutePath]): String = sourceItemOpt match { case Some(sourceItem) => diff --git a/mtags/src/main/scala/scala/meta/internal/mtags/SymbolIndexBucket.scala b/mtags/src/main/scala/scala/meta/internal/mtags/SymbolIndexBucket.scala index cdd97cc8f27..b7bd3efffd9 100644 --- a/mtags/src/main/scala/scala/meta/internal/mtags/SymbolIndexBucket.scala +++ b/mtags/src/main/scala/scala/meta/internal/mtags/SymbolIndexBucket.scala @@ -223,8 +223,7 @@ class SymbolIndexBucket( } if (!definitions.contains(symbol.value)) { // Fallback 3: guess related symbols from the enclosing class. - DefinitionAlternatives(symbol) - .flatMap(alternative => query0(querySymbol, alternative)) + DefinitionAlternatives(symbol).flatMap(query0(querySymbol, _)) } else { definitions .get(symbol.value) diff --git a/tests/cross/src/main/scala/tests/pc/BaseHoverSuite.scala b/tests/cross/src/main/scala/tests/pc/BaseHoverSuite.scala index 602f4f83f76..7e8a7501f9a 100644 --- a/tests/cross/src/main/scala/tests/pc/BaseHoverSuite.scala +++ b/tests/cross/src/main/scala/tests/pc/BaseHoverSuite.scala @@ -24,9 +24,11 @@ abstract class BaseHoverSuite expected: String, includeRange: Boolean = false, automaticPackage: Boolean = true, - compat: Map[String, String] = Map.empty + compat: Map[String, String] = Map.empty, + repeat: Int = 1 )(implicit loc: Location): Unit = { test(testOpt) { + assert(repeat > 0) val filename = "Hover.scala" val pkg = scala.meta.Term.Name(testOpt.name).syntax val noRange = original @@ -42,10 +44,14 @@ abstract class BaseHoverSuite } else { CompilerRangeParams(Paths.get(filename).toUri(), code, so, eo) } - val hover = presentationCompiler - .hover(pcParams) - .get() - .asScala + val hover = (0 to repeat) + .map(_ => + presentationCompiler + .hover(pcParams) + .get() + .asScala + ) + .last .map(_.toLsp()) val obtained: String = renderAsString(code, hover, includeRange) assertNoDiff( diff --git a/tests/cross/src/test/scala/tests/hover/HoverScala3TypeSuite.scala b/tests/cross/src/test/scala/tests/hover/HoverScala3TypeSuite.scala index 917da63e000..e4f6a81ceef 100644 --- a/tests/cross/src/test/scala/tests/hover/HoverScala3TypeSuite.scala +++ b/tests/cross/src/test/scala/tests/hover/HoverScala3TypeSuite.scala @@ -595,4 +595,21 @@ class HoverScala3TypeSuite extends BaseHoverSuite { |""".stripMargin ) + check( + "i6852", + """ + |type Foo = String + | + |/** some doc */ + |object Foo: + | /** doc doc */ + | val fo@@o = "" + |""".stripMargin, + """|```scala + |val foo: String + |``` + |doc doc""".stripMargin, + repeat = 2 + ) + } diff --git a/tests/cross/src/test/scala/tests/pc/SignatureHelpSuite.scala b/tests/cross/src/test/scala/tests/pc/SignatureHelpSuite.scala index 86df5391d32..f264e728305 100644 --- a/tests/cross/src/test/scala/tests/pc/SignatureHelpSuite.scala +++ b/tests/cross/src/test/scala/tests/pc/SignatureHelpSuite.scala @@ -520,7 +520,7 @@ class SignatureHelpSuite extends BaseSignatureHelpSuite { | | """.stripMargin, - """|apply(viewId: String, nodeUri: String, label: String, command: String = ..., icon: String = ..., tooltip: String = ..., collapseState: String = ...): TreeViewNode + """|apply(viewId: String, nodeUri: String, label: String, command: String = null, icon: String = null, tooltip: String = null, collapseState: String = null): TreeViewNode | ^^^^^^^^^^^^^^ |""".stripMargin, compat = Map(