[SwiftLexicalLookup] New unqualified lookup implementation validation #77140

MAJKFL · 2024-10-21T12:02:15Z

This PR introduces a function used to validate output of SwiftLexicalLookup lookup against ASTScope unqualified lookup. It’s part of the GSoC project I worked on with @DougGregor. The goal is to be able to automatically compare outputs of the two implementations, run it for large amounts of source files and fix any discrepancies to bring the new implementation closer to the current one. To perform validation, use -enable-experimental-feature UnqualifiedLookupValidation.

Example output of a failed validation for a file from the Swift standard library:

-----> Lookup started at: 1706:7 ("debugPrint") finishInSequentialScope: false
     |           ASTScope           |      SwiftLexicalLookup
> ✅ |          k 1699:10           |          k 1699:10
> ✅ |          v 1699:13           |          v 1699:13
> ❌ |         first 1698:9         |         type 1690:18
> ❌ |        result 1697:9         |         self 1689:17
> ❌ |         type 1690:18         |         ↕️ V 1689:49
> ❌ |         self 1689:17         |         ↕️ K 1689:46
> ℹ️ | Omitted SwiftLexicalLookup name: Self 1683:1
> ❌ |          V 1689:49           |      Look memb: 1684:11
> ❌ |          K 1689:46           |            -----
> ℹ️ | Omitted ASTScope name: K 1689:46
> ℹ️ | Omitted ASTScope name: V 1689:49
> ❌ | End here: Look memb: 1684:11 |            -----

This PR depends on PR 2883 in swift-syntax that brings #if handling through SwiftIfConfig to SwiftLexicalLookup.

slavapestov

I'm really excited about SwiftLexicalLookup, I just have some minor nitpicks

slavapestov · 2024-10-21T20:46:49Z

include/swift/AST/ASTBridging.h

+  BridgedConsumedLookupResult(swift::Identifier name,
+                              swift::SourceLoc sourceLoc,
+                              SwiftInt flag
+                              ) : Name(BridgedIdentifier(name)),


Nit: we typically put the ) on the same line and : on the following. @gottesmm can also help you set up clang-format if you'd rather fix stylistic issues automatically

Thank you for pointing this out! I manually run clang-format now for this part of the file. Is there maybe a way to automate this process and run with every commit or just run it on my changes?

slavapestov · 2024-10-21T20:47:25Z

lib/AST/ASTScope.cpp

+    if (auto *extDecl = dyn_cast<ExtensionDecl>(scopeDC)) {
+      recordedElements.push_back(BridgedConsumedLookupResult(
+          Identifier(), extDecl->getExtendedTypeRepr()->getLoc(),
+          0b10 + result));


Perhaps the magic constant should be named, or you could use a bitfield or something

This is a good idea. I documented those on the Swift side, but forgot about explaining them here. I moved 0b10 to a constant shouldLookInMembers and also renamed result to endOfLookup.

slavapestov · 2024-10-21T20:48:52Z

lib/ASTGen/Sources/ASTGen/LexicalLookup.swift

+        } else if parent.is(FunctionDeclSyntax.self) || parent.is(SubscriptDeclSyntax.self)
+          || result.scope.range.contains(lookupToken.position)
+        {
+          // If a result from function generic parameter clause or lookup started within it, reverse introduced names.


I guess the old code has some funky behaviors :)

Yes, we've encountered some really fun behavior of ASTScope when working on the library.

This particular case happens when looking up generic parameter from generic parameter clause which is part of a function or subscript or the lookup is started within the clause:

func foo<A, B: Int>(a: A, b: B) { print(a, b) }

ASTScope for some reason decides to first introduce generic parameters in wrong order just to introduce them correctly right afterwards :). Interestingly, similar behavior doesn't happen inside nominal type declarations.

-----> Lookup started at: 2:3 ("print") finishInSequentialScope: false | ASTScope | SwiftLexicalLookup > ✅ | a 1:21 | a 1:21 > ✅ | b 1:27 | b 1:27 > ✅ | B 1:13 | ↕️ B 1:13 > ✅ | A 1:10 | ↕️ A 1:10 > ℹ️ | Omitted ASTScope name: A 1:10 > ℹ️ | Omitted ASTScope name: B 1:13

With those comments, this validation function slowly becomes a documentation of "weird" ASTScope behavior. It might be necessary to revisit some of those heuristics in the future if it turns out there's a valid reason for this behavior and we should actually adjust SwiftLexicalLookup. That's why I tried to be more explicit in this part of the validation algorithm.

lib/ASTGen/Sources/ASTGen/LexicalLookup.swift

… in `LoggingASTScopeDeclConsumer`. Format `BridgedConsumedLookupResult`.

DougGregor · 2024-10-30T15:48:43Z

swiftlang/swift-syntax#2883

@swift-ci please build toolchain

DougGregor · 2024-10-30T15:49:47Z

swiftlang/swift-syntax#2883

@swift-ci please smoke test

MAJKFL · 2024-11-01T19:48:04Z

swiftlang/swift-syntax#2883

@swift-ci please build toolchain

MAJKFL · 2024-11-13T14:54:49Z

@swift-ci Please test

MAJKFL · 2024-11-13T14:55:16Z

@swift-ci Please smoke test

…onfig`.

…MAJKFL/swift into swift-lexical-lookup-validation

include/swift/AST/ASTBridging.h

…ty with ARM windows.

MAJKFL · 2024-11-13T16:52:22Z

swiftlang/swift-syntax#2883

@swift-ci Please smoke test

DougGregor · 2024-11-13T16:52:46Z

swiftlang/swift-syntax#2883

@swift-ci Please smoke test Windows

…p-validation

MAJKFL · 2024-11-14T12:23:22Z

@swift-ci Please smoke test

MAJKFL · 2024-11-14T12:24:01Z

@swift-ci Please smoke test Windows

…okupResult`.

MAJKFL · 2024-11-15T21:06:30Z

@swift-ci Please smoke test

DougGregor · 2024-11-20T15:57:24Z

@swift-ci please test Windows

DougGregor

This is excellent. I have one request, though: can you add a test that demonstrates that this experimental mode is working? It doesn't have to be comprehensive, but it should have some nontrivial Swift code in it and use %target-typecheck-verify-swift -enable-experimental-feature UnqualifiedLookupValidation. That will make sure this functionality doesn't regress based on other changes to the compiler code base.

Over time, we can add this experimental feature to more places to improve our testing coverage.

…guredRegions` to unqualified lookup validation.

…MAJKFL/swift into swift-lexical-lookup-validation

MAJKFL · 2024-11-26T08:39:03Z

@swift-ci Please smoke test

…okup validation test.

MAJKFL · 2024-12-05T09:36:57Z

@swift-ci Please smoke test

…ftlexicalLookup` test.

MAJKFL · 2024-12-13T11:42:37Z

@swift-ci Please smoke test

MAJKFL added 2 commits October 9, 2024 12:37

Merge remote-tracking branch 'upstream/main'

04f5f33

Add SwiftLexicalLookup validation.

1d94844

MAJKFL requested review from ahoppen, bnbarham, CodaFi, hamishknight, rintaro, hborla, slavapestov, xedin and DougGregor as code owners October 21, 2024 12:02

MAJKFL mentioned this pull request Oct 21, 2024

Wrong result order in ASTScope::unqualifiedLookup for macro declarations. #77141

Open

slavapestov reviewed Oct 21, 2024

View reviewed changes

Switch to integer division inaddPaddingUpTo. Rename magic constants…

2cc0088

… in `LoggingASTScopeDeclConsumer`. Format `BridgedConsumedLookupResult`.

MAJKFL requested a review from slavapestov October 23, 2024 10:39

Fix validation of names with end flag. Fix missing lib in cmake file.

ba004b6

Merge branch 'main' into swift-lexical-lookup-validation

4b544e9

MAJKFL added 2 commits November 13, 2024 17:43

Use ConfiguredRegions instead of BuildConfiguration with `LookupC…

8fc6951

…onfig`.

Merge branch 'swift-lexical-lookup-validation' of https://github.com/…

3ef2312

…MAJKFL/swift into swift-lexical-lookup-validation

DougGregor reviewed Nov 13, 2024

View reviewed changes

include/swift/AST/ASTBridging.h Outdated Show resolved Hide resolved

Add a dummy BridgedConsumedLookupResult constructor for compatibili…

f4a5069

…ty with ARM windows.

MAJKFL added 2 commits November 14, 2024 10:38

Merge remote-tracking branch 'upstream/main' into swift-lexical-looku…

34c90f1

…p-validation

Add casting of exported source file syntax to SourceFileSyntax.

d724230

MAJKFL added 2 commits November 15, 2024 22:01

Use BRIDGED_INLINE to avoid dummy constructor in `BridgedConsumedLo…

bda3c7f

…okupResult`.

Merge branch 'main' into swift-lexical-lookup-validation

5b39ba6

DougGregor reviewed Nov 20, 2024

View reviewed changes

MAJKFL added 3 commits November 26, 2024 09:34

Add a basic SwiftLexicalLookup validation test. Add caching of `Confi…

024f630

…guredRegions` to unqualified lookup validation.

Merge branch 'swift-lexical-lookup-validation' of https://github.com/…

0371783

…MAJKFL/swift into swift-lexical-lookup-validation

Merge branch 'main' into swift-lexical-lookup-validation

866f1c1

Add REQUIRES: UnqualifiedLookupValidation comment to unqualified lo…

0ba7a5e

…okup validation test.

Add REQUIRES: swift_feature_UnqualifiedLookupValidation to the `Swi…

39e4d15

…ftlexicalLookup` test.

DougGregor approved these changes Dec 18, 2024

View reviewed changes

MAJKFL merged commit 5dbe202 into swiftlang:main Dec 18, 2024
3 checks passed

bnbarham mentioned this pull request Dec 18, 2024

Revert "[SwiftLexicalLookup] New unqualified lookup implementation validation" #78280

Merged

[SwiftLexicalLookup] New unqualified lookup implementation validation #77140

[SwiftLexicalLookup] New unqualified lookup implementation validation #77140

Uh oh!

Conversation

MAJKFL commented Oct 21, 2024

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

slavapestov Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

MAJKFL Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

slavapestov Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

MAJKFL Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

slavapestov Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

MAJKFL Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DougGregor commented Oct 30, 2024

Uh oh!

DougGregor commented Oct 30, 2024

Uh oh!

MAJKFL commented Nov 1, 2024

Uh oh!

MAJKFL commented Nov 13, 2024

Uh oh!

MAJKFL commented Nov 13, 2024

Uh oh!

Uh oh!

MAJKFL commented Nov 13, 2024

Uh oh!

DougGregor commented Nov 13, 2024

Uh oh!

MAJKFL commented Nov 14, 2024

Uh oh!

MAJKFL commented Nov 14, 2024

Uh oh!

MAJKFL commented Nov 15, 2024

Uh oh!

DougGregor commented Nov 20, 2024

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

MAJKFL commented Nov 26, 2024

Uh oh!

MAJKFL commented Dec 5, 2024

Uh oh!

MAJKFL commented Dec 13, 2024

Uh oh!

Uh oh!

Uh oh!