Skip to content

[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

Merged
merged 13 commits into from
Jul 5, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ let package = Package(
.library(name: "SwiftSyntaxMacros", targets: ["SwiftSyntaxMacros"]),
.library(name: "SwiftSyntaxMacroExpansion", targets: ["SwiftSyntaxMacroExpansion"]),
.library(name: "SwiftSyntaxMacrosTestSupport", targets: ["SwiftSyntaxMacrosTestSupport"]),
.library(name: "SwiftLexicalScopes", targets: ["SwiftLexicalScopes"]),
Copy link
Member

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 this SwiftLexicalLookup? All of these operations are, essentially, lexical lookups for various kinds of things---lookups for declarations, labeled statements, where a fallthrough ends up---and most will have "lookup" in their names.

Copy link
Contributor Author

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 the break and continue lookup you’ve mentioned yesterday that is not necessarily part of ASTScope 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?

Copy link
Member

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.

.library(
name: "SwiftSyntaxMacrosGenericTestSupport",
targets: ["SwiftSyntaxMacrosGenericTestSupport"]
Expand Down Expand Up @@ -243,6 +244,18 @@ let package = Package(
]
),

// MARK: SwiftLexicalScopes

.target(
name: "SwiftLexicalScopes",
dependencies: ["SwiftSyntax"]
),

.testTarget(
name: "SwiftLexicalScopesTest",
dependencies: ["_SwiftSyntaxTestSupport", "SwiftLexicalScopes"]
),

// MARK: SwiftSyntaxMacrosGenericTestSupport

.target(
Expand Down
45 changes: 45 additions & 0 deletions Sources/SwiftLexicalScopes/LexicalScopes.swift
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 {

/// Given syntax node position, returns all available labeled statements.
public static func lookupLabeledStmts(at syntax: SyntaxProtocol)
-> [LabeledStmtSyntax]
{
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?
) {
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)
}
}
139 changes: 139 additions & 0 deletions Sources/SwiftLexicalScopes/Scope.swift
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? {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is effectively always returning a FileScope for the outermost SourceFileSyntax.

switch self.syntaxNodeType {
case is SourceFileSyntax.Type:
FileScope(syntax: self)
default:
parent?.scope
}
}
}

protocol Scope {
var parent: Scope? { get }

var sourceSyntax: SyntaxProtocol { get }

func getDeclarationFor(name: String, at syntax: SyntaxProtocol) -> Syntax?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should call this lookup and have it return an array of DeclSyntax nodes. It is possible to shadow declarations across Scopes in Swift, and I would expect a lookup in a nested scope to be able to see both declarations. That makes it the client's responsibility to overlay a tie-breaking policy (e.g. deepest decl wins)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 lookup and lookupWithShadowing.

Copy link
Member

Choose a reason for hiding this comment

The 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)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Scope protocol accordingly.

}

extension Scope {
var parent: Scope? {
getParentScope(forSyntax: sourceSyntax)
}

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]) {
Copy link
Member

Choose a reason for hiding this comment

The 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 inout array because there’s less state involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 inout actually improves performance. I find it generally helps to keep the code cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
  }
}

Copy link
Member

Choose a reason for hiding this comment

The 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?) {
Copy link
Member

Choose a reason for hiding this comment

The 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 fallthrough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s used by the compiler to check fallthrough correctness here

Copy link
Member

Choose a reason for hiding this comment

The 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)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have two similar implementations to this: ancestorOrSelf in SwiftParserDiagnostics and ancestorOrSelf in BasicFormat.

I think it would make sense to hoist that method up to the SwiftSyntax module and use it here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. I moved ancestorOrSelf to SyntaxProtocol.


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
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this currently doesn’t handle #if clauses at all. @DougGregor Have you thought about how lexical scopes should related to #if clauses. Because really, we need to evaluate the #if conditions to be able to tell which case the switch falls through to. The same issue also comes up for name lookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be possible to e.g. evaluate the #if clauses and then perform the lookup on the resulting source code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DougGregor has a PR up to evaluate #if in swift-syntax but I think it’s not quite ready yet. #1816

Let’s wait until Doug is back to discuss how we want to approach this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had in fact not considered how to handle #if clauses. I don't want to rely on any kind of syntactic preprocessing of the syntax tree to remove the #ifs: we need this query to work on the original, unmodified syntax tree.

So, it seems like we will need something like #1816 to have any chance of making lookupNextSwitchCase(at:) work when there's an #if around the case. When we do get #1816, we'll need to parameterize this operation by a BuildConfiguration. I'm okay with waiting until #1816 lands to make that change, and just... pick the first one or something... if there are #ifs around the following case.

}

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? {
guard let syntax else { return nil }

switch syntax.syntaxNodeType {
case is DoStmtSyntax.Type:
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't have the throwsClosure != nil check here: the catch node is always the function, regardless of whether it currently specifies that it throws or not. The type checker can figure out whether it should have been throws.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
}
}
}
29 changes: 29 additions & 0 deletions Sources/SwiftLexicalScopes/ScopeConcreteImplementations.swift
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FileScope isn't doing anything. I think it should be removed.

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
}
Copy link
Member

Choose a reason for hiding this comment

The 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 getDeclarationFor(name:at:) for a follow-up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I included the dummy lookup method to add assertLexicalNameLookup so that we already have a nice test harness for the future development of name lookup. If you think it’s out of scope of this PR I can move it to one of the future ones.

}
105 changes: 105 additions & 0 deletions Tests/SwiftLexicalScopesTest/Assertions.swift
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
Copy link
Member

Choose a reason for hiding this comment

The 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] })
)
}
Loading