-
Notifications
You must be signed in to change notification settings - Fork 10.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Property Wrappers] Refactor property wrappers so the synthesized backing init is only type checked once #30807
Conversation
innermost call to init(wrappedValue:) if the target is for initialization of a property wrapper's backing storage.
@swift-ci please smoke test |
@swift-ci please test source compatibility |
@@ -298,6 +298,7 @@ namespace { | |||
ConstraintSystem &cs; | |||
DeclContext *dc; | |||
Solution &solution; | |||
Optional<SolutionApplicationTarget> target; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be Optional
, but there's a Solution::coerceToType
that creates an ExprRewriter
just to call ExprRewriter:: coerceToType
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably refactor away Solution::coerceToType
later. It looks unnecessary.
ec54de5
to
37e3950
Compare
Looks like MovieSwiftUI exposed some bugs. I'll investigate! |
wrapper original wrapped value expression inside of CSApply. This prevents type checking the synthesized backing storage initializer twice - once with the original expression and again with the placeholder.
…s that involve inout closure parameters and were previously type checked twice.
initializer for property wrappers that are not memberwise initialized but have an explicit original wrapped value.
37e3950
to
5f00a1b
Compare
@swift-ci please smoke test |
@swift-ci please test source compatibility |
@@ -298,6 +298,7 @@ namespace { | |||
ConstraintSystem &cs; | |||
DeclContext *dc; | |||
Solution &solution; | |||
Optional<SolutionApplicationTarget> target; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably refactor away Solution::coerceToType
later. It looks unnecessary.
include/swift/AST/Expr.h
Outdated
@@ -4021,11 +4021,14 @@ class DynamicTypeExpr : public Expr { | |||
/// subexpressions of that AST node. | |||
class OpaqueValueExpr : public Expr { | |||
SourceRange Range; | |||
Expr *UnderlyingValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpaqueValueExpr
is a general-purpose construct where the "underlying value" comes from something elsewhere in the AST, above the OpaqueValueExpr
. Is there some other place we can put the original initializer value that doesn't require adding this field to OpaqueValueExpr
? One option: create a new Expr
node ImplicitlyWrappedPropertyValueExpr
that stores the OpaqueValueExpr
and its underlying value. This will make findWrappedValuePlaceholder
even simpler and avoid altering the design of OpaqueValueExpr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think having a dedicated Expr
node for the wrapped value is the right thing to do. I was also worried about something else starting to use the underlyingValue
field that I added here and breaking my assumptions in this PR that it's only used for property wrappers. Thank you!
wrapped value placeholder in an init(wrappedValue:) call that was previously injected as an OpaqueValueExpr. This commit also restores the old design of OpaqueValueExpr.
@swift-ci please smoke test |
@swift-ci please test source compatibility |
…ng a property wrapper backing init if the property type doesn't match the wrapped value type.
@swift-ci please smoke test |
@swift-ci please test source compatibility |
@swift-ci please smoke test Linux platform |
1 similar comment
@swift-ci please smoke test Linux platform |
@swift-ci please smoke test |
Previously, if a property wrapper had an initial wrapped value expression:
Then
PropertyWrapperBackingPropertyInfoRequest
would synthesize and type-check the backing property initializer twice - once with the wrapped value expression, and again with an opaque value placeholder.Instead, we can have CSApply inject the opaque value placeholder around the
wrappedValue
argument incoerceCallArguments
.Resolves: rdar://problem/59685601
Resolves: rdar://problem/56163425
Resolves: SR-11594