[SILGen] Emit default arg generator for stored property kind #23648
Conversation
|
The approach looks good to me but I think Slava should check the details. Thank you, Alejandro! |
|
I'm questioning whether we need a special default arg kind anymore with this implementation. Also, I feel like I explored this route in the initial patch, but just couldn't make it work then. |
There was a problem hiding this comment.
I’m a bit nervous about this because now we will emit some expressions twice, once for the property initializer and once for the default argument. In the past we’ve had bugs with multiple emission of expressions; that’s why we introduced property initializers in the first place, so that each constructor doesn’t re-emit these expressions. I would suggest therefore changing the default argument generators to call the property initializers instead of re-emitting them, even though it’s slightly more complicated.
|
Oh I didn't know this was the case for expressions. 👍 |
|
@slavapestov I believe I did this correctly, but would love feedback. I've added some more tests to exercise indirect results. |
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
Azoy
force-pushed
the
maybe-add-default-arg-generator
branch
from
f0b33f8 to
edcab95
Compare
|
@slavapestov okay, I believe I have fixed the TBD test, as well as pull in your test changes as well. |
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
Release is usually a whole-module build; maybe you have a default arg generator being emitted from one file that references an initial value in another file, or something like that? |
|
Thanks Jordan! I think I see the issue, will work on this when I get home. |
|
@slavapestov I hate to ping you again, but I believe this is the one! |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test |
This comment has been minimized.
This comment has been minimized.
|
@swift-ci Please test macOS |
|
@swift-ci Please test source compatibility |
This comment has been minimized.
This comment has been minimized.
|
Thanks @Azoy! |
…nerator [SILGen] Emit default arg generator for stored property kind fix tests
In order to properly serialize stored property default arguments, we need to emit a default arg generator.
I chose to emit the parent initializer expression instead of the apply to the variable initializer because SIL would eventually just inline the function definition anyway (or at least in most cases from my findings) and this was a lot cleaner and cheaper implementation wise. I've updated the SILGen tests and added a serialization test.
cc: @slavapestov @jrose-apple