Add implicit conversions and casts from T:Hashable <-> AnyHashable. #4022
Conversation
|
@swift-ci Please smoke test and merge. |
|
@jckarter Mind reviewing the SILGen and runtime parts of this? |
|
Passed tests on a separate Linux build. |
|
Type checker parts look good! |
rjmccall
force-pushed
the
anyhashable-conversions
branch
from
eb875a0 to
5d479e7
Compare
|
@swift-ci Please smoke test. |
| // CHECK-NEXT: Optional(AnyHashable(5)) | ||
|
|
||
| print("End") | ||
| // CHECK-NEXT: End |
There was a problem hiding this comment.
Could you use StdlibUnittest?
There was a problem hiding this comment.
Could you add a test for a cast from an existential that contains AnyHashable to an unboxed type?
Also, for a cast from an AnyHashable to a protocol existential of the unwrapped value?
There was a problem hiding this comment.
It would be good to do something like the logic in test/1_stdlib/BridgeIdAsAny.swift.gyb, to test all the possible cases of compounded existential/AnyHashable erasure:
https://github.com/apple/swift/blob/master/test/1_stdlib/BridgeIdAsAny.swift.gyb#L177
|
Does the cast optimizer need to be adjusted for this change? |
| func _anyHashableDownCastConditionalIndirect<T>( | ||
| _ value: UnsafePointer<AnyHashable>, | ||
| _ target: UnsafeMutablePointer<T>) -> Bool { | ||
| return value.pointee._downCastConditional(into: target) |
There was a problem hiding this comment.
Why not use public entry points:
if let result = value.pointee.base as? T {
target.pointee.initialize(to: result)
return true
}
return falseThere was a problem hiding this comment.
I see, this way you are potentially saving a heap box allocation for the container returned from value.pointee. If that is the important part here, I would appreciate a comment about this!
|
The cast optimizer uses the logic in DynamicCasts.cpp. |
|
Looks good, thanks for taking care of this @rjmccall! I agree with @gribozavr that it'd be good to have more exhaustive tests for all the casting stuff. |
|
Hey John. If the cast optimizer uses logic in DynamicCasts.cpp, please add specific sil tests that exercise the code in the cast optimizer. We have run into many issues with the cast optimizer not being updated properly in the face of these sorts of changes. |
|
@swift-ci please smoke test Linux |
|
@gottesmm The test is run under -O as well as not, but I'll also add specific cast-optimizer tests. |
|
To be clear, the existing test exercises the code in DynamicCasts.cpp and fails if it's wrong. |
|
The Linux failure is the same one that's been failing for a while in LLDB (which I just reverted), so let's give it one more go... I'd like to get this in so we can more easily test the combination of this change with my inference of existential element types for collection literals. |
|
@swift-ci please smoke test Linux |
rjmccall
force-pushed
the
anyhashable-conversions
branch
from
5d479e7 to
f5d941b
Compare
|
Okay, addressed code review comments. @swift-ci Please smoke test and merge. |
|
Oh, I forgot to add specific cast-optimizer tests; I'll write that up separately. |
rjmccall
force-pushed
the
anyhashable-conversions
branch
from
f5d941b to
a6e1e87
Compare
|
Nah, added them to this one. Done for now. @swift-ci Please smoke test and merge. |
|
Awesome! Once this is in, I'll write some integration tests to make sure dictionary literals work properly with AnyHashable. |
|
@rjmccall Thanks! |
What's in this pull request?
Resolved bug number: (SR-)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
A smoke test on macOS does the following:
device standard libraries are not built.
version of these tests are not run.
A smoke test on Linux does the following:
tests are not run.
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.
rdar://27615802