Observation and associated macros #63725

phausler · 2023-02-16T19:48:44Z

This is an experimental implementation of the Observation and macros associated. It provides a compiler plugin for defining the @Observable macro and a module for defining the protocols and default observation registrar.

@Observable final class MyModel {
  var someField: String = "test"
}

let m = MyModel()
...
for await value in m.changes(for: \.someField) {
}

phausler · 2023-02-16T19:49:56Z

@swift-ci please build toolchain

stdlib/public/Observation/CMakeLists.txt

phausler · 2023-02-16T20:26:11Z

@swift-ci please build toolchain

compnerd

This is going to require an associated change to apple/swift-installer-script to package up the new modules in the SDKs and Runtime .

stdlib/public/Observation/Sources/Observation/Observable.swift

stdlib/public/Observation/Sources/_ObservationRuntime/ThreadLocal.cpp

phausler · 2023-02-16T20:38:06Z

@swift-ci please build toolchain

phausler · 2023-02-16T22:11:35Z

@swift-ci please build toolchain

al45tair · 2023-02-17T10:17:49Z

The ThreadLocal.cpp file seems to duplicate a lot of what's already done for you in swift/Threading/ThreadLocalStorage.h. I wonder if it could be written using the support that's in that header instead? You'd want to add observation_tracking and observation_transaction to swift/Threading/TLSKeys.h and also to the tls_get_key() function in swift/Threading/Impl/Darwin.h, and then you could just declare two variables, ala:

static SWIFT_THREAD_LOCAL_TYPE(void *, tls_key::observation_tracking) observationTracking;
static SWIFT_THREAD_LOCAL_TYPE(void *, tls_key::observation_transaction) observationTransaction;

and do what you will with them.

phausler · 2023-02-22T22:19:02Z

@swift-ci please smoke test

phausler · 2023-02-22T22:19:12Z

@swift-ci please build toolchain

phausler · 2023-02-23T23:15:58Z

@swift-ci please smoke test

phausler · 2023-02-24T00:30:32Z

@swift-ci please smoke test

ktoso · 2023-02-27T04:40:33Z

stdlib/public/Observation/Sources/Observation/ThreadLocal.swift

+func _tlsSet(_ value: UnsafeMutableRawPointer?)
+
+@available(SwiftStdlib 5.9, *)
+struct ThreadLocal {


These likely should instead use the existing infra in: stdlib/public/core/ThreadLocalStorage.swift

I need TLS keys that are specific - that set of APIs from what I can tell does not do that.

Then add API over there rather than a new parallel world here perhaps?

phausler · 2023-02-27T17:23:53Z

@swift-ci please smoke test

test/decl/var/property_wrappers_synthesis.swift

phausler · 2023-02-27T21:52:05Z

@swift-ci please smoke test

phausler · 2023-02-28T00:02:56Z

@swift-ci please smoke test

phausler · 2023-02-28T03:26:27Z

@swift-ci please smoke test

phausler · 2023-02-28T16:47:06Z

@swift-ci please smoke test

phausler · 2023-02-28T18:08:19Z

@swift-ci please smoke test

phausler · 2023-02-28T19:22:07Z

@swift-ci please smoke test

phausler · 2023-02-28T22:03:10Z

@swift-ci please build toolchain macosx

phausler · 2023-02-28T22:28:35Z

@swift-ci please build toolchain macosx

phausler · 2023-03-01T03:04:14Z

@swift-ci please smoke test

phausler · 2023-03-01T17:24:23Z

@swift-ci please smoke test

phausler · 2023-03-01T17:47:26Z

@swift-ci please smoke test

phausler · 2023-03-01T20:09:41Z

@swift-ci please smoke test

phausler · 2023-03-01T21:38:22Z

@swift-ci please smoke test

phausler · 2023-03-01T22:27:03Z

@swift-ci please build toolchain macosx

phausler · 2023-03-02T00:26:29Z

@swift-ci please smoke test

stdlib/public/Observation/Sources/Observation/Locking.swift

lorentey · 2023-03-02T03:01:46Z

stdlib/public/Observation/Sources/Observation/Locking.swift

+
+  private let buffer: ManagedBuffer<State, UInt8>
+
+  init(_ buffer: ManagedBuffer<State, UInt8>) {


Nit: In the stdlib, we prefer to always spell out internal, even if it's the implicit default. (This is to avoid having to figure it out from context.)

lorentey · 2023-03-02T03:05:47Z

stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift

+    }
+
+    let kind: Kind
+    var collected: KeyPaths<Subject>?


Nit: This public struct does not appear to have an explicit initializer -- it's a good idea to add one explicitly, even if it matches whatever the compiler synthesizes on its own.)

(Quick, which struct do these belong to? See other nit about presentation of type definitions.)

lorentey · 2023-03-02T03:13:21Z

stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift

+  let context = Context()
+  let lifetime: Lifetime


Nit: Readers can't easily tell where these declarations will end up -- we need to carefully hunt down the start of this overlong declaration a couple hundred lines above. It's not polite to put a myriad lines in between a struct's name and its stored properties.

(See Presentation of type definitions in the manual.)

To ease reading/understanding type declarations, we prefer to define members in the following order:

Crucial type aliases and nested types, not exceeding a handful of lines in length

Stored properties

Initializers

Any other instance members (methods, computed properties, etc)

Please keep all stored properties together in a single uninterrupted list, followed immediately by the type's most crucial initializer(s). Put these as close to the top of the type declaration as possible -- we don't want to force readers to scroll around to find these core definitions.
...

In general, it is a good idea to keep the main struct/class definition as short as possible: preferably it should consist of the type's stored properties and a handful of critical initializers, and nothing else.

Everything else should go in standalone extensions, arranged by logical theme. For example, it's often nice to define protocol conformances in dedicated extensions. If it makes sense, feel free to add a comment to title these sectioning extensions.
...

It's okay for the core type declaration to forward reference large nested types or static members that are defined in subsequent extensions. It's often a good idea to define these in an extension immediately following the type declaration, but this is not a strict rule.

lorentey · 2023-03-02T03:20:47Z

stdlib/public/Observation/Sources/Observation/Locking.swift

+func _lockUnlock(_: UnsafeRawPointer)
+
+@available(SwiftStdlib 5.9, *)
+struct ManagedCriticalState<State> {


Crucial nit: In the stdlib, we strictly follow the leading underscore rule when naming internal types and functions.

All APIs that aren't part of the stdlib's official public API must include at least one underscored component in their fully qualified names. This includes symbols that are technically declared public but that aren't considered part of the public stdlib API, as well as @usableFromInline internal, plain internal and [file]private types and members.
...
This rule ensures we don't accidentally clutter the public namespace with @usableFromInline things (which could prevent us from choosing the best names for newly public API later), and it also makes it easy to see at a glance if a piece of stdlib code uses any non-public entities.

(Things declared purely internal have a remarkable tendency to become @usableFromInline or @inlinable before the release ships, usually as a side effect of performance work.)

lorentey · 2023-03-02T03:55:02Z

stdlib/public/Observation/Sources/Observation/Locking.swift

+  }
+
+  init(_ initial: State) {
+    self.init(LockedBuffer.create(minimumCapacity: _lockSize()) { buffer in


This does not technically guarantee that the managed buffer's storage will have the right alignment to store a lock.

natecook1000 · 2023-03-02T15:11:26Z

lib/Macros/Sources/ObservationMacros/ObservableMacro.swift

+
+    let registrar: DeclSyntax = 
+      """
+      let _registrar = ObservationRegistrar<\(parentName)>()


This is probably QoI work, but what's the developer experience going to be like if there's already a _registrar or _storage property implemented on the type? Can we detect that and provide a better error message?

I assume they'll have to use this registrar if they're implementing their own tracking e.g. for computed properties — is there any other kind of control we should provide?

The registrar cannot be private or file private since they may have computed properties in extensions or extensions in other files. I guess it could be explicitly internal if we wanted.

natecook1000 · 2023-03-02T15:20:36Z

stdlib/public/Observation/Sources/Observation/KeyPaths.swift

+//===----------------------------------------------------------------------===//
+
+@available(SwiftStdlib 5.9, *)
+public struct KeyPaths<Root>: SetAlgebra, Hashable, @unchecked Sendable {


I still think this will be better off without the SetAlgebra conformance, especially since it will sometimes have members that aren't accessible by a developer who is tracking changes. Does the public API need to provide more than the sequence initializer and a contains(_:) method?

The name also really implies collection conformance, but I don't think it's a dealbreaker. Does it need to have "key paths" in the name? TrackedProperties would communicate the intent more, since it isn't really a general-purpose type.

Equatable conformance also seems suspect, since we can have KeyPaths that are functionally equivalent as far as the user is concerned but that don't compare as equal due to inaccessible members. Is that a necessary conformance?

natecook1000 · 2023-03-02T15:34:42Z

stdlib/public/Observation/Sources/Observation/ObservedChanges.swift

+@available(SwiftStdlib 5.9, *)
+extension ObservedChanges: @unchecked Sendable where Subject: Sendable { }
+
+@available(*, unavailable)
+extension ObservedChanges.Iterator: Sendable { }


Why is the Sendable conformance okay for the sequence, but not the iterator? It looks like the context is going to mutated out from under both.

AsyncIterators must not be Sendable. We learned this the hard way. The problem with them being Sendable is that you cannot tie a specific consumer task to an iterator anymore. This makes it super hard to implement the AsyncSequence itself because it has to suddenly handle multiple consumers on a single iterator even though it is a unicast AsyncSequence. Our latest stance on this is that AsyncSequences should most of the time be Sendable and can be passed to different tasks. Each task should create its own iterator to consume the sequence.

If an AsyncSequence doesn't support multiple iterators we are fatalErroring on this in most places right now.

the AsyncSequence definition is just a description of what to observe which means that if the subject in question can be sent across tasks the the changes associated with said observation can also be sent across tasks. However the iteration infers stateful cancellation and mutation of the context of which next element is being iterated, that cannot be sent meaningfully because if one task cancels then all tasks would get nil, and more of an issue is that if one task consumes a next value that value is denied to other tasks.

So the sequence itself is sendable but the iterator is not.

Interesting! This all sounds very similar to the headaches around copying a regular sequence iterator. (If we designed things from scratch today, perhaps iterators would not be required to be copyable.)

Yeah, that's my reaction — it's surprising that you can make multiple copies of the iterator and then iterate them concurrently, drawing from the same underlying context, but the Sendable semantics don't explicitly ban such a system. The cancellation bit is what I was missing in the difference between the sequence(s) and iterator(s). Thanks!

FranzBusch · 2023-02-24T09:42:15Z

stdlib/public/Observation/Sources/Observation/ObservedChanges.swift

+}
+
+@available(SwiftStdlib 5.9, *)
+extension ObservedChanges: @unchecked Sendable where Subject: Sendable { }


why @unchecked Sendable here?

due to KeyPath not being Sendable (but in this use case it is fine to be sent across tasks)

FranzBusch · 2023-03-02T16:25:59Z

stdlib/public/Observation/Sources/Observation/ObservedChanges.swift

+@available(SwiftStdlib 5.9, *)
+extension ObservedChanges: @unchecked Sendable where Subject: Sendable { }
+
+@available(*, unavailable)
+extension ObservedChanges.Iterator: Sendable { }


AsyncIterators must not be Sendable. We learned this the hard way. The problem with them being Sendable is that you cannot tie a specific consumer task to an iterator anymore. This makes it super hard to implement the AsyncSequence itself because it has to suddenly handle multiple consumers on a single iterator even though it is a unicast AsyncSequence. Our latest stance on this is that AsyncSequences should most of the time be Sendable and can be passed to different tasks. Each task should create its own iterator to consume the sequence.

If an AsyncSequence doesn't support multiple iterators we are fatalErroring on this in most places right now.

phausler · 2023-03-02T17:37:46Z

@swift-ci please smoke test

stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift

phausler · 2023-03-02T23:31:22Z

@swift-ci please smoke test and merge

DougGregor

I've only looked at the macros and build-integration bits, and they all seem file.

DougGregor · 2023-03-03T02:00:10Z

lib/Macros/Sources/ObservationMacros/CMakeLists.txt

+    ${SWIFT_SYNTAX_TARGETS}
+  )
+
+  set(SWIFT_SYNTAX_LIBRARIES_SOURCE_DIR


This is a big pile of CMake that it would be really nice to share with ASTGen and any other macro implementations we might land. That refactor doesn't have to happen with your PR necessarily.

yea I was tempted to generalize this into a macro library specific function - but my rush was to get things in first and then have follow-on tasks (where we have more time) to clean that up.

DougGregor · 2023-03-03T02:01:02Z

lib/Macros/Sources/ObservationMacros/CMakeLists.txt

+
+  swift_install_in_component(TARGETS ObservationMacros
+    LIBRARY
+      DESTINATION "lib${LLVM_LIBDIR_SUFFIX}/host/plugins"


This path seems like it's missing a swift?

whoops, that is definitely missing - will follow up on that

DougGregor · 2023-03-03T02:01:45Z

lib/Macros/Sources/ObservationMacros/CMakeLists.txt

+  # Copy over all of the shared libraries from earlyswiftsyntax so they can
+  # be found via RPATH.
+  foreach (sharedlib ${SWIFT_SYNTAX_SHARED_LIBRARIES})
+    add_custom_command(
+      TARGET ObservationMacros PRE_BUILD
+      COMMAND ${CMAKE_COMMAND} -E copy_if_different ${SWIFT_SYNTAX_LIBRARIES_SOURCE_DIR}/${sharedlib} ${SWIFT_SYNTAX_LIBRARIES_DEST_DIR}/${sharedlib}
+      COMMENT "Copying ${sharedlib}"
+    )
+  endforeach()
+


I don't understand why we need this. ASTGen should already have dealt with whatever is needed?

most of this was copy-pasta from ASTGen

DougGregor · 2023-03-03T02:04:53Z

stdlib/public/Observation/Sources/Observation/Macros.swift

+//===----------------------------------------------------------------------===//
+
+#if SWIFT_OBSERVATION_MACROS
+#if compiler(>=5.8) && hasAttribute(attached)


You shouldn't actually need the hasAttribute check here

will remove

DougGregor · 2023-03-03T02:05:46Z

stdlib/public/Observation/Sources/Observation/Macros.swift

+#if SWIFT_OBSERVATION_MACROS
+#if compiler(>=5.8) && hasAttribute(attached)
+@available(SwiftStdlib 5.9, *)
+@attached(member)


This is going to need to document the names it produces, per SE-0389

what specifically needs to happen there? The MemberMacro will add in a field _registrar, the functions transactions(for:isolation:) and changes(for:), a sub-type _Storage, and a field _storage; are those what need to be listed?

I think your attribute should read

@attached(member, names: named(_registrar), named(transactions), named(changes), named(_Storage), named(_storage))

phausler requested a review from compnerd as a code owner February 16, 2023 19:48

phausler requested a review from Azoy February 16, 2023 19:58

phausler commented Feb 16, 2023

View reviewed changes

stdlib/public/Observation/CMakeLists.txt Outdated Show resolved Hide resolved

compnerd reviewed Feb 16, 2023

View reviewed changes

phausler force-pushed the pr/observation branch from f82be48 to 4df6010 Compare February 22, 2023 22:09

phausler requested review from hborla, slavapestov and al45tair as code owners February 22, 2023 22:09

ktoso reviewed Feb 27, 2023

View reviewed changes

phausler force-pushed the pr/observation branch from 46bc547 to f5fa395 Compare February 27, 2023 17:23

phausler requested a review from FranzBusch February 27, 2023 17:30

phausler commented Feb 27, 2023

View reviewed changes

test/decl/var/property_wrappers_synthesis.swift Outdated Show resolved Hide resolved

Initial draft of observation

8a7f600

phausler force-pushed the pr/observation branch from f55c899 to 9cafbe5 Compare February 28, 2023 03:25

phausler requested a review from etcwilde February 28, 2023 18:08

phausler changed the title ~~[WIP] Initial draft of observation~~ Observation and associated macros Mar 1, 2023

lorentey reviewed Mar 2, 2023

View reviewed changes

natecook1000 reviewed Mar 2, 2023

View reviewed changes

FranzBusch reviewed Mar 2, 2023

View reviewed changes

natecook1000 reviewed Mar 2, 2023

View reviewed changes

stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift Outdated Show resolved Hide resolved

phausler force-pushed the pr/observation branch 4 times, most recently from afe06f4 to 604e00f Compare March 2, 2023 23:02

Observation and associated macros

40c277b

phausler force-pushed the pr/observation branch from 604e00f to 40c277b Compare March 2, 2023 23:31

swift-ci merged commit 8dac45e into swiftlang:main Mar 3, 2023

DougGregor reviewed Mar 3, 2023

View reviewed changes


		private let buffer: ManagedBuffer<State, UInt8>

		init(_ buffer: ManagedBuffer<State, UInt8>) {

Observation and associated macros #63725

Observation and associated macros #63725

Uh oh!

Conversation

phausler commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phausler commented Feb 16, 2023

Uh oh!

Uh oh!

phausler commented Feb 16, 2023

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phausler commented Feb 16, 2023

Uh oh!

phausler commented Feb 16, 2023

Uh oh!

al45tair commented Feb 17, 2023

Uh oh!

phausler commented Feb 22, 2023

Uh oh!

phausler commented Feb 22, 2023

Uh oh!

phausler commented Feb 23, 2023

Uh oh!

phausler commented Feb 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phausler commented Feb 27, 2023

Uh oh!

Uh oh!

phausler commented Feb 27, 2023

Uh oh!

phausler commented Feb 28, 2023

Uh oh!

phausler commented Feb 28, 2023

Uh oh!

phausler commented Feb 28, 2023

Uh oh!

phausler commented Feb 28, 2023

Uh oh!

phausler commented Feb 28, 2023

Uh oh!

phausler commented Feb 28, 2023

Uh oh!

phausler commented Feb 28, 2023

Uh oh!

phausler commented Mar 1, 2023

Uh oh!

phausler commented Mar 1, 2023

Uh oh!

phausler commented Mar 1, 2023

Uh oh!

phausler commented Mar 1, 2023

Uh oh!

phausler commented Mar 1, 2023

Uh oh!

phausler commented Mar 1, 2023

Uh oh!

phausler commented Mar 2, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phausler commented Feb 16, 2023 •

edited

Loading