-
Notifications
You must be signed in to change notification settings - Fork 439
[SwiftLexicalScopes][GSoC] Add simple scope queries and initial name lookup API structure #2696
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
Changes from 1 commit
9f2fab0
d4fb767
211cb6f
a00c922
db7fd99
74ddcc4
b85dced
8dddaa3
0dc27c9
0059f2f
08297ae
b4a9b3f
67e2608
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
ahoppen marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// This source file is part of the Swift.org open source project | ||
// | ||
// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors | ||
// Licensed under Apache License v2.0 with Runtime Library Exception | ||
// | ||
// See https://swift.org/LICENSE.txt for license information | ||
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
import Foundation | ||
import SwiftSyntax | ||
|
||
public struct LexicalScopes { | ||
MAJKFL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
MAJKFL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// Given syntax node position, returns all available labeled statements. | ||
public static func lookupLabeledStmts(at syntax: SyntaxProtocol) | ||
-> [LabeledStmtSyntax] | ||
MAJKFL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
guard let scope = syntax.scope else { return [] } | ||
return scope.lookupLabeledStmts(at: syntax) | ||
} | ||
|
||
/// Given syntax node position, returns the current switch case and it's fallthrough destination. | ||
public static func lookupFallthroughSourceAndDest(at syntax: SyntaxProtocol) -> ( | ||
SwitchCaseSyntax?, SwitchCaseSyntax? | ||
) { | ||
MAJKFL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
guard let scope = syntax.scope else { return (nil, nil) } | ||
return scope.lookupFallthroughSourceAndDest(at: syntax) | ||
} | ||
|
||
/// Given syntax node position, returns the closest ancestor catch node. | ||
public static func lookupCatchNode(at syntax: SyntaxProtocol) -> Syntax? { | ||
guard let scope = syntax.scope else { return nil } | ||
return scope.lookupCatchNode(at: Syntax(syntax)) | ||
} | ||
|
||
/// Given name and syntax node position, return referenced declaration. | ||
public static func lookupDeclarationFor(name: String, at syntax: SyntaxProtocol) -> Syntax? { | ||
guard let scope = syntax.scope else { return nil } | ||
return scope.getDeclarationFor(name: name, at: syntax) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// This source file is part of the Swift.org open source project | ||
// | ||
// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors | ||
// Licensed under Apache License v2.0 with Runtime Library Exception | ||
// | ||
// See https://swift.org/LICENSE.txt for license information | ||
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
import Foundation | ||
import SwiftSyntax | ||
|
||
extension SyntaxProtocol { | ||
/// Scope at the syntax node. Could be inherited from parent or introduced at the node. | ||
var scope: Scope? { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This property is effectively always returning a |
||
switch self.syntaxNodeType { | ||
case is SourceFileSyntax.Type: | ||
FileScope(syntax: self) | ||
default: | ||
parent?.scope | ||
} | ||
} | ||
} | ||
|
||
protocol Scope { | ||
MAJKFL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
var parent: Scope? { get } | ||
|
||
var sourceSyntax: SyntaxProtocol { get } | ||
|
||
func getDeclarationFor(name: String, at syntax: SyntaxProtocol) -> Syntax? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we should call this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion. I think it could be a great idea to include both of them, since when looking up with shadowing we could stop as soon as we find a name match adding a bit of optimization. Something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even without considering shadowing, it's possible to find multiple declarations that would need to be returned. This can happen with overloaded local functions, like this: func f() {
func g(x: Int) { }
func g(x: Double) { }
g(x: 3)
g(x: 3.14159)
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah, I haven’t thought about this case. You’re right, thanks for pointing this out. I adjusted the |
||
} | ||
|
||
extension Scope { | ||
var parent: Scope? { | ||
getParentScope(forSyntax: sourceSyntax) | ||
} | ||
ahoppen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private func getParentScope(forSyntax syntax: SyntaxProtocol?) -> Scope? { | ||
if let lookedUpScope = syntax?.scope, lookedUpScope.sourceSyntax.id == syntax?.id { | ||
return getParentScope(forSyntax: sourceSyntax.parent) | ||
} else { | ||
return syntax?.scope | ||
} | ||
} | ||
|
||
// MARK: - lookupLabeledStmts | ||
|
||
func lookupLabeledStmts(at syntax: SyntaxProtocol) -> [LabeledStmtSyntax] { | ||
var result = [LabeledStmtSyntax]() | ||
lookupLabeledStmtsHelper(at: syntax.parent, accumulator: &result) | ||
return result | ||
} | ||
|
||
private func lookupLabeledStmtsHelper(at syntax: Syntax?, accumulator: inout [LabeledStmtSyntax]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have always found return values to be easier to reason about than building up an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would you say about the altered implementation? I wanted to use the accumulator to possibly benefit from tail recursion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which altered implementation are you referring to? I generally try to start with the cleanest implementation and only optimize for performance if we see that the function is a performance bottleneck and can measure that eg. using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I should’ve linked change from the last commit I made. The altered solution builds up the accumulator and then, after finishing lookup, returns it as a result. private func lookupLabeledStmtsHelper(at syntax: Syntax?, accumulator: [LabeledStmtSyntax]) -> [LabeledStmtSyntax] {
guard let syntax, !syntax.is(MemberBlockSyntax.self) else { return accumulator }
if let labeledStmtSyntax = syntax.as(LabeledStmtSyntax.self) {
return lookupLabeledStmtsHelper(at: labeledStmtSyntax.parent, accumulator: accumulator + [labeledStmtSyntax])
} else {
return lookupLabeledStmtsHelper(at: syntax.parent, accumulator: accumulator)
}
} Do you suggest to further simplify it to just building up the return value? private func lookupLabeledStmtsHelper(at syntax: Syntax?) -> [LabeledStmtSyntax] {
guard let syntax, !syntax.is(MemberBlockSyntax.self) else { return [] }
if let labeledStmtSyntax = syntax.as(LabeledStmtSyntax.self) {
return [labeledStmtSyntax] + lookupLabeledStmtsHelper(at: labeledStmtSyntax.parent)
} else {
return lookupLabeledStmtsHelper(at: syntax.parent)
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I was suggesting to use the return value variant. Sometimes it’s just me but I find it easier to reason about. |
||
guard let syntax, !syntax.is(MemberBlockSyntax.self) else { return } | ||
if let labeledStmtSyntax = syntax.as(LabeledStmtSyntax.self) { | ||
accumulator.append(labeledStmtSyntax) | ||
lookupLabeledStmtsHelper(at: labeledStmtSyntax.parent, accumulator: &accumulator) | ||
} else { | ||
lookupLabeledStmtsHelper(at: syntax.parent, accumulator: &accumulator) | ||
} | ||
} | ||
|
||
// MARK: - lookupFallthroughSourceAndDest | ||
|
||
func lookupFallthroughSourceAndDest(at syntax: SyntaxProtocol) -> (SwitchCaseSyntax?, SwitchCaseSyntax?) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just out of curiosity: Why do we need the source case of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It’s used by the compiler to check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, thanks for pointing me to it. 👍🏽 |
||
guard let originalSwitchCase = lookupClosestSwitchCaseSyntaxAncestor(at: syntax) else { return (nil, nil) } | ||
|
||
let nextSwitchCase = lookupNextSwitchCase(at: originalSwitchCase) | ||
|
||
return (originalSwitchCase, nextSwitchCase) | ||
} | ||
|
||
private func lookupClosestSwitchCaseSyntaxAncestor(at syntax: SyntaxProtocol?) -> SwitchCaseSyntax? { | ||
guard let syntax else { return nil } | ||
|
||
if let switchCaseSyntax = syntax.as(SwitchCaseSyntax.self) { | ||
return switchCaseSyntax | ||
} else { | ||
return lookupClosestSwitchCaseSyntaxAncestor(at: syntax.parent) | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have two similar implementations to this: I think it would make sense to hoist that method up to the SwiftSyntax module and use it here as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing this out. I moved |
||
|
||
private func lookupNextSwitchCase(at switchCaseSyntax: SwitchCaseSyntax) -> SwitchCaseSyntax? { | ||
guard let switchCaseListSyntax = switchCaseSyntax.parent?.as(SwitchCaseListSyntax.self) else { return nil } | ||
|
||
var visitedOriginalCase = false | ||
|
||
for child in switchCaseListSyntax.children(viewMode: .sourceAccurate) { | ||
if let thisCase = child.as(SwitchCaseSyntax.self) { | ||
if thisCase.id == switchCaseSyntax.id { | ||
visitedOriginalCase = true | ||
} else if visitedOriginalCase { | ||
return thisCase | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this currently doesn’t handle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it would be possible to e.g. evaluate the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DougGregor has a PR up to evaluate Let’s wait until Doug is back to discuss how we want to approach this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had in fact not considered how to handle So, it seems like we will need something like #1816 to have any chance of making |
||
} | ||
|
||
return nil | ||
} | ||
|
||
// MARK: - lookupCatchNode | ||
|
||
func lookupCatchNode(at syntax: Syntax) -> Syntax? { | ||
return lookupCatchNodeHelper(at: syntax, traversedCatchClause: false) | ||
} | ||
|
||
private func lookupCatchNodeHelper(at syntax: Syntax?, traversedCatchClause: Bool) -> Syntax? { | ||
MAJKFL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
guard let syntax else { return nil } | ||
|
||
switch syntax.syntaxNodeType { | ||
case is DoStmtSyntax.Type: | ||
MAJKFL marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if traversedCatchClause { | ||
return lookupCatchNodeHelper(at: syntax.parent, traversedCatchClause: false) | ||
} else { | ||
return syntax | ||
} | ||
case is CatchClauseSyntax.Type: | ||
return lookupCatchNodeHelper(at: syntax.parent, traversedCatchClause: true) | ||
case is TryExprSyntax.Type: | ||
if syntax.as(TryExprSyntax.self)!.questionOrExclamationMark != nil { | ||
return syntax | ||
} else { | ||
return lookupCatchNodeHelper(at: syntax.parent, traversedCatchClause: traversedCatchClause) | ||
} | ||
case is FunctionDeclSyntax.Type: | ||
if syntax.as(FunctionDeclSyntax.self)!.signature.effectSpecifiers?.throwsClause != nil { | ||
return syntax | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for letting me know. I removed the condition and adjusted one of the tests. |
||
} else { | ||
return lookupCatchNodeHelper(at: syntax.parent, traversedCatchClause: traversedCatchClause) | ||
} | ||
default: | ||
return lookupCatchNodeHelper(at: syntax.parent, traversedCatchClause: traversedCatchClause) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// This source file is part of the Swift.org open source project | ||
// | ||
// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors | ||
// Licensed under Apache License v2.0 with Runtime Library Exception | ||
// | ||
// See https://swift.org/LICENSE.txt for license information | ||
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
import Foundation | ||
import SwiftSyntax | ||
|
||
class FileScope: Scope { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
var parent: Scope? = nil | ||
|
||
var sourceSyntax: SyntaxProtocol | ||
|
||
init(syntax: SyntaxProtocol) { | ||
self.sourceSyntax = syntax | ||
} | ||
|
||
func getDeclarationFor(name: String, at syntax: SyntaxProtocol) -> Syntax? { | ||
// TODO: Implement the method | ||
return nil | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we focus purely on the destination-kind queries in this PR and leave There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I included the dummy lookup method to add |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// This source file is part of the Swift.org open source project | ||
// | ||
// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors | ||
// Licensed under Apache License v2.0 with Runtime Library Exception | ||
// | ||
// See https://swift.org/LICENSE.txt for license information | ||
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
import Foundation | ||
import SwiftLexicalScopes | ||
import SwiftParser | ||
import SwiftSyntax | ||
import XCTest | ||
import _SwiftSyntaxTestSupport | ||
|
||
/// Parse `source` and check if the method passed as `methodUnderTest` produces the same results as indicated in `expected`. | ||
/// | ||
/// The `methodUnderTest` provides test inputs taken from the `expected` dictionary. The closure should return result produced by the tested method as an array with the same ordering. | ||
/// | ||
/// - Parameters: | ||
/// - methodUnderTest: Closure with the tested method. Provides test argument from `expected` to the tested function. Should return method result as an array. | ||
/// - expected: A dictionary with parameter markers as keys and expected results as marker arrays ordered as returned by the test method. | ||
func assertLexicalScopeQuery( | ||
source: String, | ||
methodUnderTest: (SyntaxProtocol) -> ([SyntaxProtocol?]), | ||
expected: [String: [String?]] | ||
) { | ||
// Extract markers | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is wonderful for writing tests, thank you! |
||
let (markerDict, textWithoutMarkers) = extractMarkers(source) | ||
|
||
// Parse the test source | ||
var parser = Parser(textWithoutMarkers) | ||
let sourceFileSyntax = SourceFileSyntax.parse(from: &parser) | ||
|
||
// Iterate through the expected results | ||
for (marker, expectedMarkers) in expected { | ||
// Extract a test argument | ||
guard let position = markerDict[marker], | ||
let testArgument = sourceFileSyntax.token(at: AbsolutePosition(utf8Offset: position)) | ||
else { | ||
XCTFail("Could not find token at location \(marker)") | ||
continue | ||
} | ||
|
||
// Execute the tested method | ||
let result = methodUnderTest(testArgument) | ||
|
||
// Extract the expected results for the test argument | ||
let expectedValues: [SyntaxProtocol?] = expectedMarkers.map { expectedMarker in | ||
guard let expectedMarker else { return nil } | ||
|
||
guard let expectedPosition = markerDict[expectedMarker], | ||
let expectedToken = sourceFileSyntax.token(at: AbsolutePosition(utf8Offset: expectedPosition)) | ||
else { | ||
XCTFail("Could not find token at location \(marker)") | ||
return nil | ||
} | ||
|
||
return expectedToken | ||
} | ||
|
||
// Compare number of actual results to the number of expected results | ||
if result.count != expectedValues.count { | ||
XCTFail( | ||
"For marker \(marker), actual number of elements: \(result.count) doesn't match the expected: \(expectedValues.count)" | ||
) | ||
} | ||
|
||
// Assert validity of the output | ||
for (actual, expected) in zip(result, expectedValues) { | ||
if actual == nil && expected == nil { continue } | ||
|
||
XCTAssert( | ||
actual?.firstToken(viewMode: .sourceAccurate)?.id == expected?.id, | ||
"For marker \(marker), actual result: \(actual?.firstToken(viewMode: .sourceAccurate) ?? "nil") doesn't match expected value: \(expected?.firstToken(viewMode: .sourceAccurate) ?? "nil")" | ||
) | ||
} | ||
} | ||
} | ||
|
||
/// Parse `source` and check if the lexical name lookup matches results passed as `expected`. | ||
/// | ||
/// - Parameters: | ||
/// - expected: A dictionary of markers with reference location as keys and expected declaration as values. | ||
func assertLexicalNameLookup( | ||
source: String, | ||
references: [String: String?] | ||
) { | ||
assertLexicalScopeQuery( | ||
source: source, | ||
methodUnderTest: { argument in | ||
// Extract reference name and use it for lookup | ||
guard let name = argument.firstToken(viewMode: .sourceAccurate)?.text else { | ||
XCTFail("Couldn't find a token at \(argument)") | ||
return [] | ||
} | ||
return [LexicalScopes.lookupDeclarationFor(name: name, at: argument)] | ||
}, | ||
expected: references.mapValues({ [$0] }) | ||
) | ||
} |
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.
We should talk about the name here a little more. I think I suggested
SwiftLexicalScopes
initially, but given that the "scope" part is actually getting somewhat downplayed... should we call thisSwiftLexicalLookup
? All of these operations are, essentially, lexical lookups for various kinds of things---lookups for declarations, labeled statements, where afallthrough
ends up---and most will have "lookup" in their names.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.
Yeah, I also like the idea. I changed it to
SwiftLexicalLookup
. It’s more broad and not limiting the library to just scope lookup could pave the way for including a broader range of APIs, like thebreak
andcontinue
lookup you’ve mentioned yesterday that is not necessarily part ofASTScope
as far as I remember. Maybe it would be a good idea to use this library in the long run to consolidate this sort of logic in one codebase?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.
Yes, I think this is the right place to consolidate all of the lexical lookup logic over time.