[SwiftLexicalLookup][GSoC] Add proper guard scope and implicit name lookup by MAJKFL · Pull Request #2748 · swiftlang/swift-syntax

MAJKFL · 2024-07-23T10:28:01Z

This PR introduces proper guard handling and implicit name lookup.

Scopes of new internal IntroducingToSequentialParentScopeSyntax kind introduce their lookup results to their SequentialScopeSyntax parents. Currently, the only IntroducingToSequentialParentScopeSyntax is guard scope and the only SequentialScopeSyntaxs are code block and file scope (the latter one uses SequentialScopeSyntax properties depending on the configuration).
Lookup from within guard … else clause specifies skipSequentialIntroductionFrom property in the new LookupState object passed between lookup functions. I think it would be great to make this more opaque for the client, so I'd love to hear suggestions/other ideas how we could represent and pass the state between lookup methods.
Implicit name handling works through a new LookupName implicit case. Associated with it, a new enum LookupImplicitNameKind describes all possible implicit names that could be referenced during lexical name lookup.
TypeScopeSyntax defines behavior of type declaration and extensions scopes. In the subsequent PRs could be used to add generic parameter handling and possibly other common behavior.
This PR also includes some not addressed changes proposed in [SwiftLexicalLookup][GSoC] Add initial name lookup functionality #2719

Conflicts: Sources/SwiftLexicalLookup/ScopeImplementations.swift

…lify LookupName.

… internal. Format.

Conflicts: Tests/SwiftLexicalLookupTest/NameLookupTests.swift

Conflicts: Sources/SwiftLexicalLookup/Configurations/LookupConfig.swift Sources/SwiftLexicalLookup/LookupName.swift

…name-lookup

…ookup

…okup." This reverts commit dda31f9.

ahoppen

Could you update name -> identifier in various doc comment for which you changed the parameter name?

…equential scopes. Fix partitioning with non matching results from `guard` scopes.

DougGregor · 2024-08-01T19:16:33Z

@swift-ci please test

ahoppen · 2024-08-01T22:59:17Z

    switch self {
    case .identifier(let syntax, _):
-      return Identifier(syntax.identifier)
+      return Identifier(syntax.identifier) ?? Identifier(syntax.identifier.text)


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

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.

ahoppen · 2024-08-01T23:07:45Z

+      var sequentialItems = [CodeBlockItemSyntax]()
+      var encounteredNonDeclaration = false
+
+      for codeBlockItem in statements {


I'm a little confused myself as to why we're gathering declarations up to the first non-declaration, then gathering everything else as code block items, because I don't consider the first non-declaration to be special in Swift's lookup rules.

Does this mean that we should remove the memberBlockUpToLastDecl lookup kind?

ahoppen · 2024-08-01T23:14:16Z

+
+import Foundation
+
+@_spi(Experimental) public struct LookupState {


I agree that we should remove LookupState altogether.

ahoppen · 2024-08-01T23:15:10Z

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


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.

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.

ahoppen · 2024-08-01T23:29:25Z

+        result += introducedResults
+      } else {
+        // Extract new names from encountered node.
+        currentChunk += LookupName.getNames(


Why do we need currentChunk at all? Couldn’t we add the names directly to result?

Again, I think it was a bit poor naming on my side as result was an array of LookupResult. I renamed it now to results.

As I briefly mentioned in this comment, we can't add the names directly to results as we need to partition them based on results obtained from IntroducingToSequentialParentScopeSyntax.

Ah, I see. Could you add that as a comment to the code?

ahoppen · 2024-08-01T23:30:12Z

+          from: codeBlockItem.item,
+          accessibleAfter: codeBlockItem.endPosition
+        ).filter { introducedName in
+          checkName(identifier, refersTo: introducedName, at: origin)


I guess we could also move the filter all the way to the end instead of duplicating it.

As we need to create multiple LookupResults here, I think the only other possible place to move filtering to, would be when creating a new result from collected currentChunk. Either way, we'd need to traverse the same amount of items when filtering so the time complexity would be the same, but this way we can maintain smaller sizes of currentChunk.

ahoppen · 2024-08-01T23:32:00Z

+      result.append(getResults(currentChunk))
+    }
+
+    return result.reversed() + lookupInParent(for: identifier, at: origin, with: config, state: state)


Instead of reversing the result here (which requires a copy of the array), have you considered changing the semantics of the result to say a later element overrides earlier elements? I think I would also find that more intuitive because it matches the source order.

As we already represent results from inner-most to outer-most, I think it makes most sense to reverse it. Like in this example:

func foo() { let a = 1 // <-- Code block scope let b = 2 guard let b = a else { return } let a = 3 // <-- Code block scope let b = 4 guard let a = b else { return } // <-- Guard scope print(a, b) // <-- lookup for a }

I think the order of results should be: guard scope and exactly one code block scope result (as the first guard is not relevant for a lookup).

The order of LookupNames within LookupResults is the way you described actually. In the result from code block scope the order is exactly: let a = 1 and then let a = 3.

DougGregor · 2024-08-02T16:09:47Z

@swift-ci please test

DougGregor · 2024-08-02T16:10:05Z

@swift-ci please test Windows

DougGregor

While we have some open questions about the handling of top-level code, I'm happy with where this pull request is now and I think we should go ahead and land it once CI passes.

DougGregor · 2024-08-02T17:05:38Z

+      var sequentialItems = [CodeBlockItemSyntax]()
+      var encounteredNonDeclaration = false
+
+      for codeBlockItem in statements {


Maybe? There's definitely a difference here for the source file scope when top-level code is involved, and we need a way to model it. I think we should leave this in place for now and we can rename/revise as we learn more.

DougGregor · 2024-08-02T17:14:01Z

+    at syntax: SyntaxProtocol,
+    with config: LookupConfig,
+    state: LookupState,
+    createResultsForThisScopeWith getResults: ([LookupName]) -> (LookupResult)


Part of the reason for having fromFileScope be separate is because clients need to do extra work to do lookups into other source files and in visible imports from that scope, so it's both "here's what I found in this file" and "you need to go look elsewhere, too". I'm not totally settled on what the right answer is for dealing with file scope, so I'd rather keep thinking about it (but not block this PR on a resolution).

DougGregor · 2024-08-02T19:32:56Z

/home/build-user/swift-syntax/Tests/SwiftSyntaxTest/IdentifierTests.swift:42:43: error: cannot use optional chaining on non-optional value of type 'RawIdentifier'
40 |     let token = TokenSyntax(stringLiteral: "sometoken")
41 |     withExtendedLifetime(token) { token in
42 |       XCTAssertEqual(token.identifier?.raw?.name, SyntaxText("sometoken"))
   |                                           `- error: cannot use optional chaining on non-optional value of type 'RawIdentifier'
43 |     }
44 |   }

DougGregor · 2024-08-02T20:57:03Z

@swift-ci please test

DougGregor · 2024-08-02T20:58:32Z

@swift-ci please test

MAJKFL · 2024-08-05T16:43:47Z

Not sure what failed here now. I see in the console output all tests passed

Test Suite 'All tests' passed at 2024-08-02 23:13:38.842
	 Executed 640 tests, with 24 tests skipped and 0 failures (0 unexpected) in 448.763 (448.763) seconds

Though there are multiple „message decoding errors” from LSP like

Failed to decode message: MessageDecodingError(code: LanguageServerProtocol.ErrorCode(rawValue: -32602), message: "missing expected parameter: string", id: nil, messageKind: LanguageServerProtocol.MessageDecodingError.MessageKind.notification)

Could this be it has something to do with the new Identifier initializers?

ahoppen · 2024-08-06T19:56:01Z

It crashed with

SourceKitLSPPackageTests.xctest: /home/build-user/swift/stdlib/public/Concurrency/TaskPrivate.h:803: void swift::AsyncTask::PrivateStorage::complete(AsyncTask *): Assertion `oldStatus.getInnermostRecord() == NULL' failed.

It’s a non-deterministic failure on Linux, though I haven’t seen it for months. Unrelated to your changes though.

DougGregor · 2024-08-06T20:13:27Z

@swift-ci please test Linux

ahoppen · 2024-08-06T20:14:28Z

+  func refersTo(_ lookedUpIdentifier: Identifier) -> Bool {
+    guard let identifier else { return false }
+    return identifier == lookedUpIdentifier


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.

ahoppen · 2024-08-06T20:49:09Z

+      }
+    }
+
+    for codeBlockItem in itemsWithoutNamedDecl {


I see. Could you add that explanation as a comment? Also, I think we could still have a single loop and then gather the type declarations and normal declarations and then concatenate them. Not sure if that’s cleaner.

I still think it’s a little odd because if you think about the following

func foo() { let x = 1 print(x) class x {} }

And then consider a declaration as a re-declaration if there already exists a declaration with the same name in the scope, then let x is the invalid redeclaration of class x where you would consider it the other way round. I don’t have a great suggestion for a solution though…

ahoppen · 2024-08-06T20:51:19Z

+    }
+
+    for codeBlockItem in itemsWithoutNamedDecl {
+      guard codeBlockItem.position < origin else { break }


isAccessible uses <= while this uses <, which is inconsistent.

I did think about whether the lookup position should always be compared with <= or < but I think <= is the better choice because when you do a jump-to-definition at the start of a variable definition, I would expect to go to the definition itself, same if you do it at the character one after the name start.

Ie.

let |f|oo = 1

should resolve to foo at both | positions.

ahoppen · 2024-08-06T20:56:53Z

+        result += introducedResults
+      } else {
+        // Extract new names from encountered node.
+        currentChunk += LookupName.getNames(


Ah, I see. Could you add that as a comment to the code?

DougGregor · 2024-08-06T22:44:40Z

Alright, let's go ahead and merge this!

MAJKFL · 2024-08-07T10:20:54Z

Thanks for merging! I included suggested changes by @ahoppen in #2782.

MAJKFL added 10 commits July 15, 2024 16:41

Format.

e15bfaa

Merge branch 'main'

a68f00a

Conflicts: Sources/SwiftLexicalLookup/ScopeImplementations.swift

Add guard support to file scope.

99b5294

Add implicit names and proper guard handling.

c31046a

Separate lookup state from config.

8b5fbd2

Create a new enum representing all possible implicit name cases. Simp…

93cb3ea

…lify LookupName.

Add suggested changes from the previous PR.

9945bf0

Make IntroducingToSequentialParentScopeSyntax and SequentialScope…

6759b05

… internal. Format.

Merge branch 'main'

2d109e5

Conflicts: Tests/SwiftLexicalLookupTest/NameLookupTests.swift

Add proper guard scope and implicit name lookup.

940b06e

MAJKFL requested review from ahoppen and bnbarham as code owners July 23, 2024 10:28

MAJKFL mentioned this pull request Jul 23, 2024

[SwiftLexicalLookup][GSoC] Add initial name lookup functionality #2719

Merged

MAJKFL added 4 commits July 25, 2024 09:29

Improve documentation.

c5988ed

Add identifier based name lookup.

0ef6a21

Fix handling inline variable declaration lists.

9a28d02

Merge branch 'guard-scope'

d547606

Conflicts: Sources/SwiftLexicalLookup/Configurations/LookupConfig.swift Sources/SwiftLexicalLookup/LookupName.swift

MAJKFL mentioned this pull request Jul 26, 2024

[SwiftLexicalLookup][GSoC] Fix Swift 5.8 compatibility and update documentation. #2755

Merged

MAJKFL added 7 commits July 29, 2024 18:33

Use IdentifierKind enum to represend different identifiers internally.

32fec70

Merge remote-tracking branch 'upstream/main' into guard-and-implicit-…

e96a324

…name-lookup

Fix merge artifacts.

0e5009a

Merge branch 'identifier-based-lookup' into guard-and-implicit-name-l…

166fdb7

…ookup

Fix merge artifacts.

a255fc9

Fix merge artifacts

fbf48bb

Merge changes from swiftlang#2755 and add Identifier based lookup.

dda31f9

MAJKFL requested a review from DougGregor as a code owner July 30, 2024 09:58

MAJKFL added 3 commits July 30, 2024 12:04

Revert "Merge changes from swiftlang#2755 and add Identifier based lo…

12a5380

…okup." This reverts commit dda31f9.

Merge branch 'guard-and-implicit-name-lookup'

b2e5748

Add internal _lookup function that passes state.

5705b96

ahoppen reviewed Jul 30, 2024

View reviewed changes

MAJKFL added 5 commits August 1, 2024 10:56

Add suggested changes.

539b1d7

Add catch clause implicit error. Fix declaration name hoisting in s…

5461b07

…equential scopes. Fix partitioning with non matching results from `guard` scopes.

Break early from sequential for loop when over lookup origin position.

059860a

Simplify lookup from guard ... else handling.

252d3be

Format.

be2e588

MAJKFL requested review from DougGregor and ahoppen August 1, 2024 13:16

ahoppen reviewed Aug 1, 2024

View reviewed changes

MAJKFL added 3 commits August 2, 2024 11:21

Add suggestions

01f2887

Format.

cf273f8

Improve documentation. Improve sequential scope test case.

c83b3c6

MAJKFL requested a review from ahoppen August 2, 2024 14:49

DougGregor approved these changes Aug 2, 2024

View reviewed changes

Remove optional chaining from identifier test.

fcaecc7

ahoppen reviewed Aug 6, 2024

View reviewed changes

DougGregor merged commit 2f0b28d into swiftlang:main Aug 6, 2024

MAJKFL mentioned this pull request Aug 7, 2024

[SwiftLexicalLookup][GSoC] Add switch, generic parameter and function scopes. #2782

Merged

MAJKFL mentioned this pull request Aug 8, 2024

[SwiftLexicalLookup][GSoC] New lookup method signature and Identifier initializer changes #2789

Merged


		import Foundation

		@_spi(Experimental) public struct LookupState {

Uh oh!

Conversation

MAJKFL commented Jul 23, 2024

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DougGregor commented Aug 1, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

DougGregor commented Aug 2, 2024

Uh oh!

DougGregor commented Aug 2, 2024

Uh oh!

DougGregor left a comment

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 Aug 2, 2024

Uh oh!

DougGregor commented Aug 2, 2024

Uh oh!

DougGregor commented Aug 2, 2024

Uh oh!

MAJKFL commented Aug 5, 2024

Uh oh!

ahoppen commented Aug 6, 2024

Uh oh!

DougGregor commented Aug 6, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!