Go to definition of synthetic symbols. #3683
Conversation
Previously, Metals didn't go the definition of symbols that the presentation compiler defines as synthetic. These symbols include auto-generated case class `apply` methods and regular class constructors. Now, we ignore the `isSynthetic` bit on the definition symbol so that "Go to definition" works as expected for cases where it didn't before.
This should be OK since the definition is at the same location as the call-site.
|
I tried fixing the failing Scala 3 tests but I'm not familiar with that part of the codebase. I'd appreciate any help to get the Scala 3 tests green |
There was a problem hiding this comment.
Scala2 part looks good 👍
In terms of Scala3, it looks like enclosingSymbols is the root cause, as all the inputs (of failing tests) are swallowed by this match case.
If I remove that case, I got the following test result for eta-2, which is still different from the expected result 🤔
=> Obtained
"""|object Main {
| List(1).foldLeft(0)(_ + /*scala/Int#`+`(+4). Int.scala*/_)
|}
|""".stripMargin
=> Diff (- obtained, + expected)
object Main {
- List(1).foldLeft(0)(_ + /*scala/Int#`+`(+4). Int.scala*/_)
+ List(1).foldLeft(0)(_ + <<>>_)
}
at munit.FunSuite.assertNoDiff(FunSuite.scala:11)
at tests.pc.BasePcDefinitionSuite.check$$anonfun$1(BasePcDefinitionSuite.scala:68)I guess this is because https://github.com/tanishiking/metals/blob/7871d17c7ea19d9b54c5af6025d242d7e5f5a0e1/mtags/src/main/scala-3/scala/meta/internal/pc/PcDefinitionProvider.scala#L142 ignores the Synthetic symbol, maybe we can fix this by taking care of the Synthetic symbols there.
|
I partly reviewed the PR, but I don't have time to take a look for a few days. If someone can delve into the problem with Scala3, please go for it :) |
|
@tanishiking Thank you for the review! I added |
|
@olafurpg I made a pr onto your branch with fixes for scala3 - olafurpg#4 |
|
Thank you @dos65 ! I merged your branch and updated this PR |
There was a problem hiding this comment.
I checked @dos65 's PR: olafurpg#4 and now it's LGTM!
(except one nitpick comment)
mtags/src/main/scala-3/scala/meta/internal/pc/PcDefinitionProvider.scala
Outdated
Show resolved
Hide resolved
| case (head @ Select(target, name)) :: _ | ||
| if head.symbol.is(Synthetic) && name == StdNames.nme.apply => | ||
| val sym = target.symbol |
There was a problem hiding this comment.
Could you add a comment which case is handled by this match for future us?
There was a problem hiding this comment.
it's for synthetic apply method in case-class
Previously, Metals didn't go the definition of symbols that the
presentation compiler defines as synthetic. These symbols include
auto-generated case class
applymethods and regular classconstructors. Now, we ignore the
isSyntheticbit on the definitionsymbol so that "Go to definition" works as expected for cases where it
didn't before.