-
Notifications
You must be signed in to change notification settings - Fork 57
Make AnyType Sendable with Reflection support #782
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
base: main
Are you sure you want to change the base?
Conversation
|
pipeline is failing because some tests needs updating for usage of anyDictionary |
| // | ||
|
|
||
| import Foundation | ||
|
|
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.
Unrelated to this line, can you move the stuff that would be helpful for release notes (like the migration guide) to a "# Release Notes" section at the end of the PR notes? Anything under a "# Release Notes" section will be automatically added to the actual release notes. Example here with PR #749 in the latest release.
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.
Updated. Too verbose?
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.
No, that's perfect. Thanks!
| Use the `asAnyDictionary` property for backward compatibility with `[String: Any]`. | ||
| */ | ||
| case anyDictionary(data: [String: Any]) | ||
| case anyDictionary(data: [String: AnyType]) |
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.
Is this a breaking change? I'm thinking code like this would break with this change:
let myDict: [String: Any] = ["a": 1]
let example: AnyType = .anyDictionary(data: myDict)Is that accurate?
If this is breaking, there might not be a need for backwards compat stuff 🤔 .
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 - the example you shared is a breaking change. But, the backward compatibility helpers mitigate the issue.
The convenience initializer (110-115) could be used:
let myDict: [String: Any] = ["a": 1]
let example: AnyType = AnyType(anyDictionary: myDict)Or, the generic init(from:) (136-175)
let example: AnyType = AnyType(from: myDict)For new code, wrapped values would also work:
let example: AnyType = .anyDictionary(data: ["a": .number(data: 1)]) How would you like to categorize the PR? If it's "breaking", all the helpers can be removed as you note.
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.
Okay, I talked about it with @nancywu1 @KetanReddy @cehan-Chloe and our preference now is to introduce a second type, SendableAnyType, with all the new stuff and mark AnyType as deprecated, to be dropped in 1.0. This will avoid breaking changes and we won't really need any backwards compatibility code.
What are your thoughts?
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.
If PlayerUI introduces both AnyType and SendableAnyType, it introduces a dual-type problem.
// PlayerUI would have:
enum AnyType {
case anyDictionary([String: Any])
// ...
}
enum SendableAnyType: Codable, Sendable {
case dictionary([String: SendableAnyType])
// ...
}A few considerations:
- At the conversion boundary between the PubSubPlugin, we'd need to transform
AnyTypetoSendableAnyTypecreating client overhead. - Eventually, this would cause a breaking change (as intended) where this PR simply extends the type.
- It would be easier to introduce and maintain this new type outside PlayerUI (at least for my use-case) and potentially more useful to place in a lower shared dependency since Player is one rendering mode.
| // MARK: - Backward Compatibility & Conversion Helpers | ||
|
|
||
| extension AnyType { |
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.
What do you think about marking these as @deprecated, to be removed in Player 2.0? We're just working on player 1.0 now, so I think that would give people time to migrate.
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'm happy to do whatever makes sense. Yes, these convenience helpers could be removed and the API simplified for Player 2.0. What would you prefer?
| /** | ||
| Pattern match on anyDictionary with a legacy `[String: Any]` handler. | ||
|
|
||
| - Parameter handler: Closure to execute if this is an anyDictionary case | ||
| */ | ||
| public func matchAnyDictionary(_ handler: ([String: Any]) -> Void) { | ||
| if let dict = asAnyDictionary { | ||
| handler(dict) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| Pattern match on anyArray with a legacy `[Any]` handler. | ||
|
|
||
| - Parameter handler: Closure to execute if this is an anyArray case | ||
| */ | ||
| public func matchAnyArray(_ handler: ([Any]) -> Void) { | ||
| if let arr = asAnyArray { | ||
| handler(arr) | ||
| } | ||
| } |
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.
What are your thoughts on removing matchAnyDictionary and matchAnyArray necessary? They seem to really just remove an if statement for the user and since they're backwards compat things, I'm worried about introducing more things that users will have to migrate away from (assuming the backwards compat is temporary until users migrate to the new pattern).
If you do want to keep them, what are your thoughts on renaming? I think the name is as a bit confusing, since match is typically used (in my experience, admittedly somewhat limited) for finding and returning something. Like a regex match.
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.
Agreed - the convenience helpers can bridge the gap and they essentially remove an extra if. Will remove.
| public var dynamicValue: Any { | ||
| return asAny | ||
| } |
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.
Is this necessary? It seems like a dupe of asAny, which is also public?
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.
You're correct - let me remove this to simplify the interface.
| let title: String? = dict["title"]?.as(String.self) | ||
| ``` | ||
| */ | ||
| public subscript(key: String) -> AnyType? { |
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 is nice!
| AssetFlowView(flow: flow.flow, plugins: plugins, result: result) | ||
| .padding(padding) | ||
| .navigationBarTitle(Text(flow.name)) | ||
| .navigationTitle(Text(flow.name)) |
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.
Are the changes in this file intended?
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 was trying to debug why the build pipeline was failing remotely - and navigationBarTitle has been deprecated (and is unavailable on MacOS) and replaced with navigationTitle.
Let me revert it to keep this PR clean.
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.
Locally, I can't get the project to build without the change:
bazel build //ios/core:PlayerUI //ios/test-utils-core:PlayerUITestUtilitiesCore //plugins/pubsub/ios:PlayerUIPubSubPlugin
...
ERROR: /Users/jjessup/Documents/Code/Experiments/public/player/ios/test-utils-core/BUILD.bazel:3:13: Compiling Swift module //ios/test-utils-core:PlayerUITestUtilitiesCore failed: (Exit 1): worker failed: error executing SwiftCompile command (from target //ios/test-utils-core:PlayerUITestUtilitiesCore) bazel-out/darwin_arm64-opt-exec-ST-d57f47055a04/bin/external/rules_swift+/tools/worker/worker swiftc ... (remaining 1 argument skipped)
ios/test-utils-core/Sources/ui-test/AssetCollection.swift:53:34: error: 'navigationBarTitle' is unavailable in macOS
51 | AssetFlowView(flow: flow.flow, plugins: plugins, result: result)
52 | .padding(padding)
53 | .navigationBarTitle(Text(flow.name))
| `- error: 'navigationBarTitle' is unavailable in macOS
54 | }
55 | .accessibility(identifier: "\(section.title) \(flow.name)")
SwiftUI.View.navigationBarTitle:7:27: note: 'navigationBarTitle' has been explicitly marked unavailable here
5 | @available(watchOS, introduced: 6.0, deprecated: 100000.0, renamed: "navigationTitle(_:)")
6 | @available(visionOS, introduced: 1.0, deprecated: 100000.0, renamed: "navigationTitle(_:)")
7 | nonisolated public func navigationBarTitle(_ title: Text) -> some View
| `- note: 'navigationBarTitle' has been explicitly marked unavailable here
8 | }
9 |
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.
Ah, ios is not meant to be built directly like this---it will try to build for MacOS instead of iOS.
The intended ios dev process is one of these, also documented here.
// Build everything and open in Xcode (shorthand for my preferred)
just dev-ios
// Run the demo app from the cli with Bazel (which I basically never use except to confirm it continues to work)
bazel run ios/demo:PlayerUIDemo
Make AnyType Sendable with Reflection Support
Overview
This PR makes the
AnyTypeenum conform to theSendableprotocol, enabling safe usage across Swift concurrency boundaries (async/await, actors, etc.). Additionally, it adds reflection support for more ergonomic value access and type casting.Motivation
With Swift 6's strict concurrency checking,
AnyTypeneeded to beSendableto be used safely in concurrent contexts. The previous implementation used[String: Any]and[Any]in theanyDictionaryandanyArraycases, which are notSendablebecauseAnycannot guarantee thread-safety.Changes
1. Sendable Conformance via Recursive Types
Changed:
This recursive approach eliminates
Anytypes while maintaining the same functionality, makingAnyTypefullySendable.2. Backward Compatibility API
Added comprehensive helpers to maintain compatibility with existing code:
New Initializers
init(anyDictionary: [String: Any])- Convert[String: Any]toAnyTypeinit(anyArray: [Any])- Convert[Any]toAnyTypeinit(from: Any)- Recursively convert anyAnyvalue toAnyTypeConversion Properties
asAnyDictionary: [String: Any]?- Access as legacy dictionary formatasAnyArray: [Any]?- Access as legacy array formatasAny: Any- Convert back toAnyfor interopPattern Matching Helpers
matchAnyDictionary(_:)- Pattern match with[String: Any]handlermatchAnyArray(_:)- Pattern match with[Any]handler3. Reflection Support (New Feature)
Added reflection capabilities for more ergonomic value access:
Dynamic Value Access
Provides direct access to underlying values without pattern matching.
Type Casting Helper
Enables clean type casting:
Subscript Access
Provides convenient dictionary-like access for all dictionary cases:
4. Enhanced Type Handling
NSNumberfrom JSON deserializationinit(from:)automatically chooses the most specific type (e.g.,[String: String]becomes.dictionarynot.anyDictionary)Test Coverage
Added 23 new tests bringing the total to 44 tests (all passing):
Performance Considerations
asAnyDictionaryandasAnyArrayonly convert when neededBreaking Changes
None. All existing APIs remain functional and unchanged.
Files Changed
ios/core/Sources/Types/Generic/AnyType.swift- Core implementationios/core/Tests/Types/Generic/AnyTypeTests.swift- Test coverageBenefits
Swift 6 Ready - Full
Sendableconformance for strict concurrencyBackward Compatible - Existing code works without changes
Type Safe - No
Anytypes in the core enumBetter Ergonomics - Reflection support for cleaner code
Fully Tested - 44 comprehensive tests
No Performance Cost - Same memory layout and efficiency
Examples
Sendable Usage
Reflection Usage
Nested Structure Access
Checklist
Related Issues
This change enables safe usage of
AnyTypein Swift 6 strict concurrency mode and modern async/await codebases.Change Type (required)
Indicate the type of change your pull request is:
patchminormajorN/ARelease Notes
The
AnyTypeenum conform to theSendableprotocol, enabling safe usage across Swift concurrency boundaries (async/await, actors, etc.). Existing code continues to work without modification.For new code, consider using the more ergonomic APIs:
Old Pattern (still supported)
New Pattern (recommended)
Creating AnyType from Any
Accessing Values