Skip to content

[SwiftLexicalLookup] Composite names #2905

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 7 commits into from
Dec 6, 2024
Merged

Conversation

MAJKFL
Copy link
Contributor

@MAJKFL MAJKFL commented Dec 2, 2024

This PR introduces notion of composite names to SwiftLexicalLookup. In Swift, it’s possible to have a switch case introducing one name for several enum cases:

switch X {
case .x(let a), .y(let a):
  print(a) // Refers to both introduced names above
}

ASTScope currently deals with this special case by just introducing the first name from the list. Composite names are supposed to instead merge all names with the same semantic meaning into one, unified name representation. It stores an array of related names and, to match the behavior of the compiler, a composite name is represented by the first name.

Combined with some additional minor fixes included in this PR, I was able to finally perform entire Swift compilation with fully enabled validation! There still seem to be some discrepancies when running entire test suite with lit, but I’m happy to see the library finally maturing.

@MAJKFL
Copy link
Contributor Author

MAJKFL commented Dec 2, 2024

@swift-ci Please test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Nice

case .x(let 1️⃣a, let 2️⃣b), .y(let 3️⃣a, let 4️⃣b):
print(5️⃣x)
case .z(let 7️⃣a), .smth(let 8️⃣a):
print(9️⃣x)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t this be print(a)? x is never defined in this test case.

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 purposefully called it x to further indicate that we are not performing name matching in this case. We use useNilAsTheParameter: true for that. This test is supposed to check if we properly partition names equivalent names (like in the first case here).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I missed that and thought that we were looking up x, not that we were collecting all names available at that location. The test makes sense now 😄

@MAJKFL
Copy link
Contributor Author

MAJKFL commented Dec 4, 2024

@swift-ci Please test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏽

@ahoppen
Copy link
Member

ahoppen commented Dec 4, 2024

@swift-ci Please test Windows

1 similar comment
@DougGregor
Copy link
Member

@swift-ci Please test Windows

@MAJKFL
Copy link
Contributor Author

MAJKFL commented Dec 4, 2024

@swift-ci Please test

@MAJKFL
Copy link
Contributor Author

MAJKFL commented Dec 5, 2024

@swift-ci Please test Windows

2 similar comments
@DougGregor
Copy link
Member

@swift-ci Please test Windows

@DougGregor
Copy link
Member

@swift-ci Please test Windows

@DougGregor
Copy link
Member

@swift-ci please test platform Windows

1 similar comment
@DougGregor
Copy link
Member

@swift-ci please test platform Windows

@DougGregor
Copy link
Member

@swift-ci please test Windows

@MAJKFL MAJKFL merged commit b479382 into swiftlang:main Dec 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants