Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use alternative with stripped synthetic filename$package. #7000

Merged
merged 6 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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()
}
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
43 changes: 34 additions & 9 deletions mtags/src/main/scala/scala/meta/internal/metals/Docstrings.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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.
*
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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))
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ object DefinitionAlternatives {
*/
def apply(symbol: Symbol): List[Symbol] = {
List(
stripSyntheticPackageObjectFromObject(symbol),
caseClassCompanionToType(symbol),
caseClassApplyOrCopy(symbol),
caseClassApplyOrCopyParams(symbol),
Expand All @@ -26,6 +27,21 @@ object DefinitionAlternatives {
Some(sym.owner -> sym.value.desc)
}

private def stripSyntheticPackageObjectFromObject(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to validate this? Could there be an option where we have both object with the $package prefix and without?

And if we really need to we should have something like:

case class Alternative(private val symbol: Symbol, validation: (loc: SymbolLocation) => Boolean = true) {
  def symbol(loc: SymbolLocation) = {
   if (validation(loc)) Some(symbol)
   else None
  }
}

this way we make sure that Validation is not ignored at any point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can skip it... One would have to specifically create an object / package object called something$package (where I think that $ is generally reserved for the compiler) and still this is only a fallback...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's maybe skip it then.

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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
16 changes: 11 additions & 5 deletions tests/cross/src/main/scala/tests/pc/BaseHoverSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(
Expand Down
17 changes: 17 additions & 0 deletions tests/cross/src/test/scala/tests/hover/HoverScala3TypeSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

}
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading