Skip to content

[Compile Time Constant Extraction] Add extraction of tuples #62436

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

Jump to bottom
Merged
merged 1 commit into from
Dec 13, 2022
Merged

[Compile Time Constant Extraction] Add extraction of tuples #62436

merged 1 commit into from
Dec 13, 2022

Conversation

jPaolantonio
Copy link
Contributor

@jPaolantonio jPaolantonio commented Dec 7, 2022 •
edited
Loading

Statically extract tuples with named and unnamed elements.

This change also:

  • Refactors Expr -> CompileTimeValue into a recursive function to allow proper nesting of extracted values.
  • Refactors test extractions into named parameter by type (t for tuple, d for dictionary, etc.)

jPaolantonio
jPaolantonio commented Dec 7, 2022
View reviewed changes
lib/ConstExtract/ConstExtract.cpp Outdated
Comment on lines 170 to 139
for (auto elementExpr : tupleExpr->getElements()) {
elements.push_back({Optional<std::string>(), elementExpr->getType(),
extractCompileTimeValue(elementExpr)});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@artemcm - I wasn't able to exercise this codepath. Do you happen to know under what circumstances tupleExpr->hasElementNames() would be false?

I had thought let t1: (String, Bar) = ("foo", Bar()) would cause that, but instead the tuple expression's identifier is .empty().

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the parser sets things up with empty identifiers rather than no identifiers.
One example I see where hasElementNames() would return false is an empty tuple. And it's possible there is other places where the compiler itself synthesizes a tuple where labels are omitted. So it should be fine to keep this code-path as-is for safety.

lib/ConstExtract/ConstExtract.cpp Outdated
std::vector<TupleElement> elements;
if (tupleExpr->hasElementNames()) {
for (auto pair :
llvm::zip(tupleExpr->getElements(), tupleExpr->getElementNames())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an assumption here that when tupleExpr->hasElementNames() is true, getElements() and getElementNames() have the same number of items. I can add a check, but thought it was currently unnecessary.

@jPaolantonio
Copy link
Contributor Author

@swift-ci test

artemcm
artemcm approved these changes Dec 7, 2022
View reviewed changes
Copy link
Contributor

@artemcm artemcm left a comment

Choose a reason for hiding this comment

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

Cool!

@jPaolantonio
Copy link
Contributor Author

@swift-ci test macOS platform

@jPaolantonio jPaolantonio force-pushed the const-extraction-tuple branch from ac9b5bc to f6c6c26 Compare December 8, 2022 00:51
@jPaolantonio
Copy link
Contributor Author

@swift-ci test

@jPaolantonio
Copy link
Contributor Author

@swift-ci test Windows platform

@jPaolantonio jPaolantonio force-pushed the const-extraction-tuple branch from f6c6c26 to 10b2e14 Compare December 8, 2022 14:34
@jPaolantonio
Copy link
Contributor Author

@swift-ci test

@artemcm
Copy link
Contributor

artemcm commented Dec 8, 2022

@swift-ci test Windows platform

1 similar comment
@artemcm
Copy link
Contributor

artemcm commented Dec 8, 2022

@swift-ci test Windows platform

@jPaolantonio jPaolantonio force-pushed the const-extraction-tuple branch from 10b2e14 to bc5f64d Compare December 9, 2022 13:14
@jPaolantonio
Copy link
Contributor Author

@swift-ci test

@jPaolantonio jPaolantonio force-pushed the const-extraction-tuple branch from bc5f64d to db67e10 Compare December 9, 2022 14:34
@jPaolantonio
Copy link
Contributor Author

@swift-ci test

@jPaolantonio jPaolantonio force-pushed the const-extraction-tuple branch from db67e10 to 0177c12 Compare December 9, 2022 15:30
@jPaolantonio
Copy link
Contributor Author

@swift-ci test

@jPaolantonio jPaolantonio force-pushed the const-extraction-tuple branch from 0177c12 to c07c84b Compare December 9, 2022 17:30
@jPaolantonio
Copy link
Contributor Author

@swift-ci test

@jPaolantonio jPaolantonio force-pushed the const-extraction-tuple branch from c07c84b to 4ed167b Compare December 9, 2022 20:33
jPaolantonio
jPaolantonio commented Dec 9, 2022
View reviewed changes
lib/ConstExtract/ConstExtract.cpp
JSON.attributeArray("value", [&] {
for (auto TV : tupleValue->getElements()) {
JSON.object([&] {
if (auto Label = TV.Label) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@quinntaylor - Removed null labels

@jPaolantonio jPaolantonio force-pushed the const-extraction-tuple branch from 4ed167b to 0a1c65f Compare December 13, 2022 13:59
@jPaolantonio
Copy link
Contributor Author

@swift-ci test

@jPaolantonio jPaolantonio merged commit 0d666e2 into swiftlang:main Dec 13, 2022
@jPaolantonio jPaolantonio deleted the const-extraction-tuple branch December 13, 2022 18:39
meg-gupta pushed a commit to meg-gupta/swift that referenced this pull request Dec 16, 2022
@jPaolantonio @meg-gupta
[Compile Time Constant Extraction] Add extraction of tuples (swiftlan…
f17027e 
…g#62436)
ando-huang pushed a commit to ando-huang/swift that referenced this pull request Jan 2, 2023
@jPaolantonio @ando-huang
[Compile Time Constant Extraction] Add extraction of tuples (swiftlan…
484ef45 
…g#62436)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artemcm artemcm artemcm approved these changes

@quinntaylor quinntaylor Awaiting requested review from quinntaylor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jPaolantonio @artemcm