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
base: main
Are you sure you want to change the base?
Conversation
995d04b
to
c1fb9ac
Compare
| if constexpr (std::is_base_of<swift::Decl, Locatable>::value) { | ||
| if (auto* decl = llvm::dyn_cast<swift::PatternBindingDecl>(locatable)) { | ||
| if (decl->getPatternList().empty()) { | ||
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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:
| 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
@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-codeqlThis might make the setup swift work with 5.7.1 in the integration tests. |
No description provided.