Asynchronous Catching Result initializer - #88465
Conversation
mattmassicotte
changed the title
|
Arg. The body of this new initializer implementation is currently crashing the compiler. |
ece048f to
339e8bc
Compare
|
I've narrowed the crash down slightly. Just making the existing initializer |
|
The following reduced example, included in enum PhonyResult<Success, Failure: Error> {
/// A success, storing a `Success` value.
case success(Success)
/// A failure, storing a `Failure` value.
case failure(Failure)
}
extension PhonyResult {
init(catching body: () throws(Failure) -> Success) async {
do {
self = .success(try body())
} catch {
self = .failure(error)
}
}
} |
|
Ok, this is now getting hard to believe. I have reduced this even further. Adding the following to the bottom of this same file produces the crash: |
ktoso
added
the
swift evolution pending discussion
|
I'll have a look |
|
@swift-ci please smoke test |
|
This may just be because this is in the core module which doesnt have any of the concurrency types -- move this extension into the concurrency module :) |
|
@swift-ci please smoke test |
4a36bce to
c73c6a5
Compare
|
@swift-ci please smoke test macOS |
|
I'm having trouble understanding what is actually failing in CI... |
Me too, it looks completely unrelated to your change. I'm going to kick off CI again |
|
@swift-ci please smoke test |
|
Don’t forget to add the new file to the CMakeList file in the Runtimes/Core/Concurrency folder |
|
@swift-ci please smoke test macOS |
|
The extension is missing availability, I'll propose a fix. Don't forget this comment from @NotTheNHK
|
|
@swift-ci please smoke test |
ktoso
left a comment
ktoso
left a comment
There was a problem hiding this comment.
LGTM, just needs to get through evolution
|
I made a follow up issue about the wasm side of this -- #89155 @MaxDesiatov there's a few things going on here with ~Copyable and async... I'm not sure what's intended to be supported here on wasm already? |
|
@swift-ci please smoke test |
|
Okey so we can't just |
|
@swift-ci please smoke test |
|
@swift-ci please test source compatibility |
|
I prepared the 6.4 pick as well #89157 |
|
Awesome thank you so much! |
|
@swift-ci please test windows |
Everything is intended to be supported. I'm not aware of any limitations that could prevent this proposal from being supported on Wasm. |
|
I see we need to look into it deeper then… I’ll catch up with you if I need any help with wasm then |
| #if os(WASI) | ||
| // FIXME: https://github.com/swiftlang/swift/issues/89155 | ||
| // wasi-wasm32 traps in Result<T, any Error>'s value-witness copy | ||
| // after returning from the new Result.init(catching:) async. | ||
| #else |
There was a problem hiding this comment.
I really would like this exclusion not to end up in the final PR. This test possibly uncovered some runtime or stdlib bug, but I don't see why this should be temporarily excluded otherwise during development. IMO, fixing that bug, whatever that is, to enable it consistently on all platforms should be a pre-requisite before merging.
There was a problem hiding this comment.
Yeah I'll look into fixing this
There was a problem hiding this comment.
The root cause and repro is tracked here, I'm looking into it #89320
Thanks for uncovering this bug!
There was a problem hiding this comment.
Seems #89416 was merged so let's see if this passes now
There was a problem hiding this comment.
Still failing on Wasm, I'll investigate
|
Blocked on #89320 |
|
Depends on #89416 |
swiftlang#89416 was merged, so this should work now
|
@swift-ci please smoke test |
This reverts commit a4ce8d7.
|
Going to land this, the wasm fix is taking to long -- it can land later. |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test macOS |
1 similar comment
|
@swift-ci please smoke test macOS |
|
@swift-ci please smoke test |
7 participants
Implementation of an async catching initializer for Result as part of swiftlang/swift-evolution#3234
Details in proposal.
Issues:
Original PRs:
Risk:
Testing:
I've added two tests to cover some of functionality that was being exercised for the synchronous cases. It seems sufficient to me, but I'd like feedback on this.
Would love for @ktoso to have a peek before finalizing.