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 goto definition leading to case class for overloaded apply in another file #4795

Closed

Conversation

MaciejG604
Copy link
Collaborator

Fixes #4769

@MaciejG604 MaciejG604 changed the title Bug/goto apply definition Fix goto definition leading to case class for overloaded apply in another file Jan 2, 2023
@ckipp01 ckipp01 added bug Something that is making a piece of functionality unusable navigation Related to goto definition, find references, open symbol labels Jan 2, 2023
@MaciejG604
Copy link
Collaborator Author

The current state of debugging this issue:

  • the issue is present only in Scala 3
  • there's a discrepancy between method symbol that we fetch from SemanticDB or PresentationCompiler and the state of GlobalSymbolIndex, SemanticDB/PR tells us that the symbol of the overloading apply is e.g. _empty_/Foo.apply(+1). and the GlobalSymbolIndex only contains the symbol _empty_/Foo.apply().
    The example project I work on looks like this:
// Main.scala
object Main {
  List(1).map(Foo.apply)
}

SemanticDB occurences of the Main.scala:

Occurrences:
[1:7..1:11) <= _empty_/Main.
[2:2..2:6) => scala/package.List.
[2:10..2:13) => scala/collection/immutable/List#map().
[2:14..2:17) => _empty_/Foo.
[2:18..2:23) => _empty_/Foo.apply(+1).
// Foo.scala
case class Foo(a: Int, b: Int)

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

SemanticDB symbols (jsut the relevant part of them) of the Foo.scala:

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

Contents of the relevant bucket of GlobalSymbolIndex:
image

Notice that in the index there's no apply(+1) the one that is overloading the default case class apply has symbol apply().

Copy link
Member

@kpodsiad kpodsiad left a comment

Choose a reason for hiding this comment

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

Hey @MaciejG604, I will just leave a couple of suggestions which may improve your work loop (they surely were a gamechanger to me!) I think that https://scalameta.org/metals/docs/contributors/getting-started should be a bit easier for newcomers to start, but in this comment I want to focus on that section. What works for me is:

  • find places which interest me, that's where I'll put pprint.log(...) calls
  • find unit test associated with given feature (you already found it)
  • place .only after one test, should be relatively easy example. This will make sure that munit will run only only this test from the whole suite
  • start sbt session and then within it run ~unit/testOnly test.DefinitionLspSuite

This will rerun this session after each saved change. Surely debugger is useful when one isn't familiar with codebase, but after some time I really liked pprint way ;)


Bonus:
In my ~/.sbt/1.0/build.sbt file I have

commands += Command.command("cls") { state =>
  print("\033c")
  state
}

then cls in sbt clear my console. I really like to use this like ~; cls; unit/testOnly test.DefinitionLspSuite

cc: @jkciesluk maybe you'll find this useful too!

Comment on lines +64 to +65
MetalsLogger.setupLspLogger(workspace, redirectSystemStreams = true)

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary to make logging working, is it?

Comment on lines +87 to +89
scribe.info(
s"DefinitionProvider.definition looking for definition in snapshot"
)
Copy link
Member

Choose a reason for hiding this comment

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

You may find pprint.log more appealing rather than scribe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that is making a piece of functionality unusable navigation Related to goto definition, find references, open symbol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect goto definition for overloaded apply in another file
4 participants