🚥 Implement Document Highlight for all return / throws (function/closure/accessor/init/deinit) and break / continue (for/while/repeat...while) #2403
Conversation
…e/accessor/init/deinit) and break / continue (for/while/repeat...while)
lokesh-tr
changed the title
|
This is how it looks: 
|
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Very nice and excited for this. 😍
A preliminary review, as you already noted in the PR description, a few tests would be good 😉
| } | ||
| } | ||
|
|
||
| override func visit(_ node: BreakStmtSyntax) -> SyntaxVisitorContinueKind { |
There was a problem hiding this comment.
You shouldn’t need to repeat the list of control flow nodes if this is a SyntaxAnyVisitor and you add a visitAny override.
…look nicer for expressions spanning multiple lines
| } | ||
|
|
||
| private func addHighlightIfMatches(_ node: some SyntaxProtocol) { | ||
| if node.lookupControlStructure() == targetStructure { |
There was a problem hiding this comment.
This could be a guard to avoid the nesting level.
| if node.lookupControlStructure() == targetStructure { | ||
| highlights.append( | ||
| DocumentHighlight( | ||
| range: snapshot.absolutePositionRange(of: node.firstToken(viewMode: .sourceAccurate).map { $0.positionAfterSkippingLeadingTrivia..<$0.endPositionBeforeTrailingTrivia } ?? node.positionAfterSkippingLeadingTrivia..<node.endPositionBeforeTrailingTrivia), |
There was a problem hiding this comment.
Could you run swift-format on your changes?
| if node.lookupControlStructure() == targetStructure { | ||
| highlights.append( | ||
| DocumentHighlight( | ||
| range: snapshot.absolutePositionRange(of: node.firstToken(viewMode: .sourceAccurate).map { $0.positionAfterSkippingLeadingTrivia..<$0.endPositionBeforeTrailingTrivia } ?? node.positionAfterSkippingLeadingTrivia..<node.endPositionBeforeTrailingTrivia), |
There was a problem hiding this comment.
While currently the return/break etc keyword is always the first token in the syntax node, this may change in the future. I’d prefer if we explicitly pass in the token to highlight.
| if node.lookupControlStructure() == targetStructure { | ||
| highlights.append( | ||
| DocumentHighlight( | ||
| range: snapshot.absolutePositionRange(of: node.firstToken(viewMode: .sourceAccurate).map { $0.positionAfterSkippingLeadingTrivia..<$0.endPositionBeforeTrailingTrivia } ?? node.positionAfterSkippingLeadingTrivia..<node.endPositionBeforeTrailingTrivia), |
There was a problem hiding this comment.
Instead of $0.positionAfterSkippingLeadingTrivia..<$0.endPositionBeforeTrailingTrivia you should be able to do $0.trimmedRange or something like that.
2 participants
Upgrades
textDocument/documentHighlightto highlight various return / throw statements corresponding to a function / closure / accessor / init / deinit, and break / continue statements corresponding to a loop, thereby allowing us to not wonder about which statements could cause an exit from a function or loop.This also makes use of a new API introduced in SwiftLexicalLookup Library in Swift Syntax.
This doesn't break existing code. This is Additive.
Fixes: Highlight related identifiers for control flow keywords #2400
This is the original PR. New API for Swift Syntax introduced in 🚥 Introduce a new Lexical query
lookupControlStructureto theSwiftLexicalLookupLibrary swift-syntax#3216I don't think there's any risk associated with this new feature.
Tests and Docs are yet to be written. The code & This PR description will also be polished.
Suggested by @ahoppen and Open to be reviewed by anyone. Note: At the current state, the code is messy since I was focused on getting it to work, will polish it.