Skip to content

[SwiftLexicalLookup][GSoC] Add proper guard scope and implicit name lookup #2748

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 33 commits into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
e15bfaa
Format.
MAJKFL Jul 15, 2024
a68f00a
Merge branch 'main'
MAJKFL Jul 17, 2024
99b5294
Add guard support to file scope.
MAJKFL Jul 17, 2024
c31046a
Add implicit names and proper guard handling.
MAJKFL Jul 18, 2024
8b5fbd2
Separate lookup state from config.
MAJKFL Jul 23, 2024
93cb3ea
Create a new enum representing all possible implicit name cases. Simp…
MAJKFL Jul 23, 2024
9945bf0
Add suggested changes from the previous PR.
MAJKFL Jul 23, 2024
6759b05
Make `IntroducingToSequentialParentScopeSyntax` and `SequentialScope`…
MAJKFL Jul 23, 2024
2d109e5
Merge branch 'main'
MAJKFL Jul 23, 2024
940b06e
Add proper guard scope and implicit name lookup.
MAJKFL Jul 23, 2024
c5988ed
Improve documentation.
MAJKFL Jul 25, 2024
0ef6a21
Add identifier based name lookup.
MAJKFL Jul 25, 2024
9a28d02
Fix handling inline variable declaration lists.
MAJKFL Jul 25, 2024
d547606
Merge branch 'guard-scope'
MAJKFL Jul 25, 2024
32fec70
Use IdentifierKind enum to represend different identifiers internally.
MAJKFL Jul 29, 2024
e96a324
Merge remote-tracking branch 'upstream/main' into guard-and-implicit-…
MAJKFL Jul 30, 2024
0e5009a
Fix merge artifacts.
MAJKFL Jul 30, 2024
166fdb7
Merge branch 'identifier-based-lookup' into guard-and-implicit-name-l…
MAJKFL Jul 30, 2024
a255fc9
Fix merge artifacts.
MAJKFL Jul 30, 2024
fbf48bb
Fix merge artifacts
MAJKFL Jul 30, 2024
dda31f9
Merge changes from #2755 and add Identifier based lookup.
MAJKFL Jul 30, 2024
12a5380
Revert "Merge changes from #2755 and add Identifier based lookup."
MAJKFL Jul 30, 2024
b2e5748
Merge branch 'guard-and-implicit-name-lookup'
MAJKFL Jul 30, 2024
5705b96
Add internal `_lookup` function that passes state.
MAJKFL Jul 30, 2024
539b1d7
Add suggested changes.
MAJKFL Aug 1, 2024
5461b07
Add catch clause implicit `error`. Fix declaration name hoisting in s…
MAJKFL Aug 1, 2024
059860a
Break early from sequential for loop when over lookup origin position.
MAJKFL Aug 1, 2024
252d3be
Simplify lookup from `guard ... else` handling.
MAJKFL Aug 1, 2024
be2e588
Format.
MAJKFL Aug 1, 2024
01f2887
Add suggestions
MAJKFL Aug 2, 2024
cf273f8
Format.
MAJKFL Aug 2, 2024
c83b3c6
Improve documentation. Improve sequential scope test case.
MAJKFL Aug 2, 2024
fcaecc7
Remove optional chaining from identifier test.
MAJKFL Aug 2, 2024
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
6 changes: 6 additions & 0 deletions Sources/SwiftLexicalLookup/IdentifiableSyntax.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,9 @@ import SwiftSyntax
expression.as(DeclReferenceExprSyntax.self)!.baseName
}
}

@_spi(Experimental) extension AccessorParametersSyntax: IdentifiableSyntax {
@_spi(Experimental) public var identifier: TokenSyntax {
name
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
//===----------------------------------------------------------------------===//
//
// 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 SwiftSyntax

protocol IntroducingToSequentialParentScopeSyntax: ScopeSyntax {
/// Returns names matching lookup that should be
/// handled by it's parent sequential scope.
func introducesToSequentialParent(
Copy link
Member

Choose a reason for hiding this comment

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

I find it a little surprising that ScopeSyntax has an API that returns all names introduce in its scope (introducedNames) while this API contains a filter. I think it would be more consistent if this also returned all the names introduced to the parent scope and then be named namesIntroducedToParentScope. As far as I can tell it wouldn’t need to take any parameters at all then and could be a computed property 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.

I think that was poor naming on my part as introducesToSequentialParent worked more like lookup producing [LookupResult] to be interleaved with results of the sequential parent, rather than introducedNames that just return all LookupNames available at that scope. I renamed introducesToSequentialParent to less misleading lookupFromSequentialParent and added namesIntroducedToSequentialParent that closely resembles introducedNames of a normal scope.

for identifier: Identifier?,
at origin: SyntaxProtocol,
with config: LookupConfig,
state: LookupState
) -> [LookupResult]
}
104 changes: 93 additions & 11 deletions Sources/SwiftLexicalLookup/LookupName.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,93 @@

import SwiftSyntax

/// An entity that is implicitly declared based on the syntactic structure of the program.
@_spi(Experimental) public enum ImplicitDecl {
/// `self` keyword representing object instance.
/// Could be associated with type declaration, extension,
/// or closure captures.
case `self`(DeclSyntaxProtocol)
/// `Self` keyword representing object type.
/// Could be associated with type declaration or extension.
case `Self`(DeclSyntaxProtocol)
/// `error` value caught by a `catch`
/// block that does not specify a catch pattern.
case error(CatchClauseSyntax)
/// `newValue` available by default inside `set` and `willSet`.
case newValue(AccessorDeclSyntax)
/// `oldValue` available by default inside `didSet`.
case oldValue(AccessorDeclSyntax)

/// Syntax associated with this name.
@_spi(Experimental) public var syntax: SyntaxProtocol {
switch self {
case .self(let syntax):
return syntax
case .Self(let syntax):
return syntax
case .error(let syntax):
return syntax
case .newValue(let syntax):
return syntax
case .oldValue(let syntax):
return syntax
}
}

/// The name of the implicit declaration.
private var name: String {
switch self {
case .self:
return "self"
case .Self:
return "Self"
case .error:
return "error"
case .newValue:
return "newValue"
case .oldValue:
return "oldValue"
}
}

/// Identifier used for name comparison.
///
///
/// ```swift
/// class Foo {
/// func test() {
/// let `Self` = "abc"
/// print(Self.self)
///
/// let `self` = "def"
/// print(self)
/// }
/// }
///
/// Foo().test()
/// ```
/// prints:
/// ```
/// abc
/// def
/// ```
/// `self` and `Self` identifers override
/// implicit `self` and `Self` introduced by
/// the `Foo` class declaration.
var identifier: Identifier {
Identifier(name)
}
}

@_spi(Experimental) public enum LookupName {
/// Identifier associated with the name.
/// Could be an identifier of a variable, function or closure parameter and more.
case identifier(IdentifiableSyntax, accessibleAfter: AbsolutePosition?)
/// Declaration associated with the name.
/// Could be class, struct, actor, protocol, function and more.
case declaration(NamedDeclSyntax)
/// Name introduced implicitly by certain syntax nodes.
case implicit(ImplicitDecl)

/// Syntax associated with this name.
@_spi(Experimental) public var syntax: SyntaxProtocol {
Expand All @@ -27,16 +107,20 @@ import SwiftSyntax
return syntax
case .declaration(let syntax):
return syntax
case .implicit(let implicitName):
return implicitName.syntax
}
}

/// Introduced name.
/// Identifier used for name comparison.
@_spi(Experimental) public var identifier: Identifier? {
switch self {
case .identifier(let syntax, _):
return Identifier(syntax.identifier)
return Identifier(syntax.identifier) ?? Identifier(syntax.identifier.text)
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 need ?? Identifier(syntax.identifier.text) for self and Self? If so, I would prefer to handle these two keywords specifically. I’m not sure if treating a keyword the same as an identifier is safe in general (I certainly wouldn’t have expected so, see my previous example).

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 added new LookupName kinds: self and Self. As for now it's only really used in closure captures, but I agree it's nice to have a separate representation for both of those.

case .declaration(let syntax):
return Identifier(syntax.name)
case .implicit(let kind):
return kind.identifier
}
}

Expand All @@ -58,9 +142,9 @@ import SwiftSyntax
}

/// Checks if this name refers to the looked up phrase.
func refersTo(_ lookedUpName: String) -> Bool {
guard let name = identifier?.name else { return false }
return name == lookedUpName
func refersTo(_ lookedUpIdentifier: Identifier) -> Bool {
guard let identifier else { return false }
return identifier == lookedUpIdentifier
Comment on lines +168 to +170
Copy link
Member

Choose a reason for hiding this comment

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

Does this method provide any value anymore? I think you can remove the guard let identifier else { return false } and then all that’s left is identifier == lookedUpIdentifier.

}

/// Extracts names introduced by the given `syntax` structure.
Expand Down Expand Up @@ -105,10 +189,6 @@ import SwiftSyntax
return functionCallExpr.arguments.flatMap { argument in
getNames(from: argument.expression, accessibleAfter: accessibleAfter)
}
case .guardStmt(let guardStmt):
return guardStmt.conditions.flatMap { cond in
getNames(from: cond.condition, accessibleAfter: cond.endPosition)
}
default:
if let namedDecl = Syntax(syntax).asProtocol(SyntaxProtocol.self) as? NamedDeclSyntax {
return handle(namedDecl: namedDecl, accessibleAfter: accessibleAfter)
Expand All @@ -121,8 +201,10 @@ import SwiftSyntax
}

/// Extracts name introduced by `IdentifiableSyntax` node.
private static func handle(identifiable: IdentifiableSyntax, accessibleAfter: AbsolutePosition? = nil) -> [LookupName]
{
private static func handle(
identifiable: IdentifiableSyntax,
accessibleAfter: AbsolutePosition? = nil
) -> [LookupName] {
if identifiable.identifier.tokenKind != .wildcard {
return [.identifier(identifiable, accessibleAfter: accessibleAfter)]
} else {
Expand Down
19 changes: 19 additions & 0 deletions Sources/SwiftLexicalLookup/LookupState.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//===----------------------------------------------------------------------===//
//
// 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

/// Represents internal state for lookup.
/// It shouldn't be used by clients.
@_spi(Experimental) public struct LookupState {
Copy link
Member

Choose a reason for hiding this comment

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

Please document this, even if it's just to say that this is internal state for lookup that isn't used by clients.

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 added an appropriate doc comment.

On a side note, thanks to @ahoppen suggestion, I've simplified how we handle lookup from inside guard ... else body to filter those inside introducesToSequentialParent in guard scope implementation.

Do you think we should remove LookupState now, as it doesn't provide any meaningful functionality, or should we leave it as is. It could potentially be beneficial in the future to have it already accessible across the lookup API in case we want to utilize it.

Copy link
Member

Choose a reason for hiding this comment

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

If we don't need it now, let's remove it! The fewer parameters and less state we need to track, the better. If we find we need to bring it back... well, we know how to do it.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we should remove LookupState altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I removed LookupState and the internal _lookup method.

init() {}
}
Loading