Implement literal detection for jump-to-definition functionality #2394
Conversation
Clemo97
requested review from
ahoppen,
hamishknight and
rintaro
as code owners
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Thanks for opening the PR. Looks like the right direction, I left a few comments inline.
| while let node = currentNode { | ||
| // Check all literal expression types | ||
| if node.is(StringLiteralExprSyntax.self) | ||
| || node.is(IntegerLiteralExprSyntax.self) | ||
| || node.is(FloatLiteralExprSyntax.self) | ||
| || node.is(BooleanLiteralExprSyntax.self) | ||
| || node.is(ArrayExprSyntax.self) | ||
| || node.is(DictionaryExprSyntax.self) | ||
| || node.is(NilLiteralExprSyntax.self) | ||
| { | ||
| return true | ||
| } | ||
| currentNode = node.parent | ||
| } | ||
| return false |
There was a problem hiding this comment.
This will also prevent all jump-to-definition functionality for code inside literals, eg. jump-to-definition will no longer work inside literals, eg. you couldn’t jump to the definition of foo anymore in "Value: \(foo)" or [foo]. Instead, what I think we should do, is to check if the tokenKind is stringSegment, integerLiteral or floatLiteral. Maybe you can find other token kinds as well that we should suppress as well, I didn’t walk through all of them.
| // Also check the token before the cursor position, as users might click right after a literal | ||
| if absolutePosition.utf8Offset > 0, | ||
| let tokenBefore = tree.token(at: AbsolutePosition(utf8Offset: absolutePosition.utf8Offset - 1)) | ||
| { | ||
| if isTokenInLiteral(tokenBefore) { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
Hmm, that’s quite unfortunate. I would prefer if we could find this previous token using token.previousToken(viewMode: .sourceAccurate) because it will not need to do a tree traversal to find the token again and should thus be more efficient. It likely won’t matter much in practice but still.
| // Check if the position is on a literal value (string, integer, etc.) | ||
| if await languageService.isPositionOnLiteral(req.position, in: req.textDocument.uri) { | ||
| return [] | ||
| } |
There was a problem hiding this comment.
I think we should do this check in SwiftLanguageService.symbolInfo. I think we want to suppress the jump-to-definition kind of behavior as well for all the callers of symbolInfo (implementation, definition, call hierarchy, type hierarchy, find references).
|
Thank you for the feedback @ahoppen. |
2 participants
Fixes #2368
This PR suppresses jump-to-definition for literal values (strings, integers, floats, booleans, arrays, dictionaries, and nil).
Implementation:
isPositionOnLiteral()method toLanguageServiceprotocol with default implementationSwiftLanguageServiceusing SwiftSyntax tree traversalindexBasedDefinition()to skip literalsTesting:
testDefinitionOnLiteralsShouldReturnEmpty()covering string, integer, boolean, and array literals