Skip to content
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

Implement @ObservableDefault macro #189

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

kevinrpb
Copy link

@kevinrpb kevinrpb commented Sep 15, 2024

Hi there! Thought I'd pitch in and contribute to this great package. This addresses #142 by creating a new macro that can be used inside @Observable classes.

The macro is implemented in a new DefaultsMacros module. The decision to do so is based on the introduction of a new dependency on swift-syntax, and to keep the main module dependency-free (specially since the syntax package adds quite a bit to build times). Please, let me know if you'd rather move it to the main module.

Settings the PR as draft for early review, but there are at least a couple things I want to do to get it ready. Also, I haven't put much thought into the naming; advice on this is appreciated.

TODO

  • Figure out if @ObservationIgnored can be added automatically as part of the macro.
    • This is not possible. There's currently no way for a macro attached to a property to add attributes to that same property.
  • Add more test cases.
  • Add docstring to the macro declaration.
  • Add info on how to use the macro to the README.

This addresses sindresorhus#142 by creating a new macro that can be used inside
`@Observable` classes.

The macro is implemented in a new `DefaultsMacros` module. The
decision to do so is based on the introduction of a new dependency on
`swift-syntax`, and to keep the main module dependency-free.
@sindresorhus
Copy link
Owner

Thanks for working on this 🙏

@sindresorhus
Copy link
Owner

The decision to do so is based on the introduction of a new dependency on swift-syntax, and to keep the main module dependency-free (specially since the syntax package adds quite a bit to build times).

👍

Tests for the macro declaration were in the wrong test target and the
test method names were not accurate.

Added another test target to actually test that the macro works when
used in an @observable class.
This is to be consistent with the `@Default` property wrapper.
In the macro tests, I had to move the @observable classes out of the
test because linter was asking to mark them as private but doing so
was causing the @observable macro to error.
@kevinrpb kevinrpb marked this pull request as ready for review September 18, 2024 17:46
@kevinrpb
Copy link
Author

I think this is now ready for review. Please, let me know about any issues/concerns.

BTW I renamed the macro to @Default so it is familiar. Let me know what you thing about this!

@kevinrpb kevinrpb changed the title Implement @ObservableDefaults macro Implement @Default macro Sep 19, 2024
readme.md Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

I don't think the @Default naming is going to work. The moment you import DefaultsMacros in a place that uses the @Default property wrapper, those become macro usages too, which doesn't work in a View. I guess we can name it ObservableDefault.

@sindresorhus
Copy link
Owner

The macro is also missing an important feature; it does not trigger updates to the model if a Defaults key is changed outside of the model. For example, if you set Defaults[.foo] = true somewhere in your code, the model will not be triggered. I think the user would expect the model to always be in sync with Defaults.

@kevinrpb
Copy link
Author

kevinrpb commented Sep 26, 2024

Thank you for fixing the typos.

I don't think the @Default naming is going to work. The moment you import DefaultsMacros in a place that uses the @Default property wrapper, those become macro usages too, which doesn't work in a View. I guess we can name it ObservableDefault.

Ah, this makes sense in hindsight. Will rename 👍🏽

The macro is also missing an important feature; it does not trigger updates to the model if a Defaults key is changed outside of the model. For example, if you set Defaults[.foo] = true somewhere in your code, the model will not be triggered. I think the user would expect the model to always be in sync with Defaults.

Yeah, this is indeed an important shortcoming. I've been fiddling to see if I could come up with something that is easy to use and works well. This is what I arrived at (expanded version), let me know what you think @sindresorhus:

@Observable
final class TestModelWithoutMacro {
	@ObservationIgnored
	var testValue: String {
		get {
			_ = _testValueObserver	// <--- Make sure the lazy var is initialized

			access(keyPath: \.testValue)
			return Defaults[Defaults.Keys.test]
		}
		set {
			withMutation(keyPath: \.testValue) {
				Defaults[Defaults.Keys.test] = newValue
			}
		}
	}

	// Use a lazy var so we can access self
	@ObservationIgnored
	private lazy var _testValueObserver: Defaults.Observation =
		Defaults.observe(Defaults.Keys.test) { keyChange in
			self.testValue = keyChange.newValue
		}
}

We cannot overload @default, otherwise it is impossible to use the
property wrapper version in the same source file as the macro as the
later takes precedence.
@sindresorhus
Copy link
Owner

Yeah, that looks good.

Should be Defaults.observe(Defaults.Keys.test, options: []) { keyChange in so it doesn't trigger an initial update.

of node: AttributeSyntax,
providingAccessorsOf declaration: some DeclSyntaxProtocol,
in context: some MacroExpansionContext
) throws -> [AccessorDeclSyntax] {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
) throws -> [AccessorDeclSyntax] {
) throws(ObservableDefaultMacroError) -> [AccessorDeclSyntax] {

And then you can remove ObservableDefaultMacroError from the throw statements.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! typed throws are here and I forgot about them. Neat.

@kevinrpb
Copy link
Author

kevinrpb commented Sep 26, 2024

OK... I'm a bit confused. When typing out the observation property, since it is lazy Swift doesn't complain about capturing self. However, when it's added via macro expansion it does?

I need to investigate why this happens... Any ideas?

image

@sindresorhus
Copy link
Owner

Not sure.

@sindresorhus
Copy link
Owner

Don't forget to make the self access weak in the closure though.

@kevinrpb
Copy link
Author

Seems like the compiler is straight up crashing when generating a lazy var with a macro (swiftlang/swift#74087). Xcode eats the error, but I can see it when using swift test in a terminal.

Will ask there and try to come up with other alternatives in the meantime.

@kevinrpb kevinrpb changed the title Implement @Default macro Implement @ObservableDefault macro Sep 26, 2024
@kevinrpb
Copy link
Author

kevinrpb commented Oct 3, 2024

While there's been no activity about the lazy properties in macros, I do have something else to comment on the PR.

I was fiddling around with other alternatives, and an issue came up that also applies when using the lazy property: if the @Observable class is marked as Sendable, lazy properties cause a warning in Swift 5 and an error in Swift 6 due to their inherent mutability. Basically, this means that any solution that involves a var will cause warnings and/or errors.

Will keep thinking about other ways, but I'm no longer sure it's possible to do with the current macro capabilities. I may be missing some clever trick to it.

@sindresorhus
Copy link
Owner

Basically, this means that any solution that involves a var will cause warnings and/or errors.

Use a lock and mark the variable with @unchecked Sendable.

@kevinrpb
Copy link
Author

kevinrpb commented Oct 4, 2024

Use a lock and mark the variable with @unchecked Sendable.

Thank you for the suggestion, a lock may work. But did you mean to mark the @Observable class with @unchecked Sendable? Not sure there is a way to just mark one property.

We could generate the code below and warn users that if they need the model to be Sendable they need to either use @unchecked or mark it @MainActor (or really, any global actor), either would work. However, I'm worried that suggesting marking it as @unchecked may hide other possible errors and people complain about it. Do you think adding a warning in the README is a good enough mitigation?

/* This source generates the model below.

@Observable
final class TestModel: @unchecked Sendable {
	@ObservableDefault(Defaults.Keys.test)
	@ObservationIgnored
	var testValue: String
}
*/
@Observable
final class TestModel: @unchecked Sendable {
	@ObservationIgnored
	var testValue: String {
		get {
			// Ensure the observation is set up, using the lock to prevent data races
			_testValueLock.lock()
			if _testValueObservation == nil {
				_testValueObservation = Defaults.observe(Defaults.Keys.test, options: []) { [weak self] keyChange in
					self?.testValue = keyChange.newValue
				}
			}
			_testValueLock.unlock()

			access(keyPath: \.testValue)
			return Defaults[Defaults.Keys.test]
		}
		set {
			withMutation(keyPath: \.testValue) {
				Defaults[Defaults.Keys.test] = newValue
			}
		}
	}

	@ObservationIgnored
	private let _testValueLock: Lock = .make()
	@ObservationIgnored
	private var _testValueObservation: Defaults.Observation?
}

@sindresorhus
Copy link
Owner

I was thinking there was a keyword for ignoring sendable, but what I meant was wrap it in an unchecked sendable box:

final class LockedSendableValue<Value>: @unchecked Sendable {
	private var _value: Value
	private let lock = Lock()

	init(_ value: @autoclosure @Sendable () throws -> Value) rethrows {
		self._value = try value()
	}

	func withValue<T: Sendable>(
		_ operation: @Sendable (inout Value) throws -> T
	) rethrows -> T {
		try self.lock.withLock {
			var value = self._value
			defer { self._value = value }
			return try operation(&value)
		}
	}
}

But did you mean to mark the @observable class with @unchecked Sendable? Not sure there is a way to just mark one property.

No

@kevinrpb
Copy link
Author

kevinrpb commented Oct 8, 2024

I've been running the suggestion in my head and I think I'm missing something. I'll try to write down the full issue:

  • The initial implementation added property accessors to the variable annotated with the macro, such that reading from it would get the value from Defaults, and writing to it would update it.
  • This had the shortcoming that the property would not get updated automatically if the Defaults value was updated from elsewhere.
  • To support that, I've been looking for an ergonomic way to setup an observation so the property gets update when the Defaults value changes. This has come with its own issues related to strict concurrency mode in Swift 6:
    • The property holding the observation cannot be a var (inc. lazy var). Any mutable property will cause an error.
      • Also, lazy var is not currently supported in macros.
    • We can ensure that the property is safe by also adding a lock, such that when we initialize the observation, we do so with the lock.
      • This won't remove the compiler error, but would ensure users that marking their model as @unchecked Sendable is safe (at least in our regard).
      • However, I don't feel too confident with this because asking people to mark their models as such can lead to other concurrency issues in their own properties.
      • Other option is to suggest using a global actor, which would allow for Sendable and maintain the concurrency checks.
    • The property cannot be let either (or at least I can't think of a way to do it), because it cannot capture self to update the observed property.

Now, here are the options that come to my mind:

  1. Warn users about the behaviour that the property will not update outside their model. Suggest setting up an observation if they need that functionality.
  2. Implement the observation as a var (+ lock), warn users that if their model needs to be Sendable, they will need to either use @unchecked or mark it with a global actor (e.g. @MainActor).

I can see that neither of these is a great option, but so far I haven't been able to come up with anything better.

Let me know if I've been missing anything from the suggestions, maybe I'm not fully understanding what you meant!

@sindresorhus
Copy link
Owner

How about something like this? This also correctly destroys the event subscription when the class deinits:

extension Defaults.Keys {
	static let test = Key<String>("test", default: "")
}

@Observable
final class TestModelWithoutMacro {
	private enum AssociatedKeys {
		static var observer = 1
	}

	@ObservationIgnored
	var testValue: String {
		get {
			if objc_getAssociatedObject(self, &AssociatedKeys.observer) == nil {
				let cancellable = Defaults.publisher(.test)
					.sink { [weak self] in
						self?.testValue = $0.newValue
					}
				objc_setAssociatedObject(self, &AssociatedKeys.observer, cancellable, .OBJC_ASSOCIATION_RETAIN)
			}

			access(keyPath: \.testValue)
			return Defaults[.test]
		}
		set {
			withMutation(keyPath: \.testValue) {
				Defaults[.test] = newValue
			}
		}
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants