SR-5289: Teach Mirror how to handle unowned/unmanaged references #28368
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test that we can reflect weak, unowned, and unmanaged references to both Swift and Obj-C types (including existential references to such types) that occur in both Swift class objects and in Swift structs. This includes the specific reported case (unowned reference to an Obj-C object).
TypeReferenceOwnership here is a bitmap indicating which of the various ownership modifiers (weak, unowned, unmanaged) were present on the field declaration. Making this a bitmap ensures that we can accurately represent the data that may have been provided but we have to be careful when we interpret the result. This changes the `isWeak`, `isUnowned`, `isUnmanged` checks to only return true if exactly one bit is set. Combinations of bits will not be accepted. Also, add `isStrong` to check for the default ownership with no modifiers.
This involves refactoring how we handle reference ownership when reflecting fields of struct and class objects: * Move FieldType into ReflectionMirror.mm FieldType is really just an internal implementation detail of this one source file, so it does not belong in an ABI header. * Use TypeReferenceOwnership directly to track field ownership This avoids bitwise copying of properties and localizes some of the knowledge about reference ownership * Generate a top-level "copyFieldContents" from ReferenceStorage.def Adding new ownership types to ReferenceStorage.def will now automatically produce calls to `copy*FieldContents` - failure to provide a suitable implementation will fail the build. * Implement `copyUnownedFieldContents` and `copyUnmanagedFieldContents`
|
@swift-ci Please test |
|
Build failed |
rjmccall
reviewed
Nov 20, 2019
mikeash
approved these changes
Nov 20, 2019
rjmccall
requested changes
Nov 20, 2019
|
Hey, my approval comes with an implicit "Do the stuff John says first." I won't dispute "mad fool" though. |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
@swift-ci Please test Linux Platform |
|
@rjmccall - I believe I've addressed your concerns. |
|
Build failed |
|
Seems I need to go debug this on Linux: |
rjmccall
reviewed
Nov 21, 2019
…alIn` Balanced calls here help ensure we use consistent memory management.
rjmccall
approved these changes
Nov 22, 2019
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
…n occasional failure
|
Linux CI is failing in my new test. Unfortunately, I've not been able to reproduce the failure locally on Ubuntu 16, 18, or macOS. So I've added some more detailed logging and verification to try to narrow the issue. |
|
@swift-ci Please smoke test Linux |
I had to fix this in order to test the Mirror support changes. Weak properties are optionals, and reflection packages things in Any containers, so it's hard to test reflecting weak properties without being able to cast the result back to the original type.
|
The tests were failing on Linux because the casts I was using in the test were broken. (They worked by accident on macOS because on macOS there's a last-resort path that defers the cast onto the Obj-C runtime.) So I implemented the necessary casts, including tests for the casts themselves. |
|
@swift-ci Please test |
tbkka
commented
Dec 6, 2019
In particular, a null type at this point indicates that the cast has already been checked and is known to be valid.
If this works, I should be able to factor out the common unwrap logic here.
This reverts commit 0e86b31. Trying to fix the casting inconsistencies is turning out to be a lot more complex than I thought. Let's roll back to just the Mirror fix so that can be merged before I take off on a lengthy tangent.
This reverts commit 0e86b31.
|
I've backed out the casting changes from this branch. They were not strictly necessary for the core bug here, and had too many repercussions. Instead, I'll find some other way to verify the functionality on Linux and pick up the casting changes elsewhere. |
|
For now, I've commented out the checks that relied on broken casts. If swift-ci passes, then I think this is (finally) good to go. |
|
@swift-ci Please test macOS |
|
@swift-ci Please test Linux |
|
Build failed |
|
Build failed |
|
Test failed on Swift in the OS bot (10.14.5) https://ci.swift.org/job/oss-swift_tools-RA_stdlib-RA-test-simulator-swift-in-the-os/976/console |
shahmishal
added a commit
that referenced
this pull request
Dec 17, 2019
tbkka
added a commit
to tbkka/swift
that referenced
this pull request
Dec 17, 2019
… references (swiftlang#28368)"" This reverts commit ca693ee.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds support to Mirror to correctly reflect unowned and unmanaged (aka
unowned(unsafe)) references, including tests that verify reflection of such fields containing references to ObjC or Swift objects (including via existentials).Resolves SR-5289.