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: show zero extent references when using pc #6583

Merged
merged 1 commit into from
Jul 15, 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
Expand Up @@ -791,6 +791,7 @@ class MetalsGlobal(
*/
def namePosition: Position = {
sel match {
case _ if !sel.pos.isRange => sel.pos
case Select(qualifier: Select, name)
if (name == nme.apply || name == nme.unapply) && sel.pos.point == qualifier.pos.point =>
qualifier.namePosition
Expand Down
17 changes: 11 additions & 6 deletions mtags/src/main/scala-2/scala/meta/internal/pc/PcCollector.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import scala.meta.pc.VirtualFileParams
trait PcCollector[T] { self: WithCompilationUnit =>
import compiler._

protected def allowZeroExtentImplicits = false

def collect(
parent: Option[Tree]
)(tree: Tree, pos: Position, sym: Option[Symbol]): T
Expand Down Expand Up @@ -87,6 +89,9 @@ trait PcCollector[T] { self: WithCompilationUnit =>
traverseSought(noTreeFilter, noSoughtFilter)
}

def isCorrectPos(t: Tree): Boolean =
t.pos.isRange || (t.pos.isDefined && allowZeroExtentImplicits && t.symbol.isImplicit)

def traverseSought(
filter: Tree => Boolean,
soughtFilter: (Symbol => Boolean) => Boolean
Expand All @@ -107,7 +112,7 @@ trait PcCollector[T] { self: WithCompilationUnit =>
* All indentifiers such as:
* val a = <<b>>
*/
case ident: Ident if ident.pos.isRange && filter(ident) =>
case ident: Ident if isCorrectPos(ident) && filter(ident) =>
if (ident.symbol == NoSymbol)
acc + collect(ident, ident.pos, fallbackSymbol(ident.name, pos))
else
Expand All @@ -118,7 +123,7 @@ trait PcCollector[T] { self: WithCompilationUnit =>
* type A = [<<b>>]
*/
case tpe: TypeTree
if tpe.pos.isRange && tpe.original != null && filter(tpe) =>
if isCorrectPos(tpe) && tpe.original != null && filter(tpe) =>
tpe.original.children.foldLeft(
acc + collect(
tpe.original,
Expand All @@ -130,7 +135,7 @@ trait PcCollector[T] { self: WithCompilationUnit =>
* All select statements such as:
* val a = hello.<<b>>
*/
case sel: Select if sel.pos.isRange && filter(sel) =>
case sel: Select if isCorrectPos(sel) && filter(sel) =>
val newAcc =
if (isForComprehensionMethod(sel)) acc
else acc + collect(sel, sel.namePosition)
Expand All @@ -146,7 +151,7 @@ trait PcCollector[T] { self: WithCompilationUnit =>
*/
case Function(params, body) =>
val newAcc = params
.filter(vd => vd.pos.isRange && filter(vd))
.filter(vd => isCorrectPos(vd) && filter(vd))
.foldLeft(
acc
) { case (acc, vd) =>
Expand All @@ -167,7 +172,7 @@ trait PcCollector[T] { self: WithCompilationUnit =>
* class <<Foo>> = ???
* etc.
*/
case df: MemberDef if df.pos.isRange && filter(df) =>
case df: MemberDef if isCorrectPos(df) && filter(df) =>
(annotationChildren(df) ++ df.children).foldLeft({
val t = collect(
df,
Expand Down Expand Up @@ -277,7 +282,7 @@ trait PcCollector[T] { self: WithCompilationUnit =>
res
// catch all missed named trees
case name: NameTree
if soughtFilter(_ == name.symbol) && name.pos.isRange =>
if soughtFilter(_ == name.symbol) && isCorrectPos(name) =>
tree.children.foldLeft(
acc + collect(
name,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ trait PcReferencesProvider {
import compiler._
protected def includeDefinition: Boolean
protected def result(): List[(String, Option[l.Range])]
override def allowZeroExtentImplicits = true

def collect(
parent: Option[Tree]
Expand All @@ -27,7 +28,10 @@ trait PcReferencesProvider {
case t: DefTree if !includeDefinition =>
(compiler.semanticdbSymbol(sym.getOrElse(t.symbol)), None)
case t =>
(compiler.semanticdbSymbol(sym.getOrElse(t.symbol)), Some(pos.toLsp))
(
compiler.semanticdbSymbol(sym.getOrElse(t.symbol)),
Some(pos.toLsp)
)
}
}

Expand Down
12 changes: 9 additions & 3 deletions mtags/src/main/scala-3/scala/meta/internal/pc/PcCollector.scala
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import dotty.tools.dotc.util.Spans.Span
trait PcCollector[T]:
self: WithCompilationUnit =>

def allowZeroExtentImplicits: Boolean = false

def collect(
parent: Option[Tree]
)(tree: Tree | EndMarker, pos: SourcePosition, symbol: Option[Symbol]): T
Expand Down Expand Up @@ -81,6 +83,10 @@ trait PcCollector[T]:
def isCorrect =
!span.isZeroExtent && span.exists && span.start < sourceText.size && span.end <= sourceText.size

extension (tree: Tree)
def isCorrectSpan =
tree.span.isCorrect || (allowZeroExtentImplicits && tree.symbol.is(Flags.Implicit))

def traverseSought(
filter: Tree => Boolean,
soughtFilter: (Symbol => Boolean) => Boolean,
Expand All @@ -102,7 +108,7 @@ trait PcCollector[T]:
* val a = <<b>>
*/
case ident: Ident
if ident.span.isCorrect && filter(ident) && !isExtensionMethodCall(
if ident.isCorrectSpan && filter(ident) && !isExtensionMethodCall(
parent,
ident.symbol,
) =>
Expand All @@ -121,7 +127,7 @@ trait PcCollector[T]:
* val x = new <<A>>(1)
*/
case sel @ Select(New(t), _)
if sel.span.isCorrect &&
if sel.isCorrectSpan &&
sel.symbol.isConstructor &&
t.symbol == NoSymbol =>
if soughtFilter(_ == sel.symbol.owner) then
Expand All @@ -136,7 +142,7 @@ trait PcCollector[T]:
* val a = hello.<<b>>
*/
case sel: Select
if sel.span.isCorrect && filter(sel) &&
if sel.isCorrectSpan && filter(sel) &&
!sel.isForComprehensionMethod =>
occurences + collect(
sel,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class PcReferencesProvider(
driver: InteractiveDriver,
request: ReferencesRequest,
) extends WithCompilationUnit(driver, request.file()) with PcCollector[Option[(String, Option[lsp4j.Range])]]:
override def allowZeroExtentImplicits: Boolean = true

private def soughtSymbols =
if(request.offsetOrSymbol().isLeft()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ class BaseDocumentHighlightSuite extends BasePCSuite with RangeReplace {
def check(
name: TestOptions,
original: String
// compat: Map[String, String] = Map.empty
)(implicit location: Location): Unit =
test(name) {

Expand Down
135 changes: 135 additions & 0 deletions tests/cross/src/test/scala/tests/pc/PcReferencesSuite.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
package tests.pc

import java.net.URI

import scala.meta.internal.jdk.CollectionConverters._
import scala.meta.internal.metals.CompilerVirtualFileParams
import scala.meta.internal.metals.EmptyCancelToken
import scala.meta.internal.pc.PcReferencesRequest

import munit.Location
import munit.TestOptions
import org.eclipse.lsp4j.jsonrpc.messages.{Either => JEither}
import tests.BasePCSuite
import tests.RangeReplace

class PcReferencesSuite extends BasePCSuite with RangeReplace {
def check(
name: TestOptions,
original: String,
compat: Map[String, String] = Map.empty
)(implicit location: Location): Unit =
test(name) {
val edit = original.replaceAll("(<<|>>)", "")
val expected = original.replaceAll("@@", "")
val base = original.replaceAll("(<<|>>|@@)", "")

val (code, offset) = params(edit, "Highlight.scala")
val ranges = presentationCompiler
.references(
PcReferencesRequest(
CompilerVirtualFileParams(
URI.create("file:/Highlight.scala"),
code,
EmptyCancelToken
),
includeDefinition = false,
offsetOrSymbol = JEither.forLeft(offset)
)
)
.get()
.asScala
.flatMap(_.locations().asScala.map(_.getRange()))
.toList

assertEquals(
renderRangesAsString(base, ranges),
getExpected(expected, compat, scalaVersion)
)
}

check(
"implicit-args",
"""|package example
|
|class Bar(i: Int)
|
|object Hello {
| def m(i: Int)(implicit b: Bar) = ???
| val foo = {
| implicit val b@@arr: Bar = new Bar(1)
| m<<>>(3)
| }
|}
|""".stripMargin,
compat = Map("3" -> """|package example
|
|class Bar(i: Int)
|
|object Hello {
| def m(i: Int)(implicit b: Bar) = ???
| val foo = {
| implicit val barr: Bar = new Bar(1)
| m(3)<<>>
| }
|}
|""".stripMargin)
)

check(
"implicit-args-2",
"""|package example
|
|class Bar(i: Int)
|class Foo(implicit b: Bar)
|
|object Hello {
| implicit val b@@arr: Bar = new Bar(1)
| val foo = <<>>new Foo
|}
|""".stripMargin,
compat = Map(
"3" -> """|package example
|
|class Bar(i: Int)
|class Foo(implicit b: Bar)
|
|object Hello {
| implicit val barr: Bar = new Bar(1)
| val foo = new Foo<<>>
|}
|""".stripMargin
)
)

// for Scala 3 the symbol in bar reference is missing (<none>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we reprort an issue upstrem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't compile for Scala 3. I think I confused myself by a combination of VirtusLab/scala-cli#2638 and VirtusLab/scala-cli#2226 (or something similar) 😂

check(
"implicit-args-3".tag(IgnoreScala3),
"""|package example
|
|class Bar(i: Int)
|class Foo(implicit b: Bar)
|
|object Hello {
| implicit val b@@arr = new Bar(1)
| for {
| _ <- Some(1)
| foo = <<>>new Foo()
| } yield ()
|}
|""".stripMargin
)

check(
"case-class",
"""|case class Ma@@in(i: Int)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add these two tests with no usages? Is this on puspose?

Copy link
Contributor Author

@kasiaMarek kasiaMarek Jul 15, 2024

Choose a reason for hiding this comment

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

Initially I added all zero-extent symbols, not only implicits, so it showed some additional refs in those tests, that shouldn't be there.

|""".stripMargin
)

check(
"case-class-with-implicit",
""""|case class A()(implicit val fo@@o: Int)
|""".stripMargin
)

}
Loading