Incorrect goto definition for overloaded apply in another file

[ckipp01]

Describe the bug

This might be a result of the fix for #4255 and also might be a duplicate of #4484, but in the following scenario:

// Main.scala
object Main:
  List(1).map(Foo.apply)

// Foo.scala
case class Foo(a: Int, b: Int)

object Foo:
  def apply(a: Int): Foo = Foo(a, 1)

When you trigger a goto definition on the explicit apply in Main.scala it brings you to the case class Foo instead of the overloaded apply definition in object Foo

Expected behavior

In this scenario I'd expect it to bring you to def apply in object Foo.

NOTE that if they are in the same file, it works as expected. It's only a problem when they are in a different file.

Operating system

macOS

Editor/Extension

Nvim (nvim-metals)

Version of Metals

0.11.9+234-2d64131d-SNAPSHOT

Extra context or search terms

No response

[kasiaMarek]

case class Foo(a: Int, b: Int)

object Foo {
  def apply(a: Int): Foo = Foo(a, 1)
 }

In SemanticDB the synthetic apply is added in companion object on different position with regard to other methods in Scala 2 and 3:

• for Scala 3 it's added at the begging

_empty_/Foo.apply(). => method apply(a: Int, b: Int): Foo
_empty_/Foo.apply(+1). => method apply(a: Int): Foo

• for Scala 2 it's added at the end

_empty_/Foo.apply(). => method apply(a: Int): Foo
_empty_/Foo.apply(+1). => method apply(a: Int, b: Int): Foo

The indexing in ScalaMtags is compatible with Scala 2, thus we see THIS BUG in Scala 3.

I created a workaround in the linked PR for Scala 3 dialects, so the indexing is compatible with SemanticDB. However, this is insufficient and won't work for projects that use both Scala 2 and Scala 3.

We need an upstream-fix of the SementicDB inconsistency.

[tanishiking]

I can take a look at the SemanticDB part in Scala3 👍, but I don't have time this month, and going to check in March.

[tanishiking]

I will take a look :)

[tanishiking]

⚠️ The theory in this comment wasn't correct, skip it ⚠️

It looks like the problem is that Metals is looking for an apply symbol with the wrong disambiguator In Scala3.
So the problem might be a SemanticDB in call-site, instead of

// Main.scala
object Main extends App {
  List(1.toString).map(Foo.apply)
}

// Foo.scala
case class Foo(a: Int)

object Foo {
  def apply(a: String): Foo = Foo(a.toInt)
}

In this case, we should jump from Foo.appl@y to the def apply(a: String): Foo, but we jump to case class Foo(a: Int) (in Scala3).

In this setting, Scala3 generate the following SemanticDB, and it generates correct symbol (and occurrences) with right disambiguator.
Why we jump to wrong apply method then? My hunch is, Metals wrongly convert it for synthetic apply method (since Scala3 anyway generates Synthetics apply) here

metals/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala

Lines 262 to 276 in 784494b

	private def syntheticApplyOccurrence(
	queryPositionOpt: Option[semanticdb.Range],
	snapshot: TextDocument,
	) = {
	snapshot.synthetics.collectFirst {
	case Synthetic(
	Some(range),
	SelectTree(_: OriginalTree, Some(IdTree(symbol))),
	)
	if queryPositionOpt
	.exists(queryPos => range == queryPos) =>
	SymbolOccurrence(Some(range), symbol, SymbolOccurrence.Role.REFERENCE)
	}
	}

nevermind, this shouldn't be an issue since we combine the result.

# Main.scala
Occurrences:
...
[3:23..3:26) => hello/Foo.
[3:27..3:32) => hello/Foo.apply(+1).

Synthetics:
[3:2..3:6) => *.apply[String]
[3:2..3:22) => *[Foo]

# Foo.scala
Symbols:
...
hello/Foo.apply(). => method apply(a: Int): Foo
...
hello/Foo.apply(+1). => method apply(a: String): Foo

outdated

---

The wired thing is,

when we have

// Main.scala
object Main extends App {
  List(1).map(Foo.apply)
}

Foo.apply jump to def apply(a: String): Foo while it should jump to case class Foo(a: Int) 🤔

I'll keep taking a look

[tanishiking]

Ah, ok so the problem is indeed GlobalSymbolIndex#definitions returns
the wrong SymbolDefinition for the right Symbol string here

metals/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala

Line 467 in 784494b

val matched = definitions.find { defn =>

Indeed the mtags indexer seems the culprit 👀

Now, I think I'm on the same page as @kasiaMarek

[tanishiking]

What we should do is making the disambiguator compatible with mtags indexer and SemanticDB as @kasiaMarek tried to do in #4914

But, is it possible? Is disambiguators are determined syntactically? (otherwise, it's not possible to align mtags indexer and SemanticDB generated by compiler because former is based on parser, and latter knows the all semantic information).
According to the scalameta specification, it should be possible: they say

https://scalameta.org/docs/semanticdb/specification.html#symbol-1

Empty string for the definition that appears first.
+1 for the definition that appears second.
+2 for the definition that appears third.

So, if we follow the specification, we should fix the SemanticDB plugin for Scala2, instead of Scala3.

Of course, we should make sure Scala3 compiler, mtags indexers for all dialects properly follow the specifications around disambiguator.

[tgodzik]

So, if we follow the specification, we should fix the SemanticDB plugin for Scala2, instead of Scala3.

Of course, we should make sure Scala3 compiler, mtags indexers for all dialects properly follow the specifications around disambiguator.

I think it's not clear for synthetic applies, since it doesn't appear in the code at all. We should make that more clear in the definition and it might be easier to change the Scala 3 compiler instead since the Scala 2 behaviour might be usedalready in a number of places.

[kasiaMarek]

Works with 3.4.0-RC1-bin-20231223-938d405-NIGHTLY.