Annotate Foundation overlay with __shared/__owned #19143
@swift-ci Please test macOS
@swift-ci Please smoke test Linux
@eeckstein As far as I'm aware, our actual static bridging functions (_forceBridgeFromObjectiveC, _conditionallyBridgeFromObjectiveC, and _unconditionallyBridgeFromObjectiveC) should not require annotations as:
- They're
static funcs which have no local storage (or write out toinout-params) - Since they call into the shared reference variants changed above, the default for
funcs applies here — the params should not be owned
Is my understanding here correct?
Sorry, why is this specific to reference types? cc @airspeedswift
@jrose-apple Sorry, have I missed something here? Is this not inherently related to values which require reference counting?
"Values which require reference counting" isn't quite the same as "reference types". String and Array and Data all require reference counting too, right?
Ah, therein lies my misunderstanding about what was necessary here. I'll re-audit for those types as well.
Closing for now.
Build failed
Swift Test OS X Platform
Git Sha - 402457b
Made another pass here to add annotations to cover other non-trivial types. Will be doing another pass to confirm there's nothing I missed.
@swift-ci Please test macOS
@swift-ci Please smoke test Linux
Build failed
Swift Test OS X Platform
Git Sha - 402457b
@jrose-apple @eeckstein Do the changes here look correct? Does anything stand out as being out of place?
| 157 | 157 | } | |
| 158 | 158 | ||
| 159 | mutating func append(contentsOf other: [Int]) { | ||
| 159 | mutating func append(contentsOf other: __owned [Int]) { |
airspeedswift6 years agothis probably isn't necessary since it's just [Int]
eeckstein6 years agoNo, Array of Int is not a trivial type, i.e. involves reference counting of the array buffer.
jrose-apple6 years agoRight, but IndexPath doesn't gain anything by taking the array owned, since it's not going to be able to use that buffer for storage, and the type isn't generic.
eeckstein6 years agoYou are right, assuming that the case where self is empty and size of other > 2 is not the common case, it's better to pass other as guaranteed.
(I misread airspeedswift's comment: I though he meant that it's not worth annotating because of the parameter type).
Can you explain what the principles are for what is and isn't annotated? Since you're explicitly writing both __shared and __owned, I'd expect it to be
- every (non-trivial) parameter
- of all
public/@usableFromInlinefunctions and initializers
and perhaps some of the non-public stuff as optimization hints. But I see public functions where that hasn't been done, such as https://github.com/apple/swift/blob/9f042bc7c3452307b7929f5b76db5261b623d79a/stdlib/public/SDK/Foundation/Calendar.swift#L507.
| 107 | 107 | // MARK: Bridging | |
| 108 | 108 | ||
| 109 | fileprivate init(reference : NSCalendar) { | ||
| 109 | fileprivate init(reference : __shared NSCalendar) { |
jrose-apple6 years agoThis one should be owned, no? Since you're immediately going to store it.
jrose-apple6 years agoAlthough it's not a public entry point anyway, so it doesn't really matter.
itaiferber6 years ago_MutableHandle makes a copy of the reference given to it so we never store the original — I've propagated __shared out to every init(reference:) which uses this, per @eeckstein's recommendation.
| 962 | 962 | /// Initializes `self` by referencing the given dictionary container in the given encoder. | |
| 963 | 963 | fileprivate init(referencing encoder: _JSONEncoder, | |
| 964 | key: CodingKey, convertedKey: CodingKey, wrapping dictionary: NSMutableDictionary) { | ||
| 964 | key: CodingKey, convertedKey: __shared CodingKey, wrapping dictionary: NSMutableDictionary) { |
jrose-apple6 years agoI don't see why only the one parameter changed here.
itaiferber6 years agoThis parameter is not stored; all others are. Based on your comments, it sounds to me like I've misunderstood something critical here. Let me start an email thread so we can sort this out.
| 1179 | 1179 | /// - parameter options: Options for the read operation. Default value is `[]`. | |
| 1180 | 1180 | /// - throws: An error in the Cocoa domain, if `url` cannot be read. | |
| 1181 | public init(contentsOf url: URL, options: Data.ReadingOptions = []) throws { | ||
| 1181 | public init(contentsOf url: __shared URL, options: __shared Data.ReadingOptions = []) throws { |
jrose-apple6 years agoData.ReadingOptions is an ObjC enum, right? So it shouldn't matter what ownership you use.
itaiferber6 years agoSure — some of the changes here were mechanical, so I can pull this one out. I'm surprised the compiler didn't produce a warning/error in this case.
jrose-apple6 years agoWe haven't really talked about what form that'll take yet, but yeah, that's a reasonable idea. (You wouldn't necessarily want to do it to everything that's trivial, because a new version of the library might make it non-trivial, but that's not going to happen to an @objc enum.)
@jrose-apple Sure — based on the guidelines I was given, I went ahead and annotated every function parameter which deviated from the default rules that I understand are in place (__shared is assumed for all function parameters by default, __owned is assumed for all init parameters by default). Anything not annotated is meant to assume the default.
If it would be more helpful (or even necessary) to annotate all parameters regardless of the default, then I'm willing to go through and do that. Similarly, if I've misunderstood the guidelines (see rdar://problem/34989766 for discussion between between me and @eeckstein), happy to be corrected.
If it would be more helpful (or even necessary) to annotate all parameters regardless of the default
No, that's not necessary
I feel silly now. I didn't realize you were just annotating divergences from the default. Sorry for jumping in!
@swift-ci Please test macOS
@swift-ci Please smoke test Linux
Build failed
Swift Test OS X Platform
Git Sha - 9f042bc
Latest commit removes the unnecessary __shared annotations on Data.ReadingOptions and friends (which are Obj-C enums). If tests are green I'll merge.
@jrose-apple No worries at all, BTW — thanks for taking the time to review! Thanks @eeckstein for the help, too.
Login to write a write a comment.
What's in this pull request?
Annotates portions of the Foundation overlay with the appropriate
__shared/__ownedannotations following the patterns we use (resolving rdar://problem/34989766). I looked at every declaredfuncandinitwithin our overlay and annotated all methods as appropriate. ~~~A search of all storedvars shows no necessary changes AFAICT as we don't expose reference types from our Swift APIs.~~~ (Sincevartypes cannot be annotated for ownership, there's nothing for us to do in the cases wherevars should be__shared.)Overall the changes mostly revolve around bridging:
copy()the passedNStype before holding onto it as a reference to preserve value semantics, and in those cases, the parameter should be__sharedas we don't hold on to the original. (AFAIK,copy()should help enforce the right refcounting in the case wherecopy()returnsself.)NStype out of the Swift type via bridging (which copies as well) and hold on to theNStypeThere are a few cases where
initparameters go unused (such as inString.init?(contentsOf:)), and some cases aroundKeyPathwhere we appear to not store the givenKeyPathbut it actually ends up stored in a global table.