enhancement: Refine jump-to-definition for literals #2404
Conversation
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
This looks great. Just a few comments to clean up the code slightly.
| /// Literal value tokens do not support jump-to-definition because they represent | ||
| /// concrete values written directly in source code and therefore have no | ||
| /// meaningful definition location. | ||
| func isPositionOnLiteralToken(_ position: Position, in uri: DocumentURI) async -> Bool |
There was a problem hiding this comment.
I don’t think we need this as a requirement in LanguageService at all if we are only calling it from SwiftLanguageService.symbolInfo, right?
| case .stringSegment, | ||
| .integerLiteral, | ||
| .floatLiteral: | ||
| return true | ||
| case .keyword(let keyword) where keyword == .true || keyword == .false || keyword == .nil: | ||
| return true |
There was a problem hiding this comment.
I think this can be simplified slightly
| case .stringSegment, | |
| .integerLiteral, | |
| .floatLiteral: | |
| return true | |
| case .keyword(let keyword) where keyword == .true || keyword == .false || keyword == .nil: | |
| return true | |
| case .stringSegment, | |
| .integerLiteral, | |
| .floatLiteral, | |
| .keyword(.true), .keyword(.false), .keyword(.nil):: | |
| return true |
| XCTAssertEqual(response?.locations?.map(\.uri), [try project.uri(for: "Test.h")]) | ||
| } | ||
|
|
||
| func testDefinitionOnLiteralsShouldReturnEmpty() async throws { |
There was a problem hiding this comment.
Could you run swift-format on your changes? https://github.com/swiftlang/sourcekit-lsp/blob/main/CONTRIBUTING.md#formatting
| let positions = testClient.openDocument( | ||
| """ | ||
| let name = "Ayman" | ||
| let greeting = "Hello \\(1️⃣name)" |
There was a problem hiding this comment.
If you start the string literal with #""" you don’t need the double backslash here.
|
I just realized that we have two open PRs open for this from @Clemo97 (#2394) and you, @a7maad-ayman, (#2404). First of all, thanks to both of you for working on this issue 🙏🏽. Given that both PRs contain (nearly) the same contents, you have already converged on a solution. To consolidate the two PRs, I would suggest that:
Would that work for both of you? |
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Nice. Looks good to me. Just one more request: Could you add @Clemo97 as a co-author of this PR by squashing your commits and adding Co-Authored-By: clemo97 <[email protected]> to the commit message so both of you are credited properly?
|
I’ve squashed the commits and added the Co-Authored-By line for @Clemo97 so we’re both properly credited. Thanks again for the review and for pointing that out — much appreciated. 
|
|
Looks like the git reset 7495f5532fdb17184d69518f46a207e596b26c64
# Commit your changes, including Co-Authored-By in the last line of the commit
git push -f |
Return empty response when cursor is on literal tokens (strings, integers, floats, booleans, nil) since they have no declaration. Resolves swiftlang#2368 Co-Authored-By: clemo97 <[email protected]>
|
Sorry for not triggering a test run when you pushed your changes. There’s a merge conflict now, could you rebase your changes on top of the latest |
2 participants
This PR refines jump-to-definition behavior by excluding literal values from definition lookups.
Problem
Currently, invoking jump-to-definition on literal values (strings, integers, floats, booleans,
nil) triggers a definition lookup that returns no meaningful result, since literals are concrete values written directly in source code with no declaration to navigate to.Solution
Detect when the cursor is positioned on a literal token and return an empty response early, avoiding unnecessary SourceKit queries. This aligns with how SourceKit handles non-symbol tokens.
Changes
SwiftLanguageServiceusing SwiftSyntaxsymbolInfoResolves #2368
Co-Authored-By: clemo97 [email protected]