Skip to content

[SwiftLexicalLookup] Add ASTScope related fixes and lookInMembers result kind. #2852

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.

Sign up for GitHub

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 8 commits into from
Oct 8, 2024
Merged

[SwiftLexicalLookup] Add ASTScope related fixes and lookInMembers result kind. #2852

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d83af9c
Add astscope related fixes and lookInMembers functionality.
MAJKFL Sep 17, 2024
7521e4e
Add sequential handling of `subscript` and switch `case` scopes. Remo…
MAJKFL Sep 18, 2024
fae4d0e
Fix ordering of implicit `self` introduction by subscript accessors.
MAJKFL Sep 18, 2024
3de7208
Add initializer declaration scope. Generalize function decl and initi…
MAJKFL Sep 19, 2024
433f14b
Add suggested changes. Add new specialized scope: CanInterleaveResult…
MAJKFL Sep 23, 2024
0f9b2bc
Add implicit `Self` to extension declaration scope. Fix name introduc…
MAJKFL Sep 25, 2024
baf6a2f
Fix formatting issue caused by too long `todo` comments.
MAJKFL Sep 26, 2024
fc1ab88
In protocol decl scope, whitelist member block instead of filtering l…
MAJKFL Sep 26, 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
20 changes: 13 additions & 7 deletions Sources/SwiftLexicalLookup/LookupName.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import SwiftSyntax
case `self`(DeclSyntaxProtocol)
/// `Self` keyword representing object type.
/// Could be associated with type declaration or extension.
case `Self`(ProtocolDeclSyntax)
case `Self`(DeclSyntaxProtocol)
/// `error` value caught by a `catch`
/// block that does not specify a catch pattern.
case error(CatchClauseSyntax)
Expand Down Expand Up @@ -147,27 +147,33 @@ import SwiftSyntax
case .identifier(let syntax, _):
return syntax.identifier.positionAfterSkippingLeadingTrivia
case .declaration(let syntax):
return syntax.name.position
return syntax.name.positionAfterSkippingLeadingTrivia
case .implicit(let implicitName):
switch implicitName {
case .self(let declSyntax):
switch Syntax(declSyntax).as(SyntaxEnum.self) {
case .functionDecl(let functionDecl):
return functionDecl.name.position
return functionDecl.name.positionAfterSkippingLeadingTrivia
case .initializerDecl(let initializerDecl):
return initializerDecl.initKeyword.positionAfterSkippingLeadingTrivia
case .subscriptDecl(let subscriptDecl):
return subscriptDecl.accessorBlock?.position ?? subscriptDecl.endPosition
return subscriptDecl.accessorBlock?.positionAfterSkippingLeadingTrivia
?? subscriptDecl.endPositionBeforeTrailingTrivia
case .variableDecl(let variableDecl):
return variableDecl.bindings.first?.accessorBlock?.positionAfterSkippingLeadingTrivia
?? variableDecl.endPosition
default:
return declSyntax.positionAfterSkippingLeadingTrivia
}
case .Self(let protocolDecl):
return protocolDecl.name.positionAfterSkippingLeadingTrivia
case .Self(let declSyntax):
switch Syntax(declSyntax).as(SyntaxEnum.self) {
case .protocolDecl(let protocolDecl):
return protocolDecl.name.positionAfterSkippingLeadingTrivia
Comment on lines +170 to +171
Copy link
Member

Choose a reason for hiding this comment

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

Why do we only need this for protocols and not other type declarations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR, protocol declarations are the only places we are introducing Self keyword, so handling other declarations wouldn’t matter in this case. I changed associated syntax node type of ImplicitDecl.Self to ProtocolDeclSyntax, to be more explicit and remove this nested switch.

Copy link
Member

Choose a reason for hiding this comment

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

So, you’re not handling Self in classes yet?

class Foo {
  func bar() -> Self { fatalError() }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, it seems to me like ASTScope doesn’t introduce implicit Self in nominal type declarations. I suppose this could be implicitly modeled by the lookInMembers result? Here’s the entire output of the validation function for the snippet you’ve provided

-----> Lookup started at: 2:17 ("Self")
     |      ASTScope      | SwiftLexicalLookup 
> ✅ |   Look memb: 1:7   |   Look memb: 1:7   

-----> Lookup started at: 2:17 ("Self")
     |      ASTScope      | SwiftLexicalLookup 
> ✅ |   Look memb: 1:7   |   Look memb: 1:7   

-----> Lookup started at: 2:24 ("fatalError")
     |      ASTScope      | SwiftLexicalLookup 
> ✅ |      self 2:8      |      self 2:8      
> ✅ |End here: Look memb: 1:7|   Look memb: 1:7   

-----> Lookup started at: 2:24 ("fatalError")
     |      ASTScope      | SwiftLexicalLookup 
> ✅ |      self 2:8      |      self 2:8      
> ✅ |   Look memb: 1:7   |   Look memb: 1:7

Nevertheless, there certainly are more places Self is introduced apart from protocol declarations. This seems to be also the case for extension declarations. I fixed it now as well and this is how the output looks like for the following snippet (you can also notice here the duplicate omission I’ve mentioned in my previous comment)

protocol Foo {}
extension Foo {
  func bar() -> Self { fatalError() }
}
-----> Lookup started at: 2:11 ("Foo")
     |      ASTScope      | SwiftLexicalLookup 

-----> Lookup started at: 2:11 ("Foo")
     |      ASTScope      | SwiftLexicalLookup 

-----> Lookup started at: 3:17 ("Self")
     |      ASTScope      | SwiftLexicalLookup 
> ✅ | End here: Self 2:1 |      Self 2:1      
> ⏩ |       -----        |  Look memb: 2:11   

-----> Lookup started at: 3:24 ("fatalError")
     |      ASTScope      | SwiftLexicalLookup 
> ✅ |      self 3:8      |      self 3:8      
> ✅ |      Self 2:1      |      Self 2:1      
> ✅ |End here: Look memb: 2:11|  Look memb: 2:11   

-----> Lookup started at: 3:24 ("fatalError")
     |      ASTScope      | SwiftLexicalLookup 
> ✅ |      self 3:8      |      self 3:8      
> ✅ |      Self 2:1      |      Self 2:1      
> ✅ |  Look memb: 2:11   |  Look memb: 2:11   
> ℹ️ | Omitted ASTScope name: Self 2:1
> ℹ️ | Omitted ASTScope name: Self 2:1

default:
return declSyntax.positionAfterSkippingLeadingTrivia
}
case .error(let catchClause):
return catchClause.body.position
return catchClause.body.positionAfterSkippingLeadingTrivia
default:
return implicitName.syntax.positionAfterSkippingLeadingTrivia
}
Expand Down
3 changes: 3 additions & 0 deletions Sources/SwiftLexicalLookup/Scopes/CanInterleaveResultsLaterScopeSyntax.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
import SwiftSyntax

protocol CanInterleaveResultsLaterScopeSyntax: ScopeSyntax {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment explaining what it means to interleave results later?

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 added documentation comments to the definition of lookupWithInterleavedResults function and it's concrete implementations.

/// Perform lookup in this scope and later introduce results
/// passed as `resultsToInterleave`.
/// The exact behavior depends on a specific scope.
func lookupWithInterleavedResults(
_ identifier: Identifier?,
at lookUpPosition: AbsolutePosition,
Expand Down
26 changes: 18 additions & 8 deletions Sources/SwiftLexicalLookup/Scopes/ScopeImplementations.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,13 @@ import SwiftSyntax
with config: LookupConfig
) -> [LookupResult] {
if memberBlock.range.contains(lookUpPosition) {
return [.lookInMembers(self)] + defaultLookupImplementation(identifier, at: lookUpPosition, with: config)
let implicitSelf: [LookupName] = [.implicit(.Self(self))]
.filter { name in
checkIdentifier(identifier, refersTo: name, at: lookUpPosition)
}

return (implicitSelf.isEmpty ? [] : [.fromScope(self, withNames: implicitSelf)]) + [.lookInMembers(self)]
+ defaultLookupImplementation(identifier, at: lookUpPosition, with: config)
} else {
return defaultLookupImplementation(identifier, at: lookUpPosition, with: config)
}
Expand Down Expand Up @@ -665,13 +671,16 @@ import SwiftSyntax
)
}

let lookInMembers: [LookupResult] =
inheritanceClause?.range.contains(lookUpPosition) ?? false ? [] : [.lookInMembers(self)]
Copy link
Member

Choose a reason for hiding this comment

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

Instead of blacklisting the inheritance clause, wouldn’t it be better to whitelist the memberBlock? I got to this because I was thinking that self shouldn’t be available in the generic parameters of the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. As for now, I couldn't find a specific case this wouldn't work and it definitely improves readability of this part of code as well. I changed it to check if lookup was started inside memberBlock.


return results
+ defaultLookupImplementation(
identifier,
at: lookUpPosition,
with: config,
propagateToParent: false
) + [.lookInMembers(self)] + lookupInParent(identifier, at: lookUpPosition, with: config)
) + lookInMembers + lookupInParent(identifier, at: lookUpPosition, with: config)
}
}

Expand Down Expand Up @@ -868,12 +877,11 @@ extension SubscriptDeclSyntax: WithGenericParametersScopeSyntax, CanInterleaveRe
at lookUpPosition: AbsolutePosition,
with config: LookupConfig
) -> [LookupResult] {
if let parentScope,
parentScope.is(MemberBlockSyntax.self),
bindings.first?.accessorBlock?.range.contains(lookUpPosition) ?? false
{
if bindings.first?.accessorBlock?.range.contains(lookUpPosition) ?? false {
let shouldIntroduceSelf = parentScope?.is(MemberBlockSyntax.self) ?? false

return defaultLookupImplementation(
in: [.implicit(.self(self))],
in: LookupName.getNames(from: self) + (shouldIntroduceSelf ? [.implicit(.self(self))] : []),
identifier,
at: lookUpPosition,
with: config
Expand All @@ -883,14 +891,16 @@ extension SubscriptDeclSyntax: WithGenericParametersScopeSyntax, CanInterleaveRe
}
}

/// If a member, introduce results passed in `resultsToInterleave`
/// and then pass lookup to the parent. Otherwise, perform `lookup`.
func lookupWithInterleavedResults(
_ identifier: Identifier?,
at lookUpPosition: AbsolutePosition,
with config: LookupConfig,
resultsToInterleave: [LookupResult]
) -> [LookupResult] {
guard parentScope?.is(MemberBlockSyntax.self) ?? false else {
return lookupInParent(identifier, at: lookUpPosition, with: config)
return lookup(identifier, at: lookUpPosition, with: config)
}

return resultsToInterleave + lookupInParent(identifier, at: lookUpPosition, with: config)
Expand Down
6 changes: 4 additions & 2 deletions Tests/SwiftLexicalLookupTest/NameLookupTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ final class testNameLookup: XCTestCase {
func testImplicitSelf() {
assertLexicalNameLookup(
source: """
extension a {
7️⃣extension a {
struct b {
2️⃣func foo() {
let x: 3️⃣Self = 4️⃣self
Expand All @@ -675,6 +675,7 @@ final class testNameLookup: XCTestCase {
references: [
"3️⃣": [
.lookInMembers(StructDeclSyntax.self),
.fromScope(ExtensionDeclSyntax.self, expectedNames: [NameExpectation.implicit(.Self("7️⃣"))]),
.lookInMembers(ExtensionDeclSyntax.self),
],
"4️⃣": [
Expand All @@ -683,7 +684,8 @@ final class testNameLookup: XCTestCase {
.lookInMembers(ExtensionDeclSyntax.self),
],
"5️⃣": [
.lookInMembers(ExtensionDeclSyntax.self)
.fromScope(ExtensionDeclSyntax.self, expectedNames: [NameExpectation.implicit(.Self("7️⃣"))]),
.lookInMembers(ExtensionDeclSyntax.self),
],
"6️⃣": [
.fromScope(FunctionDeclSyntax.self, expectedNames: [NameExpectation.implicit(.self("1️⃣"))]),
Expand Down