Skip to content
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

Swift: upgrade to Swift 5.7.1 #11370

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

AlexDenisov
Copy link
Contributor

@AlexDenisov AlexDenisov commented Nov 22, 2022

No description provided.

if constexpr (std::is_base_of<swift::Decl, Locatable>::value) {
if (auto* decl = llvm::dyn_cast<swift::PatternBindingDecl>(locatable)) {
if (decl->getPatternList().empty()) {
return;
}
}
}
Copy link
Contributor

@redsun82 redsun82 Nov 25, 2022

Choose a reason for hiding this comment

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

I don't think you need a dyn_cast here because Locatable is already const swift::PatternBindingDecl here. And also, rather than not giving any location, we can use the (existing) start location as end location as well.
So maybe the following works as well:

Suggested change
if constexpr (std::is_base_of<swift::Decl, Locatable>::value) {
if (auto* decl = llvm::dyn_cast<swift::PatternBindingDecl>(locatable)) {
if (decl->getPatternList().empty()) {
return;
}
}
}
if constexpr (std::is_same_v<const swift::PatternBindingDecl, Locatable>) {
if (locatable->getPatternList().empty()) {
attachLocation(locatable->getStartLoc(), locatableLabel);
return;
}
}

Apart from that, this is a bit ewww 😅

Shouldn't we maybe consider patching this on PatternBindingDecl::getSourceRange() before prebuilding? We already have patches on the swift frontend library, although for the moment it's just for the build system, right? If we go for that we could also move there the patch I already introduced for headers.

Copy link
Contributor Author

@AlexDenisov AlexDenisov Nov 25, 2022

Choose a reason for hiding this comment

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

Yeah, patching the source code makes more sense, agreed, I'll clean it up when we resolve all the rest of the issues if it doesn't cause too much friction.

The suggested change won't work unfortunately as the crash is coming right from Decl::getStart/EndLoc which subsequently calls getSourceRange.

Copy link
Contributor

@redsun82 redsun82 Nov 25, 2022

Choose a reason for hiding this comment

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

ah, you are right, this code is called in fetchLabel that might be called with a less specific type than the translation actually uses. If we moved the location attachment to createEntry instead, we would always get the exact specific type and we would be able to do this kind of tweaks to locations on a per-type basis. But we can leave that for another PR

@redsun82
Copy link
Contributor

redsun82 commented Nov 25, 2022

@AlexDenisov could you try applying

diff --git a/swift/actions/run-integration-tests/action.yml b/swift/actions/run-integration-tests/action.yml
index 9325fb8b11..2ff09ae9c0 100644
--- a/swift/actions/run-integration-tests/action.yml
+++ b/swift/actions/run-integration-tests/action.yml
@@ -13,7 +13,7 @@ runs:
     - uses: actions/setup-python@v4
       with:
         python-version-file: 'swift/.python-version'
-    - uses: swift-actions/setup-swift@v1
+    - uses: redsun82/setup-swift@63ad25fa169bb1e89d8aa67036007d6f3078e9f6
       with:
         swift-version: "${{steps.get_swift_version.outputs.version}}"
     - uses: ./.github/actions/fetch-codeql

This might make the setup swift work with 5.7.1 in the integration tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants