-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Refocus observation to it's core capability #65528
Conversation
let binding = property.bindings.first | ||
else { | ||
return false | ||
struct DebugMessage: DiagnosticMessage { |
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 should be removed before merging
58dc02c
to
d7ab7d0
Compare
@swift-ci please test |
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.
Admittedly not as familiar with the macro stuff right now, but the rest of the changes look good to me!
providingConformancesOf declaration: Declaration, | ||
in context: Context | ||
) throws -> [(TypeSyntax, GenericWhereClauseSyntax?)] { | ||
return [("Observable", nil)] |
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.
Do we want to check whether the type has explicitly introduced a conformance to Observable
?
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.
that might be good but it can only be textual comparisons
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, that would be fine.
of keyPath: PartialKeyPath<Self> | ||
) -> TrackedProperties<Self> | ||
} | ||
@_marker public protocol Observable { } |
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.
Oh, that's a really big conceptual change, to introduce Observable
as a marker protocol. Are we sure that we aren't ever going to want to have requirements or extensions on Observable
things? Operations we're injecting as members now, like access
and withMutation
, seem like they would have been candidates for operations on Observable
.
Is there a meta-goal here I missed, of being able to add observation support to a class without back-deployment concerns?
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.
the problem with normal protocol adoption is that it would break subclassing. the marker is for utility to APIs that want to restrict to something that has been marked as observable.
If later we want to add in protocol based methods/requirements we would need to have finer grained constraints (e.g. identifying the inheritance etc)
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.
I don't understand how this breaks subclassing. I think we're going to find the marker protocol to be very limiting
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.
so the problem with normal protocols would be that if the type is marked as @Observable
then the macro generates a conformance via an extension: extension MyType: Observable { }
. Then if MySubclassType
inherits from MyType
and also gets a @Observable
application to it (which makes sense for observation being required on each layer since the macro does not have knowledge beyond what is typed directly on the type) it means that there will be an extension MySubclassType: Observable { }
. In the case of normal protocols that emits an error of a redundant protocol conformance.
Marker protocols do not have this restriction.
I feel that if we have additional protocol emissions for specific behaviors in the future we can add a new (non marker) protocol that has the desired features we want.
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.
Just a couple of questions around tests, otherwise LGTM!
We should add non-executable tests that verify that the macros are behaving as expected; especially that invalid types are correctly erroring.
@@ -53,39 +106,21 @@ public struct ObservationRegistrar<Subject: Observable>: Sendable { | |||
} | |||
} | |||
|
|||
public func willSet<Member>( | |||
public func willSet<Subject: Observable, Member>( |
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.
Do we need tests of willSet
or didSet
being used directly? Or do you consider testing the usage via withMutation
enough?
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.
since the withMutation is really just a pass-through I consider that probably enough
|
||
} | ||
|
||
struct NonObservableContainer { |
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.
Did you mean for this to store an instance of ObservableContents
?
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.
yep, I wanted to ensure nesting of the macro works correctly (because it emits an extension to the type it could cause problems)
d7ab7d0
to
29d09b0
Compare
@swift-ci please test |
…s and re-work the macro to utilize more robust emissions
29d09b0
to
236f221
Compare
@swift-ci please test and merge |
@swift-ci please test |
…s and re-work the macro to utilize more robust emissions (apple#65528)
…s and re-work the macro to utilize more robust emissions (apple#65528)
This is a draft of a leaner and more focused
Observation
. It removes some of the features that caused restrictions to the usages of the APIs in favor for leaving those for future development (and perhaps even better compiler/macro features to support them).Removed:
AsyncSequences for values and changes
dependencies requirements
A type housing ObservationTracking
Much complexity with the registrar
Generics from the registrar
protocol requirements for observable types
synthetic memberwise initializers
abstracted storage
Added:
support for subclassing
method/field override support for the macro (if you write the method by hand it won't try to re-emit it)
easy methods for calling access/withMutation housed on the type itself
function level generics for the registrar
nested tracking support (calling tracking inside tracking is no longer a misuse)
guards for trying to make enumerations observable (how would that even work...?)
direct storage
a way to prevent observation on properties
support for weak, unowned, IUO, and actor bound properties
rdar://108795488