SE-0258: Property Wrappers (second review)

anandabits · June 19, 2019, 1:16pm

Given how controversial wrapperValue has been I’m disappointed that something like this existed in the implementation but was not mentioned here. How were people supposed to discover it? I would have given the same feedback weeks ago had I been aware of this behavior.

If you want to get into philosophy we could debate here what “its” stored properties actually are. Consider the following:

struct Foo {
    private var x = 42
}

struct Bar {
    var foo: Foo
}

Here, foo.x is not directly a property of Bar, but it is part of the storage area of Bar. Nevertheless, the implementation of Bar does not have access to it because access control allows the author of Bar to hide its properties if desired. I don’t see a wrapper type hiding its storage using wrapperValue as being materially different from this. It is only the mechanism used to encapsulate that is different. There could be endless philosophical debate about this topic and that probably wouldn’t be productive, but I think this perspective should be represented and considered.

If we can’t find ways to support out-of-line initialization and non-synthesized encoding / decoding without exposing $$ and these are considered crucial use cases then perhaps that is a sign that wrapperValue is a bit too much magic.

I stand by my prior review. If you feel strongly that a wrapper should not be able to hide the actual storage then I am opposed to wrapperValue. The final design provides no encapsulation tools for library authors. It only provides very magical syntactic sugar with a lot more potential for confusion than usual.

I’ll restate that $foo.binidng is not that bad and is much more clear about what is going on. I know very experienced and talented programmers who spent a bunch of time scratching their heads trying to figure out how State turns into Binding. If you don’t want to go with the original version of this feature which has a lot less magic and offers encapsulation to wrapper authors maybe it should just be dropped.

anandabits · June 19, 2019, 1:18pm

Reading between the lines, it appears to be because Apple wants people to pass Binding references to @State properties in SwiftUI with the $foo syntax instead of having to write $foo.binding.

calebkleveter · June 19, 2019, 1:43pm

Could we make it possible to get the backing storage for a key-path?

struct Foo {
    @Lazy var bar: Int = 42
}

var foo = Foo()
var bar: Lazy<Int> = foo[storage: \.bar]

Lantua · June 19, 2019, 1:46pm

I find $foo.binding rather troubling.
Binding does provide a way to access data via dynamic member lookup, so if I’m to add binding to foo.bar, I would instead write $foo.binding.bar which still isn’t exactly obvious. Granted it may not be common, but it’s not something we should just ignore. Though I couldn’t come up with a satisfiable solution that changes only property wrapper itself, or the Binding.

I still think that wrapperValue/wrapperReplacement is a useful concept, but it should distance itself from the base storage.

anandabits · June 19, 2019, 2:03pm

I agree. I support the earlier version. I don't like the last minute change. If forced to choose between the version under review and removing wrapperValue from the proposal I would do the latter. I don't think the design that is under review is the right one. If the earlier version isn't going to fly then we need more design iteration on this concept.

Apple kind of boxed themselves into a corner by announcing a major framework that relies on language features that haven't been accepted yet. It's disappointing to see so much pressure to accept put on a feature that remains somewhat controversial. That isn't a good thing for Swift. Even if we need more time to iterate on wrapperValue it seems unlikely that we will get it.

Lantua · June 19, 2019, 2:30pm

That’s actually not a bad idea. The proposal without wrapperValue is still very much useful. Then we could pitch & add wrapperValue back semi-additively.

mattpolzin · June 19, 2019, 2:37pm

I agree that the desugaring implies the current behavior. I think there is a missed opportunity if you step away from the given name "property wrapper" and look at it how it reads in code, as simply a modification of a type's behavior. In general, my concern applies to property wrappers looking to conform to any protocol that declares initializers, but I will use Decodable as an example of such a protocol and I'll use Clamping as an example of a property wrapper.

If I was the author of Clamping, I would want the meaning of the following line to be "x is an integer between 0 and 255."

@Clamping(min: 0, max: 255) var x: Int

This meaning holds at face value, but now imagine that x is a property within a type that I want to decode from JSON sometimes but create from user input other times. Even worse, I am using an existing API to retrieve the data I am decoding so I do not have control over the JSON and therefore the minimum and maximum values of the Clamping construct cannot be decoded (they are not offered up by the API). I would want the "guarantee" of a clamped value to apply to all initialization paths, but it does not.

I have not come up with any brilliant ideas for future directions, so here is where things fall apart a bit. Imagine for a moment that there was a way to allow property wrappers to take control of all initialization paths rather than simply offering overloads that the compiler chooses under some situations.

Maybe one way to express this idea is that a type with the @propertyWrapper annotation is allowed to conform to protocols that require specific init definitions in a way that adds arguments to those initializers. Here's a strawman for an initializer that provides conformance of Clamping to Decodable:

@propertyWrapper
struct Clamping<V: Comparable>: Decodable {
  var value: V
  let min: V
  let max: V

  init(initialValue: V, min: V, max: V) { ... }

  // the following would satisfy `Decodable`
  init(from decoder: Decoder, min: V, max: V) { ... }
  ...
}

More generally, a tuple of arguments might look alright, too.

init(conformingInitArgs: (decoder: Decoder), min: V, max: V)
init(conformingInitArgs: (/* a tuple of labeled arguments conforming to a different protocol */), min: V, max: V)

Douglas_Gregor · June 19, 2019, 5:01pm

anandabits:

struct Foo {
    private var x = 42
}

struct Bar {
    var foo: Foo
}
Here, foo.x is not directly a property of Bar , but it is part of the storage area of Bar . Nevertheless, the implementation of Bar does not have access to it because access control allows the author of Bar to hide its properties if desired. I don’t see a wrapper type hiding its storage using wrapperValue as being materially different from this.

I don't see this as related at all, because it's not about whether Bar can reason about the storage of foo---it can't, because Foo is a distinct type. It's about whether Bar is able to create a Foo using the available API to initialize its own foo property.

Let's be clear: the stored property has to exist (it's fundamentally part of the compilation model), and it will be discoverable in metadata. When we get something like StoredPropertyIterable, you're going to be able to get key paths to these backing stored properties fairly easily, with names. Whatever name we choose for this backing storage, it's going to be exposed to the user at some point. My initial thought that it was an implementation detail---hence not documented explicitly---was factually incorrect, and (as I've noted before) wrong for real-world uses, led me to document this.

I emphasized the above sentence of yours, because I disagree with it. In the proposal, the backing storage property $$foo is always private---there is no mechanism to escalate its access, and I would oppose any change that did so. Having the stored property be private is encapsulation. It "hides" the stored property from outside of the type (and outside of the file) in any meaningful sense of the word. Obfuscating the name of the stored property (or pretending it has no name) doesn't give any more encapsulation tools to the library author, but it does take away their ability to reason about the stored properties of their own type.

In this proposal, the actual backing stored property is always private, which is Swift's strongest access-control mechanism to to achieve encapsulation. Property wrapper type authors can introduce wrapperValue to explicitly opt-in to having the $ property exposed further to provide wrapper-specific API via a custom type, providing per-wrapper control to cover the gamut of use cases.

I can understand the position that wrapperValue is too magic/unnecessary, even if I don't agree with it. However, I don't feel like the position that "having a name for the private stored property breaks encapsulation" is tenable.

Doug

blangmuir · June 19, 2019, 5:19pm

I have no strong opinions on $prefix vs. postfix$, but could you say more about the advantages you see for code-completion? If you're doing fuzzy matching they're nearly equivalent once you are typing the property name. If you aren't typing out the name, currently the '$' impacts the relative sort order for symbols in the same scope, but we could change that in other ways if we wanted.

John_McCall · June 19, 2019, 7:38pm

The proposal authors have informed me that they'd like to further revise this proposal in response to feedback. You are all welcome to continue discussing the proposal here in the meantime, but the authors may be less responsive while they're making those changes. The community will have a fair chance to respond to these revisions; we'll make a decision when they're ready whether to simply extend the review or treat it as a third review.

DevAndArtist · June 19, 2019, 8:02pm

I followed everything so far, but it's not clear to me what the proposal author will now revise from the given feedback. Is it possible to provide upfront a very short list of changes that will follow? That would be really helpful to keep track where the proposal evolves from the current discussion. Thank you in advance.

John_McCall · June 19, 2019, 8:06pm

I'll leave that to them; I'm not sure it's totally settled. I'll make sure there's a list of changes along with the revision, of course.

Jon_Shier · June 19, 2019, 8:32pm

While trying out my old NotificationObservable in the 5.1 toolchain, I ran into this crash:

Crash Output

CompileSwift normal x86_64 /Users/jshier/Desktop/WrapperTest/WrapperTest/ViewController.swift (in target: WrapperTest)
cd /Users/jshier/Desktop/WrapperTest
/Users/jshier/Library/Developer/Toolchains/swift-5.1-DEVELOPMENT-SNAPSHOT-2019-06-18-a.xctoolchain/usr/bin/swift -frontend -c -primary-file /Users/jshier/Desktop/WrapperTest/WrapperTest/ViewController.swift /Users/jshier/Desktop/WrapperTest/WrapperTest/AppDelegate.swift /Users/jshier/Desktop/WrapperTest/WrapperTest/SceneDelegate.swift -emit-module-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController~partial.swiftmodule -emit-module-doc-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController~partial.swiftdoc -serialize-diagnostics-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController.dia -emit-dependencies-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController.d -emit-reference-dependencies-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController.swiftdeps -target x86_64-apple-ios13.0-simulator -enable-objc-interop -sdk /Applications/Xcode-beta.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator13.0.sdk -I /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Products/Debug-iphonesimulator -F /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Products/Debug-iphonesimulator -enable-testing -g -module-cache-path /Users/jshier/Library/Developer/Xcode/DerivedData/ModuleCache.noindex -swift-version 5 -enforce-exclusivity=checked -Onone -D DEBUG -serialize-debugging-options -enable-anonymous-context-mangled-names -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/swift-overrides.hmap -Xcc -iquote -Xcc /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/WrapperTest-generated-files.hmap -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/WrapperTest-own-target-headers.hmap -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/WrapperTest-all-target-headers.hmap -Xcc -iquote -Xcc /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/WrapperTest-project-headers.hmap -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Products/Debug-iphonesimulator/include -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/DerivedSources-normal/x86_64 -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/DerivedSources/x86_64 -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/DerivedSources -Xcc -DDEBUG=1 -Xcc -working-directory/Users/jshier/Desktop/WrapperTest -module-name WrapperTest -o /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController.o -index-store-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Index/DataStore -index-system-modules

SIL verification failed: return value type does not match return type of function: functionResultType == instResultType
Verifying instruction:
%10 = apply %9(%3, %6, %8, %0) : $@convention(method) <τ_0_0> (@in Optional<τ_0_0>, @owned String, @owned NotificationCenter, @thick NotificationObservable<τ_0_0>.Type) -> @owned NotificationObservable<τ_0_0> // user: %12
-> return %10 : $NotificationObservable // id: %12
In function:
// variable initialization expression of ViewController.$string
sil [transparent] [ossa] @$s11WrapperTest14ViewControllerC7$string33_70E4070CA1C1352CB26B44847E78467BLLAA22NotificationObservableCySSGvpfi : $@convention(thin) () -> @owned Optional {
bb0:
%0 = metatype $@thick NotificationObservable.Type // user: %10
%1 = metatype $@thin Optional.Type
%2 = enum $Optional, #Optional.none!enumelt // user: %4
%3 = alloc_stack $Optional // users: %11, %10, %4
store %2 to [trivial] %3 : $*Optional // id: %4
// function_ref default argument 1 of NotificationObservable.init(initialValue:name:center:)
%5 = function_ref @$s11WrapperTest22NotificationObservableC12initialValue4name6centerACyxGxSg_SSSo20NSNotificationCenterCtcfcfA0_ : $@convention(thin) <τ_0_0> () -> @owned String // user: %6
%6 = apply %5() : $@convention(thin) <τ_0_0> () -> @owned String // user: %10
// function_ref default argument 2 of NotificationObservable.init(initialValue:name:center:)
%7 = function_ref @$s11WrapperTest22NotificationObservableC12initialValue4name6centerACyxGxSg_SSSo20NSNotificationCenterCtcfcfA1_ : $@convention(thin) <τ_0_0> () -> @owned NotificationCenter // user: %8
%8 = apply %7() : $@convention(thin) <τ_0_0> () -> @owned NotificationCenter // user: %10
// function_ref NotificationObservable.__allocating_init(initialValue:name:center:)
%9 = function_ref @$s11WrapperTest22NotificationObservableC12initialValue4name6centerACyxGxSg_SSSo20NSNotificationCenterCtcfC : $@convention(method) <τ_0_0> (@in Optional<τ_0_0>, @owned String, @owned NotificationCenter, @thick NotificationObservable<τ_0_0>.Type) -> @owned NotificationObservable<τ_0_0> // user: %10
%10 = apply %9(%3, %6, %8, %0) : $@convention(method) <τ_0_0> (@in Optional<τ_0_0>, @owned String, @owned NotificationCenter, @thick NotificationObservable<τ_0_0>.Type) -> @owned NotificationObservable<τ_0_0> // user: %12
dealloc_stack %3 : $*Optional // id: %11
return %10 : $NotificationObservable // id: %12
} // end sil function '$s11WrapperTest14ViewControllerC7$string33_70E4070CA1C1352CB26B44847E78467BLLAA22NotificationObservableCySSGvpfi'

Stack dump:
0. Program arguments: /Users/jshier/Library/Developer/Toolchains/swift-5.1-DEVELOPMENT-SNAPSHOT-2019-06-18-a.xctoolchain/usr/bin/swift -frontend -c -primary-file /Users/jshier/Desktop/WrapperTest/WrapperTest/ViewController.swift /Users/jshier/Desktop/WrapperTest/WrapperTest/AppDelegate.swift /Users/jshier/Desktop/WrapperTest/WrapperTest/SceneDelegate.swift -emit-module-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController~partial.swiftmodule -emit-module-doc-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController~partial.swiftdoc -serialize-diagnostics-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController.dia -emit-dependencies-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController.d -emit-reference-dependencies-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController.swiftdeps -target x86_64-apple-ios13.0-simulator -enable-objc-interop -sdk /Applications/Xcode-beta.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator13.0.sdk -I /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Products/Debug-iphonesimulator -F /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Products/Debug-iphonesimulator -enable-testing -g -module-cache-path /Users/jshier/Library/Developer/Xcode/DerivedData/ModuleCache.noindex -swift-version 5 -enforce-exclusivity=checked -Onone -D DEBUG -serialize-debugging-options -enable-anonymous-context-mangled-names -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/swift-overrides.hmap -Xcc -iquote -Xcc /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/WrapperTest-generated-files.hmap -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/WrapperTest-own-target-headers.hmap -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/WrapperTest-all-target-headers.hmap -Xcc -iquote -Xcc /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/WrapperTest-project-headers.hmap -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Products/Debug-iphonesimulator/include -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/DerivedSources-normal/x86_64 -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/DerivedSources/x86_64 -Xcc -I/Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/DerivedSources -Xcc -DDEBUG=1 -Xcc -working-directory/Users/jshier/Desktop/WrapperTest -module-name WrapperTest -o /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Build/Intermediates.noindex/WrapperTest.build/Debug-iphonesimulator/WrapperTest.build/Objects-normal/x86_64/ViewController.o -index-store-path /Users/jshier/Library/Developer/Xcode/DerivedData/WrapperTest-avoucjkhurbevxfgmwhvtqomftyo/Index/DataStore -index-system-modules

While silgen emitStoredPropertyInitialization SIL function "@$s11WrapperTest14ViewControllerC7$string33_70E4070CA1C1352CB26B44847E78467BLLAA22NotificationObservableCySSGvpfi".
for expression at [/Users/jshier/Desktop/WrapperTest/WrapperTest/ViewController.swift:12:6 - line:13:27] RangeText="NotificationObservable
var string: String? = "
While verifying SIL function "@$s11WrapperTest14ViewControllerC7$string33_70E4070CA1C1352CB26B44847E78467BLLAA22NotificationObservableCySSGvpfi".
for expression at [/Users/jshier/Desktop/WrapperTest/WrapperTest/ViewController.swift:12:6 - line:13:27] RangeText="NotificationObservable
var string: String? = "
0 swift 0x00000001108ec865 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37
1 swift 0x00000001108ebb55 llvm::sys::RunSignalHandlers() + 85
2 swift 0x00000001108ece48 SignalHandler(int) + 264
3 libsystem_platform.dylib 0x00007fff5b057b5d _sigtramp + 29
4 swift 0x0000000112730000 (anonymous namespace)::DarwinX86AsmBackend::getCompactUnwindRegNum(unsigned int) const::CU64BitRegs + 219474
5 libsystem_c.dylib 0x00007fff5af116a6 abort + 127
6 swift 0x000000010d8c0b38 (anonymous namespace)::SILVerifier::_require(bool, llvm::Twine const&, std::__1::function<void ()> const&) + 616
7 swift 0x000000010d8d41dc swift::SILInstructionVisitor<(anonymous namespace)::SILVerifier, void>::visit(swift::SILInstruction*) + 67836
8 swift 0x000000010d8c266c (anonymous namespace)::SILVerifier::visitSILBasicBlock(swift::SILBasicBlock*) + 1484
9 swift 0x000000010d8bd307 swift::SILFunction::verify(bool) const + 7447
10 swift 0x000000010d36ecad swift::Lowering::SILGenModule::postEmitFunction(swift::SILDeclRef, swift::SILFunction*) + 205
11 swift 0x000000010d37762f swift::Lowering::SILGenModule::emitStoredPropertyInitialization(swift::PatternBindingDecl*, unsigned int)::$_6::operator()(swift::SILFunction*) const + 271
12 swift 0x000000010d37149b swift::Lowering::SILGenModule::emitStoredPropertyInitialization(swift::PatternBindingDecl*, unsigned int) + 459
13 swift 0x000000010d4412cd (anonymous namespace)::SILGenType::emitType() + 381
14 swift 0x000000010d441149 swift::Lowering::SILGenModule::visitNominalTypeDecl(swift::NominalTypeDecl*) + 25
15 swift 0x000000010d373876 swift::Lowering::SILGenModule::emitSourceFile(swift::SourceFile*) + 822
16 swift 0x000000010d3746d5 swift::SILModule::constructSIL(swift::ModuleDecl*, swift::SILOptions&, swift::FileUnit*) + 293
17 swift 0x000000010d374bf6 swift::performSILGeneration(swift::FileUnit&, swift::SILOptions&) + 38
18 swift 0x000000010d099ae6 performCompile(swift::CompilerInstance&, swift::CompilerInvocation&, llvm::ArrayRef<char const*>, int&, swift::FrontendObserver*, swift::UnifiedStatsReporter*) + 8422
19 swift 0x000000010d096a32 swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 2978
20 swift 0x000000010d040768 main + 696
21 libdyld.dylib 0x00007fff5ae6c3d5 start + 1
error: Abort trap: 6

Since this is now integrated into master, should this be filed like a normal bug?

One other thing is that optional properties still don't get defaulted to nil, it has to be passed.

Ultimately, I'm trying to create an observable wrapper that doesn't require an initial value to be passed, as observed values don't really need one, and really shouldn't be optional either. I tried using an optional private storage property, but it still requires explicit initialization to nil. I seem recall both of these issues from the previous review or pitch (explicit nil required, default parameters for init(initialValue) not being accepted). Are these both intended limitations, or just limitations of the implementation on 5.1 toolchain?

masters3d · June 19, 2019, 8:59pm

Douglas_Gregor · June 19, 2019, 9:02pm

Here's the set of things I want to revise:

Give the private storage property a consistent name _<property name> regardless of whether wrapperValue is present. The leading underscore aligns with existing conventions. So the desugaring of @A public var foo: Int always has the basic form of:

private var _foo: A<Int>
public var foo: Int {
  get { _foo.wrappedValue }
  set { _foo.wrappedValue = newValue }
}

When wrapperValue is present, the $foo property becomes available and has the same access level as the original property foo, so when A has a wrapperValue (say it returns a Ref<Value>), the above example adds on:

public var $foo: Ref<Int> {
  get { _foo.wrapperValue }
  set { _foo.wrapperValue = newValue }
}

Rename wrapperValue to projectedValue, so that (1) it's not so close to wrappedValue and (2) gives us a specific term to use to talk about the $ property as the "projected property", a term that's been used a bit in the discourse here and describes the use cases well.

Doug

EDIT: I dropped in the names I'm planning to use, which is based on subsequent feedback on this thread. A revised proposal + implementation is forthcoming.

DevAndArtist · June 19, 2019, 9:19pm

With that spirit and with some bikeshedding alarm.

For the backing storage I think a lot of us already use the _foo syntax, so I'd vote for that.

private var _foo: A<Int>
public var foo: Int {
  get { _foo.wrappedValue }
  set { _foo.wrappedValue = newValue }
}

It's simple, everyone is familiar with it, and I don't see here that much of a problem except for types that cannot introduce a new stored property with such a name as it otherwise would collide. In such cases I think it should be fine if the user manually implements it and resolves the conflict.

@Douglas_Gregor the differentiation from your above list signals me that wrapperValue and $foo can be viewed as a standalone concept it's not a requirement to be part of @propertyWrapper feature set. We could generalize it into something standalone, which then would allow the user either specify manually a computed property with a $ prefix, or it's generated for the user in case property wrapper synthetization is already in use.

// allowed to be written manually or is synthesised, but must always return or set `wrapperValue`.
public var $foo: Ref<Int> {
  get { _foo.wrapperValue }
  set { _foo.wrapperValue = newValue }
}

As a standalone feature we could have a separate debate on how to call the feature and what wrapperValue should be renamed into instead.

Last but not least. If we would go somewhere along the above lines then we could also separate the access control of both _foo and $foo so it becomes more controllable/predictable even if it requires a little access control dance like public(wrapper) public(replacement) or whatever.

To be clear, that's just my opinion, nothing that try to force to happen. ;) So please view it as pure and honest feedback.

Douglas_Gregor · June 19, 2019, 9:34pm

DevAndArtist:

For the backing storage I think a lot of us already use the _foo syntax, so I'd vote for that.
private var _foo: A<Int>
public var foo: Int {
  get { _foo.wrappedValue }
  set { _foo.wrappedValue = newValue }
}
It's simple, everyone is familiar with it, and I don't see here that much of a problem except for types that cannot introduce a new stored property with such a name as it otherwise would collide. In such cases I think it should be fine if the user manually implements it resolves the conflict.

That's a reasonable position.

I'm not keen to split this part of the feature out of the property wrappers proposal. Use cases abound (e.g., @tanner0101 described some, SwiftUI has a bunch, more are in the proposal). Splitting out wrapperValue makes property wrappers no longer serve those myriad use cases... and makes this proposal a whole lot less compelling.

Doug

DevAndArtist · June 19, 2019, 9:35pm

Another super late bikeshedding :

Rename @propertyWrapper into @propertyStorage and wrappedValue into storedValue (access control through access_level(storage)).
Add @storageWrapper and keep wrapperValue (access control through access_level(wrapper))

@propertyStorage
@storageWrapper
struct IntAsStringStorage {
  let storedValue: Int
  var wrapperValue: String {
    "\(storedValue)"
  }

  init(initialValue: Int) {
    storedValue = initialValue
  }
}

@IntAsStringStorage 
public(wrapper)
internal(storage)
public var property: Int = 42

// translates to:
internal var _property = IntAsStringStorage(initialValue: 42)

public var $property: String {
  _property.wrapperValue
}

public var property: Int {
  _property.storedValue
}

Just thinking out loud.

Edit: I see some drawbacks on the access control about this design. Okay so forget it. However I kinda like if the final design of property wrappers had some simple and very predictable synthetization like shown in the translation part above.

Douglas_Gregor · June 19, 2019, 9:40pm

Please file a bug via bugs.swift.org. This backtrace looks really familiar; I might have already fixed the issue on master.

Hmmm, this is working for me against master:

@propertyWrapper
struct A<Value> {
  var wrappedValue: Value?

  init(initialValue: Value?) {
    wrappedValue = initialValue
  }
}

struct Foo {
  @A var foo: Int?
}

print(Foo())

You should be able to put a public init() into your property wrapper type. It will be called when no other initializer is provided. Here's an example that's working for me with master:

@propertyWrapper
struct A<Value> {
  private var value: Value?

  init() {
    value = nil
  }
  
  init(initialValue: Value) {
    value = initialValue
  }

  var wrappedValue: Value {
    get { value! }
    set { value = newValue }
  }
}

struct Foo {
  @A var foo: Int
}

print(Foo())

Doug

Douglas_Gregor · June 19, 2019, 9:42pm

I really don't want to complicate the access control story further here. The amended proposal has a very clean story:

Backing storage property (_foo?) is always private
$foo (when present) has the same access as foo

Doug