[Typechecker] Fix _modify for properties using a property wrapper #28216
Conversation
theblixguy
changed the title
|
@swift-ci please test |
|
@rjmccall Sure, I have updated the test! Does it look good to you? |
|
There's a few crashes due to:
I’ll take a look tomorrow. EDIT: Figured out - basically we weren't correctly synthesising |
Yes, the test looks good, thanks! |
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test |
|
Linux failure is unrelated (“no such module 'Foo'”). Let’s try again: @swift-ci please test Linux |
|
The only failure on macOS is |
|
Well, SwiftUI is a relatively heavy user of property wrappers, so if there's a problem that only shows up in some corner case, it's not too surprising that SwiftUI would exercise it. The crash is just parsing the |
|
Yeah, I was just a bit surprised to see a SIL crash, but I didn't realise that was expected (due to compiling the swiftinterface file). Anyway, this crash is happening because we have a If I remove |
|
Maybe we're not computing the |
|
Hmm it seems like not using |
|
Let's see if this works now: @swift-ci please test |
|
Nope, that didn't work (strange it worked locally, idk what happened there). Let me investigate more - my assumption is that the _modify should have the same mutating-ness as the setter, in case of |
…Value check" This reverts commit 48a5e86338c099d86f2d9f77e9ba14b4a37b3fe9.
|
In the SwiftUI swiftinterface file, we have @frozen @propertyWrapper @dynamicMemberLookup
public struct _Binding<Value> {
public var wrappedValue: Value {
get { fatalError() }
nonmutating set { }
}
public var projectedValue: _Binding<Value> {
get { fatalError() }
}
public subscript<Subject>(dynamicMember keyPath: WritableKeyPath<Value, Subject>) -> _Binding<Subject> {
get { fatalError() }
}
public init() {
fatalError()
}
}
public struct _ToggleStyleConfiguration {
@_Binding public var isOn: Bool
}which compiles. Looking at the generated swiftinterface file for this, I can see that the compiler is inserting a |
|
Yep, this seems to be happening when compiling the swiftinterface file only. I omitted the |
|
If there's something unusual about building this from a |
|
Yes, I cannot reproduce this with Swift code, but I can with a swiftinterface file and it’s interesting how specific the mutating-ness has to be in order to produce a crash. I don’t know much about swiftinterface files though, so I haven’t managed to figure out why this crashes yet. I think once you regenerate the file then things would work correctly, since it would have the modify accessor, so it only affects files that don’t have one and use this specific mutating-ness. I don’t know if there’s any other way to work around it in the meantime (I mean we could temporarily disable it for swiftinterface files), but perhaps someone who has worked on swiftinterface might be able to help. |
|
Okay, I think I have narrowed down the problem. It seems like the problem is that we're not computing the property wrapper mutability, because in the If I also add a check for a swiftinterface file, then it works correctly. So, I think the fix is to make sure we don't return |
… when parsing getter/setter from swiftinterface file
|
@swift-ci please test |
|
Hooray, that worked. Let’s run source compat just to be sure: @swift-ci please test source compatibility |
| target = TargetImpl::WrapperStorage; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
@theblixguy I don't understand why this is required.
Given a property like:
struct Foo {
@Wrap var bar: Int
}
Before this PR, the synthesized modify in SIL looked like:
Foo.bar.modify:
var tmp = call Foo.bar.getter(self)
yield &tmp
call Foo.bar.setter(tmp, &self)
where the Foo.text.getter calls Wrap.wrappedValue.getter, and Foo.text.setter calls Wrap.wrappvedValue.setter.
After this PR, it looks like:
Foo.bar.modify:
var tmp = call Wrap.wrappedValue.getter(self._text)
yield &tmp
call Wrap.wrappedValue.setter(tmp)
These are equivalent, but I thought it was cleaner that Foo.bar.modify calls into Foo.bar.getter/setter instead of doing something special for property wrappers. Why is it being done the latter way?
Follow up to https://forums.swift.org/t/property-wrapper-causing-excessive-array-copying
_modifyfor properties with an attached property wrapper. It was yielding the property itself, instead of thewrappedValue.ReadWriteImplKindwasn'tModifyso the synthesised_modifywas never used (although even if it was then transparent inlining would error out, due to the incorrect synthesis of the coroutine).Resolves SR-11762