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: (Scala3) Don't navigate to enclosing symbols on go-to-definition if cursor is not on symbol #3807

Merged
merged 1 commit into from Apr 15, 2022

Conversation

tanishiking
Copy link
Member

@tanishiking tanishiking commented Apr 8, 2022

#1707

go-to-definition-no-sym

Basically the same strategy as #3792
But this PR checks isOnName inside enclosingSymbols because this Metals-version of enclosingSymbols well deals with synthetic symbols like for-flatMap and caseclass-apply. We check isOnName only if it's not on synthetic symbols or special cases (e.g. NameArg and Import).

Maybe we can make hover (for Scala3) better using this Metals-version of enclosingSymbols and it handles the issue #3792 (comment)


Remained issues around go-to-definition and hover are

* ```
* @param path - path to the position given by `Interactive.pathTo`
*/
def isOnName(
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved from HoverProvider.scala

def contains(tree: Tree): Boolean = tree match
case select: Select =>
// using `nameSpan` as SourceTree for Select (especially symbolic-infix e.g. `::` of `1 :: Nil`) miscalculate positions
select.nameSpan.contains(sourcePos.span)
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this case

StdNames.nme.flatMap,
StdNames.nme.foreach
)
def isForSynthetic(gtree: Tree)(using Context): Boolean =
Copy link
Member Author

Choose a reason for hiding this comment

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

Scala3 version of

lazy val isForName: Set[Name] = Set[Name](
nme.map,
nme.withFilter,
nme.flatMap,
nme.foreach
)
def isForSynthetic(gtree: Tree): Boolean = {
def isForComprehensionSyntheticName(select: Select): Boolean = {
select.pos == select.qualifier.pos && isForName(select.name)
}
gtree match {
case Apply(fun, List(_: Function)) => isForSynthetic(fun)
case TypeApply(fun, _) => isForSynthetic(fun)
case gtree: Select if isForComprehensionSyntheticName(gtree) => true
case _ => false
}
}

@@ -429,4 +415,40 @@ class PcDefinitionSuite extends BasePcDefinitionSuite {
|""".stripMargin
)
)
check(
Copy link
Member Author

Choose a reason for hiding this comment

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

Copied from #3743

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM! @tanishiking could you rebase the changes?

@tanishiking
Copy link
Member Author

Thank you for reviewing! rebased, merging after CI passed.

@tanishiking tanishiking merged commit 3b169f9 into scalameta:main Apr 15, 2022
@tanishiking tanishiking deleted the no-goto-definition-on-nosym branch April 15, 2022 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants