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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ff1ac2d
fix: use alternative with stripped synthetic `filename$package.`
kasiaMarek Dec 3, 2024
2a7e016
deduplicate docs for symbol and its companion
kasiaMarek Dec 4, 2024
104dff6
skip validation
kasiaMarek Dec 5, 2024
1b7a9a0
use proxy docs
kasiaMarek Dec 5, 2024
cf1d3e6
test: allow to repeat hover call to use cache in tests
kasiaMarek Dec 5, 2024
c33a642
refactor: remove redundant match
kasiaMarek Dec 6, 2024
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
2 changes: 1 addition & 1 deletion mtags/src/main/scala-2/scala/meta/internal/pc/CompletionItemResolver.scala
View file
Open in desktop
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
7 changes: 4 additions & 3 deletions mtags/src/main/scala-3/scala/meta/internal/mtags/MtagsEnrichments.scala
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion mtags/src/main/scala-3/scala/meta/internal/pc/CompletionItemResolver.scala
View file
Open in desktop
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
30 changes: 29 additions & 1 deletion mtags/src/main/scala/scala/meta/internal/metals/Docstrings.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import scala.meta.pc.SymbolDocumentation
*/
class Docstrings(index: GlobalSymbolIndex) {
val cache = new TrieMap[Content, SymbolDocumentation]()
val alternativesMap = new TrieMap[String, Set[String]]
private val logger = Logger.getLogger(classOf[Docstrings].getName)

def documentation(
Expand Down Expand Up @@ -130,6 +131,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 +140,29 @@ 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.contains(queryContent)) {
cache
.get(Content.from(defSymbol, contentType))
.foreach(cache.put(queryContent, _))

alternativesMap.synchronized {
tgodzik marked this conversation as resolved.
Show resolved Hide resolved
val prev = alternativesMap.get(defSymbol)
alternativesMap.put(
defSymbol,
prev.map(_ + querySymbol).getOrElse(Set(querySymbol))
)
}
}
}

private def indexSymbolDefinition(
defn: SymbolDefinition,
contentType: ContentType
Expand Down Expand Up @@ -166,7 +191,10 @@ class Docstrings(index: GlobalSymbolIndex) {
): Unit = {
for {
contentType <- ContentType.values()
} cache.remove(Content.from(occ.symbol, contentType))
symbol <- alternativesMap
.remove(occ.symbol)
.getOrElse(Set.empty) + occ.symbol
} cache.remove(Content.from(symbol, contentType))
}
}

Expand Down
44 changes: 34 additions & 10 deletions mtags/src/main/scala/scala/meta/internal/mtags/DefinitionAlternatives.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,25 @@
package scala.meta.internal.mtags

import scala.meta.internal.mtags.MtagsEnrichments._
import scala.meta.internal.semanticdb.Scala._

object DefinitionAlternatives {
type ValidateLocation = SymbolLocation => Boolean

/**
* Returns a list of fallback symbols that can act instead of given symbol.
*/
def apply(symbol: Symbol): List[Symbol] = {
List(
caseClassCompanionToType(symbol),
caseClassApplyOrCopy(symbol),
caseClassApplyOrCopyParams(symbol),
initParamToValue(symbol),
varGetter(symbol),
methodOwner(symbol),
objectInsteadOfAny(symbol)
).flatten
def apply(symbol: Symbol): List[(Symbol, Option[ValidateLocation])] = {
stripSyntheticPackageObjectFromObject(symbol).toList ++
List(
caseClassCompanionToType(symbol),
caseClassApplyOrCopy(symbol),
caseClassApplyOrCopyParams(symbol),
initParamToValue(symbol),
varGetter(symbol),
methodOwner(symbol),
objectInsteadOfAny(symbol)
).flatten.map((_, None))
}

object GlobalSymbol {
Expand All @@ -26,6 +29,27 @@ 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, Option[ValidateLocation])] = {
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,
Some(loc => s"${loc.path.scalaFileName}$$package" == pkgName)
)
)
} 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
3 changes: 3 additions & 0 deletions mtags/src/main/scala/scala/meta/internal/mtags/ScalametaCommonEnrichments.scala
View file
Open in desktop
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
32 changes: 20 additions & 12 deletions mtags/src/main/scala/scala/meta/internal/mtags/SymbolIndexBucket.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import scala.meta.Dialect
import scala.meta.internal.io.FileIO
import scala.meta.internal.io.PathIO
import scala.meta.internal.io.PlatformFileIO
import scala.meta.internal.mtags.DefinitionAlternatives.ValidateLocation
import scala.meta.internal.mtags.ScalametaCommonEnrichments._
import scala.meta.internal.semanticdb.Scala._
import scala.meta.internal.{semanticdb => s}
Expand Down Expand Up @@ -190,7 +191,8 @@ class SymbolIndexBucket(
*/
private def query0(
querySymbol: Symbol,
symbol: Symbol
symbol: Symbol,
validate: Option[ValidateLocation] = None
): List[SymbolDefinition] = {

removeOldEntries(symbol)
Expand Down Expand Up @@ -224,21 +226,27 @@ class SymbolIndexBucket(
if (!definitions.contains(symbol.value)) {
// Fallback 3: guess related symbols from the enclosing class.
DefinitionAlternatives(symbol)
.flatMap(alternative => query0(querySymbol, alternative))
.flatMap { case (alternative, validate) =>
query0(querySymbol, alternative, validate)
}
} else {
definitions
.get(symbol.value)
.map { paths =>
paths.map { location =>
SymbolDefinition(
querySymbol = querySymbol,
definitionSymbol = symbol,
path = location.path,
dialect = dialect,
range = location.range,
kind = None,
properties = 0
)
paths.flatMap { location =>
if (validate.forall(_(location))) {
Some(
SymbolDefinition(
querySymbol = querySymbol,
definitionSymbol = symbol,
path = location.path,
dialect = dialect,
range = location.range,
kind = None,
properties = 0
)
)
} else None
}.toList
}
.getOrElse(List.empty)
Expand Down
16 changes: 16 additions & 0 deletions tests/cross/src/test/scala/tests/hover/HoverScala3TypeSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,4 +595,20 @@ 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
)

}
2 changes: 1 addition & 1 deletion tests/cross/src/test/scala/tests/pc/SignatureHelpSuite.scala
View file
Open in desktop
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