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

MAJKFL · 2024-09-17T16:11:31Z

This PR brings closer the current SwiftLexicalLookup unqualified lookup implementation to ASTScope. Changes include:

New lookInMembers result kind. It prompts client where to perform member lookup.
Add configuration to not include member names. Compiler doesn't introduce members during unqualified lookup itself.
Add configuration to terminate lookup early in first encountered sequential scope. It's supposed to match behavior of ASTScope::lookupLocalDecls stopAfterInnermostBraceStmt parameter.
Fix self keyword behavior. Compiler introduces it on a function boundary, that's why this PR moves it into function declaration scope.
Reverse order of name introductions in sequential scopes.
Make closure expression a sequential scope to properly handle it's children.
Remove setting of accessibleAfter for generic parameters.

This PR also includes a simple debug description functionality for all lookup components to make it easier to work with the library.

…ve wild card filtering.

…alizer decl scopes to function scope. Introduce `self` keyword on the edge of computed properties.

MAJKFL · 2024-09-19T13:15:50Z

With the recent changes in this PR, SwiftLexicalLookup actually matches results of ASTScope unqualified lookup for almost all source files of SwiftIfConfig when compared with this validation function! The only exception is handling of $0 in closures.

public var description: String {
  return components.map { String($0) }.joined(separator: ".")
}

As for now, SwiftLexicalLookup is missing the representation of almost visible names in scope. Additionally the validation method used for comparison accounts for some ASTScope behavior:

It skips the same (generic parameter) names introduced more than once by ASTScope.
It skips variable declaration names visible inside of their declarations.
It reorders results returned from generic parameter clauses of function declarations for consistency.

It might be necessary to revisit some of those cases in the future, but I agreed with @DougGregor to ignore them for now.

Here’s a sample output of properly matched results for one of the references from ConfiguredRegions.swift:

-----> Lookup started at: 215:19 ("node")
     |      ASTScope      | SwiftLexicalLookup 
> ✅ |  outerState 214:9  |  outerState 214:9  
> ✅ |syntaxErrorsAllowed 213:9|syntaxErrorsAllowed 213:9
> ✅ | foundActive 212:9  | foundActive 212:9  
> ✅ |priorInAnyIfConfig 205:9|priorInAnyIfConfig 205:9
> ✅ |End here: node 203:25|    node 203:25     
> ⏩ |       -----        |    self 203:17     
> ⏩ |       -----        |Configuration 177:43
> ⏩ |       -----        | Look memb: 177:19

DougGregor · 2024-09-19T18:22:59Z

@swift-ci please test

DougGregor · 2024-09-19T21:52:45Z

@MAJKFL looks like you need to run the format script

ahoppen

Left a few comments of things I noticed while reading through the change.

Super exciting to see this progressing and seeing you continue working on it after the GSoC project is over, @MAJKFL ❤️

Sources/SwiftLexicalLookup/Configurations/LookupConfig.swift

Sources/SwiftLexicalLookup/LookupName.swift

ahoppen · 2024-09-20T01:20:18Z

Sources/SwiftLexicalLookup/LookupName.swift

+        case .protocolDecl(let protocolDecl):
+          return protocolDecl.name.positionAfterSkippingLeadingTrivia


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

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.

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

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

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

Sources/SwiftLexicalLookup/LookupName.swift

Sources/SwiftLexicalLookup/LookupResult.swift

Sources/SwiftLexicalLookup/Scopes/ScopeImplementations.swift

ahoppen · 2024-09-20T01:31:46Z

Sources/SwiftLexicalLookup/Scopes/ScopeImplementations.swift

+    let implicitSelf: [LookupName] = [.implicit(.self(self))]
+      .filter { name in
+        checkIdentifier(identifier, refersTo: name, at: lookUpPosition)
+      }


Should we only add self if this is a member? Ie. we shouldn’t add self in a top level variable with an accessor, right?

This case was handled here implicitly. The result with self was handed over to a special function that passed it to AccessorBlockSyntax parent which later handed it off to it’s SubscriptDeclSyntax parent.

I’ve noticed now this solution wasn’t perfect though. It was quite inflexible and didn’t work for variable declaration accessors. To fix this, I introduced now a new specialized scope kind: CanInterleaveResultsLaterScopeSyntax with lookupWithInterleavedResults function to further generalize this behavior. Now accessor can hand the result with self keyword to it’s CanInterleaveResultsLaterScopeSyntax parent. The self keyword is introduced properly by accessors inside subscript and variables and it does differentiate whether it’s a member or not.

…sLaterScopeSyntax.

ahoppen · 2024-09-24T20:21:27Z

Sources/SwiftLexicalLookup/Scopes/CanInterleaveResultsLaterScopeSyntax.swift

-  @_spi(Experimental) public var introducedNames: [LookupName] {
-    implicitInstanceAndTypeNames
-  }
+protocol CanInterleaveResultsLaterScopeSyntax: ScopeSyntax {


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

Thanks for pointing this out! I added documentation comments to the definition of lookupWithInterleavedResults function and it's concrete implementations.

…tion of a variable with accessors in global scope. Add suggested changes and documentation.

DougGregor · 2024-09-25T15:28:31Z

@swift-ci please test

DougGregor · 2024-09-25T15:39:30Z

@swift-ci please test Windows

ahoppen · 2024-09-25T21:15:48Z

Sources/SwiftLexicalLookup/Scopes/ScopeImplementations.swift

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


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.

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.

DougGregor · 2024-09-25T23:37:39Z

macOS failure is a formatting issue:

syntax/Tests/SwiftLexicalLookupTest/NameLookupTests.swift:897:34: warning: [EndOfLineComment] move end-of-line comment that exceeds the line length

DougGregor · 2024-09-25T23:42:09Z

@swift-ci please build toolchain

MAJKFL · 2024-09-26T08:44:21Z

Oh, so the comments were the problem, so that's why running formatting script didn't help. Thanks for pointing this out! Hopefully should be fixed now.

…ookup position from inheritance clause.

DougGregor · 2024-09-26T18:08:21Z

@swift-ci please test

DougGregor · 2024-09-26T18:15:11Z

@swift-ci please test

DougGregor · 2024-10-02T15:31:21Z

@swift-ci test macOS

MAJKFL · 2024-10-06T08:30:51Z

@swift-ci please test

MAJKFL · 2024-10-07T06:56:26Z

Seems like everything passed now, do you think @ahoppen it's good to merge?

Add astscope related fixes and lookInMembers functionality.

d83af9c

MAJKFL requested review from ahoppen and bnbarham as code owners September 17, 2024 16:11

MAJKFL added 3 commits September 18, 2024 14:52

Add sequential handling of subscript and switch case scopes. Remo…

7521e4e

…ve wild card filtering.

Fix ordering of implicit self introduction by subscript accessors.

fae4d0e

Add initializer declaration scope. Generalize function decl and initi…

3de7208

…alizer decl scopes to function scope. Introduce `self` keyword on the edge of computed properties.

ahoppen reviewed Sep 20, 2024

View reviewed changes

Add suggested changes. Add new specialized scope: CanInterleaveResult…

433f14b

…sLaterScopeSyntax.

MAJKFL requested a review from ahoppen September 23, 2024 19:50

ahoppen reviewed Sep 24, 2024

View reviewed changes

Add implicit Self to extension declaration scope. Fix name introduc…

0f9b2bc

…tion of a variable with accessors in global scope. Add suggested changes and documentation.

MAJKFL requested a review from ahoppen September 25, 2024 11:27

ahoppen reviewed Sep 25, 2024

View reviewed changes

Fix formatting issue caused by too long todo comments.

baf6a2f

In protocol decl scope, whitelist member block instead of filtering l…

fc1ab88

…ookup position from inheritance clause.

MAJKFL requested a review from ahoppen September 26, 2024 09:43

DougGregor approved these changes Oct 7, 2024

View reviewed changes

ahoppen approved these changes Oct 8, 2024

View reviewed changes

MAJKFL merged commit 084005f into swiftlang:main Oct 8, 2024
3 checks passed

MAJKFL deleted the astscope-fixes-generics-lookmembers branch October 16, 2024 11:24

		case .protocolDecl(let protocolDecl):
		return protocolDecl.name.positionAfterSkippingLeadingTrivia

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

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

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

Uh oh!

Conversation

MAJKFL commented Sep 17, 2024

Uh oh!

MAJKFL commented Sep 19, 2024

Uh oh!

DougGregor commented Sep 19, 2024

Uh oh!

DougGregor commented Sep 19, 2024

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Sep 25, 2024

Uh oh!

DougGregor commented Sep 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DougGregor commented Sep 25, 2024

Uh oh!

DougGregor commented Sep 25, 2024

Uh oh!

MAJKFL commented Sep 26, 2024

Uh oh!

DougGregor commented Sep 26, 2024

Uh oh!

DougGregor commented Sep 26, 2024

Uh oh!

DougGregor commented Oct 2, 2024

Uh oh!

MAJKFL commented Oct 6, 2024

Uh oh!

MAJKFL commented Oct 7, 2024

Uh oh!

Uh oh!

Uh oh!