Skip to content

[SwiftLexicalLookup] Add support for #if and macro declarations. More ASTScope related fixes. #2883

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 2 commits into from
Nov 13, 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
2 changes: 1 addition & 1 deletion Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ let package = Package(

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

.testTarget(
Expand Down
3 changes: 2 additions & 1 deletion Sources/SwiftLexicalLookup/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,6 @@ add_swift_syntax_library(SwiftLexicalLookup
)

target_link_swift_syntax_libraries(SwiftLexicalLookup PUBLIC
SwiftSyntax)
SwiftSyntax
SwiftIfConfig)

7 changes: 6 additions & 1 deletion Sources/SwiftLexicalLookup/LookupConfig.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
//
//===----------------------------------------------------------------------===//

import SwiftIfConfig

@_spi(Experimental) public struct LookupConfig {
/// Specifies whether lookup should finish in the closest sequential scope.
///
Expand All @@ -31,14 +33,17 @@
/// If `finishInSequentialScope` would be set to `false`, the only name
/// returned by lookup would be the `a` declaration from inside function body.
@_spi(Experimental) public var finishInSequentialScope: Bool
@_spi(Experimental) public var buildConfiguration: BuildConfiguration?

/// Creates a new lookup configuration.
///
/// - `finishInSequentialScope` - specifies whether lookup should finish
/// in the closest sequential scope. `false` by default.
@_spi(Experimental) public init(
finishInSequentialScope: Bool = false
finishInSequentialScope: Bool = false,
buildConfiguration: BuildConfiguration? = nil
Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth taking in a ConfiguredRegions here rather than a BuildConfiguration. ConfiguredRegions caches results so we aren't re-evaluating the same #ifs repeatedly.

) {
self.finishInSequentialScope = finishInSequentialScope
self.buildConfiguration = buildConfiguration
}
}
16 changes: 13 additions & 3 deletions Sources/SwiftLexicalLookup/LookupName.swift
Original file line number Diff line number Diff line change
Expand Up @@ -104,20 +104,26 @@ import SwiftSyntax
case .variableDecl(let variableDecl):
return variableDecl.bindings.first?.accessorBlock?.positionAfterSkippingLeadingTrivia
?? variableDecl.endPosition
case .accessorDecl(let accessorDecl):
return accessorDecl.accessorSpecifier.positionAfterSkippingLeadingTrivia
case .deinitializerDecl(let deinitializerDecl):
return deinitializerDecl.deinitKeyword.positionAfterSkippingLeadingTrivia
default:
return declSyntax.positionAfterSkippingLeadingTrivia
}
case .Self(let declSyntax):
switch Syntax(declSyntax).as(SyntaxEnum.self) {
case .protocolDecl(let protocolDecl):
return protocolDecl.name.positionAfterSkippingLeadingTrivia
case .extensionDecl(let extensionDecl):
return extensionDecl.extensionKeyword.positionAfterSkippingLeadingTrivia
default:
return declSyntax.positionAfterSkippingLeadingTrivia
}
case .newValue(let accessorDecl), .oldValue(let accessorDecl):
return accessorDecl.accessorSpecifier.positionAfterSkippingLeadingTrivia
case .error(let catchClause):
return catchClause.catchItems.positionAfterSkippingLeadingTrivia
default:
return syntax.positionAfterSkippingLeadingTrivia
}
}
}
Expand Down Expand Up @@ -276,7 +282,11 @@ import SwiftSyntax
identifiable: IdentifiableSyntax,
accessibleAfter: AbsolutePosition? = nil
) -> [LookupName] {
[.identifier(identifiable, accessibleAfter: accessibleAfter)]
if case .wildcard = identifiable.identifier.tokenKind {
return []
}

return [.identifier(identifiable, accessibleAfter: accessibleAfter)]
}

/// Extracts name introduced by `NamedDeclSyntax` node.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ extension FunctionScopeSyntax {
@_spi(Experimental) public var defaultIntroducedNames: [LookupName] {
signature.parameterClause.parameters.flatMap { parameter in
LookupName.getNames(from: parameter)
} + (parentScope?.is(MemberBlockSyntax.self) ?? false ? [.implicit(.self(self))] : [])
} + (isMember ? [.implicit(.self(self))] : [])
}

/// Lookup results from this function scope.
Expand Down
174 changes: 140 additions & 34 deletions Sources/SwiftLexicalLookup/Scopes/ScopeImplementations.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//

import SwiftIfConfig
import SwiftSyntax

@_spi(Experimental) extension SyntaxProtocol {
Expand Down Expand Up @@ -391,7 +392,11 @@ import SwiftSyntax
}
@_spi(Experimental) extension ExtensionDeclSyntax: LookInMembersScopeSyntax {
@_spi(Experimental) public var lookupMembersPosition: AbsolutePosition {
extendedType.position
if let memberType = extendedType.as(MemberTypeSyntax.self) {
return memberType.name.positionAfterSkippingLeadingTrivia
}

return extendedType.positionAfterSkippingLeadingTrivia
}

@_spi(Experimental) public var defaultIntroducedNames: [LookupName] {
Expand Down Expand Up @@ -420,8 +425,8 @@ import SwiftSyntax
+ defaultLookupImplementation(identifier, at: lookUpPosition, with: config, propagateToParent: false)
+ [.lookInMembers(self)]
+ lookupInParent(identifier, at: lookUpPosition, with: config)
} else if !extendedType.range.contains(lookUpPosition) && genericWhereClause != nil {
if inRightTypeOrSameTypeRequirement(lookUpPosition) {
} else if !extendedType.range.contains(lookUpPosition), let genericWhereClause {
if genericWhereClause.range.contains(lookUpPosition) {
return [.lookInGenericParametersOfExtendedType(self)] + [.lookInMembers(self)]
+ defaultLookupImplementation(identifier, at: lookUpPosition, with: config)
}
Expand All @@ -433,23 +438,6 @@ import SwiftSyntax
return [.lookInGenericParametersOfExtendedType(self)]
+ lookupInParent(identifier, at: lookUpPosition, with: config)
}

/// Returns `true` if `checkedPosition` is a right type of a
/// conformance requirement or inside a same type requirement.
private func inRightTypeOrSameTypeRequirement(
_ checkedPosition: AbsolutePosition
) -> Bool {
genericWhereClause?.requirements.contains { elem in
switch Syntax(elem.requirement).as(SyntaxEnum.self) {
case .conformanceRequirement(let conformanceRequirement):
return conformanceRequirement.rightType.range.contains(checkedPosition)
case .sameTypeRequirement(let sameTypeRequirement):
return sameTypeRequirement.range.contains(checkedPosition)
default:
return false
}
} ?? false
}
}

@_spi(Experimental) extension AccessorDeclSyntax: ScopeSyntax {
Expand Down Expand Up @@ -491,7 +479,7 @@ import SwiftSyntax

let implicitSelf: [LookupName] = [.implicit(.self(self))]
.filter { name in
checkIdentifier(identifier, refersTo: name, at: lookUpPosition)
checkIdentifier(identifier, refersTo: name, at: lookUpPosition) && !attributes.range.contains(lookUpPosition)
}

return defaultLookupImplementation(
Expand All @@ -512,13 +500,19 @@ import SwiftSyntax
@_spi(Experimental) extension CatchClauseSyntax: ScopeSyntax {
/// Implicit `error` when there are no catch items.
@_spi(Experimental) public var defaultIntroducedNames: [LookupName] {
var containsExpressionSyntax = false

let extractedNames = catchItems.flatMap { item in
guard let pattern = item.pattern else { return [LookupName]() }

if !containsExpressionSyntax && pattern.is(ExpressionPatternSyntax.self) {
containsExpressionSyntax = true
}

Comment on lines +527 to +530
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 because we suppress the implicit error when there is some kind of pattern? Please leave such a comment with a code example to remind our future selves.

return LookupName.getNames(from: pattern)
}

return extractedNames.isEmpty ? [.implicit(.error(self))] : extractedNames
return extractedNames.isEmpty && !containsExpressionSyntax ? [.implicit(.error(self))] : extractedNames
}

@_spi(Experimental) public var scopeDebugName: String {
Expand Down Expand Up @@ -594,14 +588,27 @@ import SwiftSyntax
checkIdentifier(identifier, refersTo: name, at: lookUpPosition)
}

return sequentialLookup(
in: statements,
identifier,
at: lookUpPosition,
with: config,
propagateToParent: false
) + LookupResult.getResultArray(for: self, withNames: filteredNamesFromLabel)
+ (config.finishInSequentialScope ? [] : lookupInParent(identifier, at: lookUpPosition, with: config))
if label.range.contains(lookUpPosition) {
return config.finishInSequentialScope ? [] : lookupInParent(identifier, at: lookUpPosition, with: config)
} else if config.finishInSequentialScope {
return sequentialLookup(
in: statements,
identifier,
at: lookUpPosition,
with: config,
propagateToParent: false
)
} else {
return sequentialLookup(
in: statements,
identifier,
at: lookUpPosition,
with: config,
propagateToParent: false
)
+ LookupResult.getResultArray(for: self, withNames: filteredNamesFromLabel)
+ lookupInParent(identifier, at: lookUpPosition, with: config)
}
}
}

Expand Down Expand Up @@ -697,6 +704,18 @@ import SwiftSyntax
}
}

@_spi(Experimental) extension MacroDeclSyntax: WithGenericParametersScopeSyntax {
public var defaultIntroducedNames: [LookupName] {
signature.parameterClause.parameters.flatMap { parameter in
LookupName.getNames(from: parameter)
}
}

@_spi(Experimental) public var scopeDebugName: String {
"MacroDeclScope"
}
}

@_spi(Experimental)
extension SubscriptDeclSyntax: WithGenericParametersScopeSyntax, CanInterleaveResultsLaterScopeSyntax {
/// Parameters introduced by this subscript and possibly `self` keyword.
Expand Down Expand Up @@ -758,7 +777,7 @@ extension SubscriptDeclSyntax: WithGenericParametersScopeSyntax, CanInterleaveRe
) -> [LookupResult] {
var thisScopeResults: [LookupResult] = []

if !parameterClause.range.contains(lookUpPosition) && !returnClause.range.contains(lookUpPosition) {
if accessorBlock?.range.contains(lookUpPosition) ?? false {
thisScopeResults = defaultLookupImplementation(
identifier,
at: position,
Expand Down Expand Up @@ -866,8 +885,6 @@ extension SubscriptDeclSyntax: WithGenericParametersScopeSyntax, CanInterleaveRe
with config: LookupConfig
) -> [LookupResult] {
if bindings.first?.accessorBlock?.range.contains(lookUpPosition) ?? false {
let isMember = parentScope?.is(MemberBlockSyntax.self) ?? false

return defaultLookupImplementation(
in: (isMember ? [.implicit(.self(self))] : LookupName.getNames(from: self)),
identifier,
Expand All @@ -887,10 +904,99 @@ extension SubscriptDeclSyntax: WithGenericParametersScopeSyntax, CanInterleaveRe
with config: LookupConfig,
resultsToInterleave: [LookupResult]
) -> [LookupResult] {
guard parentScope?.is(MemberBlockSyntax.self) ?? false else {
guard isMember else {
return lookup(identifier, at: lookUpPosition, with: config)
}

return resultsToInterleave + lookupInParent(identifier, at: lookUpPosition, with: config)
}
}

@_spi(Experimental) extension DeinitializerDeclSyntax: ScopeSyntax {
@_spi(Experimental) public var defaultIntroducedNames: [LookupName] {
[.implicit(.self(self))]
}

@_spi(Experimental) public var scopeDebugName: String {
"DeinitializerScope"
}
}

@_spi(Experimental) extension IfConfigDeclSyntax: IntroducingToSequentialParentScopeSyntax, SequentialScopeSyntax {
Copy link
Member

Choose a reason for hiding this comment

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

This was quite straightforward, I love it!

/// Names from all clauses.
var namesIntroducedToSequentialParent: [LookupName] {
clauses.flatMap { clause in
clause.elements.flatMap { element in
LookupName.getNames(from: element, accessibleAfter: element.endPosition)
} ?? []
}
}

/// Performs sequential lookup in the active clause.
/// Active clause is determined by the `BuildConfiguration`
/// inside `config`. If not specified, defaults to the `#else` clause.
func lookupFromSequentialParent(
_ identifier: Identifier?,
at lookUpPosition: AbsolutePosition,
with config: LookupConfig
) -> [LookupResult] {
let clause: IfConfigClauseSyntax?

if let buildConfiguration = config.buildConfiguration {
(clause, _) = activeClause(in: buildConfiguration)
} else {
clause =
clauses
.first { clause in
clause.poundKeyword.tokenKind == .poundElse
}
}

return sequentialLookup(
in: clause?.elements?.as(CodeBlockItemListSyntax.self) ?? [],
identifier,
at: lookUpPosition,
with: config,
ignoreNamedDecl: true,
propagateToParent: false
)
}

/// Returns all `NamedDeclSyntax` nodes in the active clause specified
/// by `BuildConfiguration` in `config` from bottom-most to top-most.
func getNamedDecls(for config: LookupConfig) -> [NamedDeclSyntax] {
let clause: IfConfigClauseSyntax?

if let buildConfiguration = config.buildConfiguration {
(clause, _) = activeClause(in: buildConfiguration)
} else {
clause =
clauses
.first { clause in
clause.poundKeyword.tokenKind == .poundElse
}
Comment on lines +992 to +996
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to use the #else clause if no configured regions are specified? It’s a question. I don’t know what the correct answer here should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was my intention that when config.buildConfiguration is set to nil it should provide some "default" behavior. I think it makes sense to use #else as the only other logical solution here I can think of would be maybe not including any names from #if clauses? Either way, I'm open to debate and I'm curious what do you think about it.

Copy link
Member

Choose a reason for hiding this comment

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

The only alternative I can think of would be to throw an error or something like that. @DougGregor Do you remember what we do in SwiftIfConfig when we’re unable to answer a query in the build configuration?

Copy link
Member

Choose a reason for hiding this comment

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

SwiftIfConfig treats the condition as having failed if the configuration can't answer the query.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I guess that matches picking the #else clause then.

}

guard let clauseElements = clause?.elements?.as(CodeBlockItemListSyntax.self) else { return [] }

var result: [NamedDeclSyntax] = []

for elem in clauseElements.reversed() {
if let namedDecl = elem.item.asProtocol(NamedDeclSyntax.self) {
result.append(namedDecl)
} else if let ifConfigDecl = elem.item.as(IfConfigDeclSyntax.self) {
result += ifConfigDecl.getNamedDecls(for: config)
}
}

return result
}

@_spi(Experimental) public var defaultIntroducedNames: [LookupName] {
[]
}

@_spi(Experimental) public var scopeDebugName: String {
"IfConfigScope"
}
}
19 changes: 19 additions & 0 deletions Sources/SwiftLexicalLookup/Scopes/ScopeSyntax.swift
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,27 @@ extension SyntaxProtocol {
introducedName.isAccessible(at: lookUpPosition) && introducedName.refersTo(identifier)
}

var isMember: Bool {
if parentScope?.is(MemberBlockSyntax.self) ?? false {
return true
} else if let parentIfConfig = parentScope?.as(IfConfigDeclSyntax.self) {
return parentIfConfig.isMember
} else {
return false
}
}

/// Debug description of this scope.
@_spi(Experimental) public var scopeDebugDescription: String {
scopeDebugName + " " + debugLineWithColumnDescription
}

/// Hierarchy of scopes starting from this scope.
@_spi(Experimental) public var scopeDebugHierarchyDescription: String {
if let parentScope = parentScope {
return parentScope.scopeDebugHierarchyDescription + " <-- " + scopeDebugDescription
} else {
return scopeDebugDescription
}
}
}
Loading