-
Notifications
You must be signed in to change notification settings - Fork 472
🚥 Introduce a new Lexical query lookupControlStructure to the SwiftLexicalLookup Library
#3216
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
base: main
Are you sure you want to change the base?
Conversation
ahoppen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 😍 Very excited to get my hands on this.
The overall direction looks good to me. A couple things we should ensure:
- We handle labelled
breakstatements correctly, eg. the following. We should havelookupLabeledStmtsto help with this.
LOOP: while true {
switch x {
case 1: break LOOP
default: break
}
}- Double check that all syntax nodes are added correctly, I think subscripts are currently missing but there might be other.
|
CC @MAJKFL if you’re interested in helping review this and contributing your expertise in name lookup edge cases. 😉 |
rintaro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Overall, I think this is a great addition to SwiftLexicalLookup.
I know this PR is still in draft, but I left some comments anyway.
|
|
||
| extension SyntaxProtocol { | ||
|
|
||
| @_spi(Experimental) public func lookupControlStructure() -> Syntax? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel comfortable with having this method on SyntaxProtocol.
Instead, how about making
@_spi(Experimental) public protocol ControlTransferStmtSyntax {
var controlTransferTarget: Syntax? { get }
}And make break, continue, return, throw, and fallthrough conform to it?
| case .breakStmt, .continueStmt: | ||
| return lookupEnclosingControlStructure(for: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, break and continue target is not that simple. You'd need to handle labels e.g. break LOOP.
Also, they can target other non-loop labeled statements , e.g.
OUTER: do {
for val in values {
if val.condition {
break OUTER
}
}
}In the actual compiler, the rule is implemented in swift::findBreakOrContinueStmtTarget
| let syntax = Syntax(self) | ||
|
|
||
| switch syntax.as(SyntaxEnum.self) { | ||
| case .throwStmt, .returnStmt: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The target of throw can be do-catch statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, SyntaxProtoco.lookupCatchNode() should be usable for throw case.
Introduces a new Lexical query
lookupControlStructureto theSwiftLexicalLookupLibrary which answers "which function / closure / init / deinit / accessor gets terminated by this return / throw statement?", also answers "which loop does this break / continue statement correspond to?"Purely Additive. Shouldn't break anything. New API exposed under
@_spi(Experimental).This API will be used by SourceKit-LSP to fix (Highlight related identifiers for control flow keywords sourcekit-lsp#2400).
Link to the SourceKit-LSP PR: 🚥 Implement Document Highlight for all return / throws (function/closure/accessor/init/deinit) and break / continue (for/while/repeat...while) sourcekit-lsp#2403
No Real Risks to the best of my knowledge. This is my first contribution to SwiftSyntax, it's possible that I overlooked something. If that's the case, enlighten me :)
Tests and Docs are yet to be written. After some time, will polish this PR description as well.
Discussed with @ahoppen on some fun things that i can implement in sourcekit-lsp, he suggested this 😁. Open for anyone to review, the current version makes the feature work, might require some polishing.