So I am working on an implementation for Pitch: didSet Semantics and I need to tweak the modify coroutine to yield a reference to the underlying storage and call didSet (if there is no willSet and didSet doesn't reference oldValue).
It was suggested that a short-term simple solution would be to change the ReadWriteImplKind
to be Modify
. However, changing the accessor's storage's StorageImplInfo
with ReadWriteImplKind::Modify
can only be done if the WriteImplKind
is Modify
or Set
and the ReadImplKind
is either Get
, Address
or Read
.
What would be the right combination of ReadImplKind
& WriteImplKind
here? cc @John_McCall
It seems like we yield a direct_to_impl
reference to the storage if ReadImplKind
is Get
and WriteImplKind
is Modify
(and ReadWriteImplKind
is Modify
as well):
(brace_stmt implicit range=[/Users/suyashsrijan/Desktop/test.swift:2:7 - line:2:7]
(yield_stmt implicit range=[/Users/suyashsrijan/Desktop/test.swift:2:7 - line:2:7]
(inout_expr implicit type='inout Int'
(member_ref_expr implicit type='@lvalue Int' decl=test.(file).Foo.value@/Users/suyashsrijan/Desktop/test.swift:2:7 direct_to_impl
(declref_expr implicit type='Foo' decl=test.(file).Foo.<anonymous>.self@/Users/suyashsrijan/Desktop/test.swift:2:7 function_ref=unapplied)))))))
for something like:
class Foo {
var value: Int {
didSet { print("Hello") }
}
init(value: Int) { self.value = value }
}
let foo = Foo(value: 0)
foo.value = 1
Oh, sorry. I should have said that it makes sense to make the ReadWriteImplKind
be StoredWithSimpleDidSet
or InheritedWithSimpleDidSet
, kindof like with the WriteImplKind
.
1 Like
Thanks for clarifying! Just to make sure I am on the right path, I am gonna ask a couple of questions:
- In
synthesizeCoroutineAccessorBody()
, if the modify accessor's ReadWriteImplKind
is StoredWithSimpleDidSet
or InheritedWithSimpleDidSet
AND the storage does not have willSet
AND there is a didSet
which doesn't refer to the oldValue
then we build the body of the accessor differently by simply yielding a reference to the storage and calling didSet
?
- You mentioned we need a
direct_to_impl
reference, I suppose that can be done by setting the storage's TargetImpl
to be Implementation
and let buildStorageReference()
handle it?
I think you shouldn’t set this impl kind unless you know that there’s no willSet and the didSet is simple. But yes, that’s the right approach. We should already have code to generate a well-typed l-value reference to the underlying storage that’s used in the synthesis of the getter and setter for observed storage, and that should take care of all the direct-to-impl details for you.
1 Like
Thanks! I think it seems to work based on the generated SIL:
// Foo.value.modify
sil hidden [transparent] @test.Foo.value.modify : Swift.Int : $@yield_once @convention(method) (@guaranteed Foo) -> @yields @inout Int {
bb0(%0 : $Foo):
debug_value %0 : $Foo, let, name "self", argno 1
%2 = ref_element_addr %0 : $Foo, #Foo.value
%3 = begin_access [modify] [dynamic] %2 : $*Int
yield %3 : $*Int, resume bb1, unwind bb2
bb1:
end_access %3 : $*Int
%6 = function_ref @test.Foo.value.didset : Swift.Int : $@convention(method) (@guaranteed Foo) -> ()
%7 = apply %6(%0) : $@convention(method) (@guaranteed Foo) -> ()
%8 = tuple ()
return %8 : $()
bb2:
end_access %3 : $*Int
unwind
}
I haven't added those ImplKinds as I can infer the presence/absence of the observers from the storage, but I'll add them now.
EDIT: oops, I think its still doing direct_to_storage
, not direct_to_impl
. Actually, when I call buildStorageReference()
and pass TargetImpl::Implementation
, then _modify
still calls the setter. When I pass TargetImpl::Storage
, then I get the SIL above. I am no SIL expert, so maybe that's not actually what the SIL should look like if everything is working properly.
Either TargetImpl::Storage
or TargetImpl::Super
is right, depending on whether the observers are added to an override to not; see how the setter is defined.
The ReadWriteImpl
thing matters not for code-generation of the _modify
accessor but for code-generation of a direct read-write access to the observed declaration.
1 Like
Thanks! I have updated the implementation to support this new feature.
1 Like