Update PropertyList #267
Update PropertyList #267
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant OpenSwiftUITestsSupport
participant PropertyList
participant Tracker
TestSuite->>OpenSwiftUITestsSupport: Use test property keys/derived keys
TestSuite->>PropertyList: Initialize, set, override, merge, describe
TestSuite->>Tracker: Track values, invalidate, union
PropertyList->>Tracker: Provide property values, notify on change
Tracker->>PropertyList: Invalidate or recompute values
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Tests/OpenSwiftUISymbolDualTests/Data/PropertyListTests.swift (1)
266-270: Potential logic hole identical to Core tracker testThe symbol-dual tracker test also assumes it is acceptable to return a stale
StringKeybut a freshDerivedStringKeyafter invalidating onlyIntKey.
If the design decision changes, remember to update this duplicate test as well.
🧹 Nitpick comments (3)
Package.swift (1)
197-203: Consider declaringOpenSwiftUITestsSupportasrequiresTestsDependency(test-only) to avoid it leaking into client build graphs
OpenSwiftUITestsSupportis a regular target. Because no product exposes it, it will still be compiled whenever a consumer depends onOpenSwiftUIand requests thetestproduct set (e.g.swift test). That is fine, but it also means:
- The code is built in release configuration when consumers run their own unit-tests, so any accidental public API in this target becomes part of the transitive dependency graph.
- SwiftPM does not presently have a first-class “test-support” target kind, but you can achieve the same isolation by
isTestTarget: true(Swift 6) or by keeping everythinginternal/packageand adding aTARGET_IS_TEST_SUPPORTdefine to fail the build if somebody tries to import it from a non-test target.If you want to guarantee that this helper can never be imported by production code, consider:
- let openSwiftUITestsSupportTarget = Target.target( + let openSwiftUITestsSupportTarget = Target.target( name: "OpenSwiftUITestsSupport", dependencies: ["OpenSwiftUI"], swiftSettings: sharedSwiftSettings + [ + // Ensures the module can only be used from test targets. + .define("OPENSWIFTUI_TEST_SUPPORT") ] )and gate every
publicdeclaration behind#if OPENSWIFTUI_TEST_SUPPORT.
This is optional, but prevents accidental API surface growth.Sources/OpenSwiftUITestsSupport/Data/PropertyList+Test.swift (1)
19-29: Minor consistency nit – favourstatic letfor constant defaults
BoolKeyandIntKeyusestatic let defaultValue, whereas
StringKeydeclares a computedstatic var. For a compile-time constant
string the read-onlyletform is marginally faster and keeps style
uniform across all test keys.-package struct StringKey: PropertyKey { - package static var defaultValue: String { "" } +package struct StringKey: PropertyKey { + package static let defaultValue = "" }Tests/OpenSwiftUISymbolDualTests/Data/PropertyListTests.swift (1)
82-100: Same brittleness concern as Core tests – avoid hard-coding the fulldescriptionThe
swiftUI_descriptionassertions duplicate the ordering & sentinel
coupling discussed for the Core test suite. A change in internal element
layout will break two test files. Consider extracting shared helper
assertions (e.g.assertContains(plist, key: StringKey.self, value: "override"))
intoOpenSwiftUITestsSupportto remove duplication and make future updates
trivial.Also applies to: 142-144
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
Package.swift(8 hunks)Sources/OpenSwiftUICore/Data/Other/BloomFilter.swift(0 hunks)Sources/OpenSwiftUICore/Data/Property/DerivedPropertyKey.swift(0 hunks)Sources/OpenSwiftUICore/Data/Property/PropertyKey.swift(0 hunks)Sources/OpenSwiftUICore/Data/Property/PropertyList.swift(0 hunks)Sources/OpenSwiftUICore/Data/PropertyList.swift(1 hunks)Sources/OpenSwiftUICore/Data/State/ObservableObjectLocation.swift(1 hunks)Sources/OpenSwiftUICore/Data/Update.swift(1 hunks)Sources/OpenSwiftUICore/Semantic/SemanticFeature.swift(1 hunks)Sources/OpenSwiftUICore/Util/RunLoopUtils.swift(1 hunks)Sources/OpenSwiftUICore/View/Graph/ViewRendererHost.swift(1 hunks)Sources/OpenSwiftUISymbolDualTestsSupport/Data/PropertyListTestsStub.c(1 hunks)Sources/OpenSwiftUISymbolDualTestsSupport/Extension/CGSize+ExtensionTestsStub.c(1 hunks)Sources/OpenSwiftUITestsSupport/Data/PropertyList+Test.swift(1 hunks)Tests/OpenSwiftUICoreTests/Data/Property/PropertyListTests.swift(0 hunks)Tests/OpenSwiftUICoreTests/Data/PropertyListTests.swift(1 hunks)Tests/OpenSwiftUISymbolDualTests/Data/PropertyListTests.swift(1 hunks)Tests/OpenSwiftUISymbolDualTests/Extension/CGSize+ExtensionTests.swift(2 hunks)Tests/OpenSwiftUISymbolDualTests/README.md(1 hunks)
💤 Files with no reviewable changes (5)
- Tests/OpenSwiftUICoreTests/Data/Property/PropertyListTests.swift
- Sources/OpenSwiftUICore/Data/Property/PropertyKey.swift
- Sources/OpenSwiftUICore/Data/Property/DerivedPropertyKey.swift
- Sources/OpenSwiftUICore/Data/Other/BloomFilter.swift
- Sources/OpenSwiftUICore/Data/Property/PropertyList.swift
🧰 Additional context used
🧬 Code Graph Analysis (3)
Tests/OpenSwiftUICoreTests/Data/PropertyListTests.swift (3)
Sources/OpenSwiftUICore/Data/PropertyList.swift (14)
override(86-102)valueWithSecondaryLookup(132-144)valueWithSecondaryLookup(456-471)value(277-279)value(439-454)value(741-743)value(783-785)merging(271-275)invalidateValue(494-506)derivedValue(473-488)hasDifferentUsedValues(519-540)invalidateAllValues(508-517)formUnion(542-544)formUnion(579-596)Sources/OpenSwiftUITestsSupport/Data/PropertyList+Test.swift (2)
value(20-22)value(26-28)Sources/OpenSwiftUICore/Data/Other/BloomFilter.swift (1)
formUnion(46-49)
Tests/OpenSwiftUISymbolDualTests/Data/PropertyListTests.swift (4)
Sources/OpenSwiftUITestsSupport/Data/PropertyList+Test.swift (2)
value(20-22)value(26-28)Sources/OpenSwiftUICore/Data/PropertyList.swift (12)
value(277-279)value(439-454)value(741-743)value(783-785)override(86-102)valueWithSecondaryLookup(132-144)valueWithSecondaryLookup(456-471)merging(271-275)invalidateValue(494-506)invalidateAllValues(508-517)formUnion(542-544)formUnion(579-596)Tests/OpenSwiftUICoreTests/Data/PropertyListTests.swift (7)
override(10-92)valueWithSecondaryLookup(94-104)description(106-126)merging(128-144)invalidateValue(148-173)invalidateAllValues(175-203)formUnion(205-236)Sources/OpenSwiftUICore/Data/Other/BloomFilter.swift (1)
formUnion(46-49)
Sources/OpenSwiftUITestsSupport/Data/PropertyList+Test.swift (2)
Tests/OpenSwiftUISymbolDualTests/Data/PropertyListTests.swift (1)
value(69-71)Sources/OpenSwiftUICore/Data/PropertyList.swift (4)
value(277-279)value(439-454)value(741-743)value(783-785)
🪛 SwiftLint (0.57.0)
Tests/OpenSwiftUISymbolDualTests/Data/PropertyListTests.swift
[Warning] 213-213: FIXMEs should be resolved (Compiler limitation)
(todo)
[Warning] 26-26: Setter value is not used
(unused_setter_value)
Sources/OpenSwiftUICore/Data/PropertyList.swift
[Error] 71-71: Force casts should be avoided
(force_cast)
[Warning] 637-637: This memberwise initializer would be synthesized automatically - you do not need to define it
(unneeded_synthesized_initializer)
[Warning] 180-180: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Execute compatibility tests for iOS (macos-15, 16.0, 2024, 18.0)
- GitHub Check: Execute tests on iOS (macos-15, 16.0, 2021, 18.0)
- GitHub Check: Execute tests on iOS (macos-15, 16.0, 2024, 18.0)
- GitHub Check: Execute tests on Ubuntu (6.0.1)
🔇 Additional comments (16)
Sources/OpenSwiftUICore/View/Graph/ViewRendererHost.swift (1)
241-241: Clean stylistic update.Updated the loop condition syntax by removing parentheses from
while(true)towhile true. This change improves code readability and maintains consistent style with other loop conditions in the codebase.Sources/OpenSwiftUICore/Util/RunLoopUtils.swift (1)
61-61: Consistent style improvement.Updated the loop condition syntax by removing parentheses from the
whilestatement, following the same pattern applied to other loop conditions in the codebase. This improves readability and maintains a consistent code style.Sources/OpenSwiftUICore/Data/Update.swift (1)
176-176: Style consistency improvement.Updated the do-while loop condition syntax by removing parentheses, maintaining style consistency with other loop conditions in the codebase.
Sources/OpenSwiftUISymbolDualTestsSupport/Extension/CGSize+ExtensionTestsStub.c (1)
3-3: Updated file header to reflect target rename.The comment header now correctly reflects the renamed target
OpenSwiftUISymbolDualTestsSupport(previouslyOpenSwiftUISymbolDualTestsHelper), maintaining consistency with the changes made in Package.swift.Sources/OpenSwiftUICore/Semantic/SemanticFeature.swift (1)
11-11: Protocol inheritance from Feature improves type hierarchy.Making
SemanticFeatureinherit fromFeatureprotocol establishes a clear hierarchical relationship and better aligns with the overall architecture.This change will allow code to leverage polymorphism when working with features, making it possible to handle
SemanticFeatureinstances through the more generalFeatureprotocol.Tests/OpenSwiftUISymbolDualTests/README.md (2)
14-14: Renamed property to follow SwiftUI naming convention.Renaming from
swiftUIHasZerotoswiftUI_hasZerofollows a more consistent naming pattern for SwiftUI extensions.
21-21: Improved test expectation clarity.The updated expectation
size.swiftUI_hasZero == trueis more explicit than the previous form and correctly matches the property rename.Tests/OpenSwiftUISymbolDualTests/Extension/CGSize+ExtensionTests.swift (3)
11-11: Renamed property for consistency with README example.The property rename to
swiftUI_hasZeromaintains consistency with the README documentation.
26-33: Improved formatting for test arguments.The reordering and reformatting of the
@Testattributes and arguments improves readability and follows a more consistent style.
35-36: Simplified test implementation.The test function has been streamlined to check only the relevant property against the expected result, making the test more focused and easier to understand.
Sources/OpenSwiftUISymbolDualTestsSupport/Data/PropertyListTestsStub.c (2)
9-10: Prefer#include(or rename the file to.m) to avoid Objective-C semantics leakage
#importis accepted by Clang in C mode, but it is primarily an Objective-C directive.
Using it in a plain.cfile can surprise other compilers/tool-chains and breaks
portability. Either:
- Replace
#importwith standard#include, or- Rename the file to
.m/.mmso the intention to use Objective-C
headers is explicit.-#import <SymbolLocator.h> +#include <SymbolLocator.h>This small tweak prevents accidental compiler-specific behaviour.
[ suggest_optional_refactor ]
7-8: Guard non-Darwin builds with an#elsestubThe whole file is wrapped in
#if OPENSWIFTUI_TARGET_OS_DARWIN, but there is no
#elsefallback.
When a non-Darwin target is compiled the identifier
OpenSwiftUITestStub_PropertyListInit(and friends) will be missing,
breaking linkage for tests that are still compiled on those platforms.Consider adding:
#else /* Empty stubs so the symbols still exist on non-Darwin builds */ #define DEFINE_SL_STUB_SLF(name, module, symbol) \ void name(void) __attribute__((weak_import)); \ void name(void) {} #endif(or conditionally exclude the test target completely).
[ suggest_optional_refactor ]Sources/OpenSwiftUICore/Data/PropertyList.swift (4)
67-72: Avoid force-castingdata– make the initializer safer
elements = (data as! Element)will trap at runtime if a wrong object is
passed. A defensive cast keeps the library safe in Debug and Release
builds alike:- elements = (data as! Element) + guard let element = data as? Element else { + assertionFailure("Expected PropertyList.Element, got \(type(of: data))") + return + } + elements = elementFail-fast with a clear message helps catch integration mistakes while still
preventing a hard crash in production.
[ flag_critical_issue ]🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 71-71: Force casts should be avoided
(force_cast)
176-187: Replace the unusedstopparameter with_to silence SwiftLintThe closure passed to
forEachdoes not mutatestop. Renaming it to_
removes the “unused parameter” warning and clarifies the intent:- elements.forEach(filter: BloomFilter()) { element, stop in + elements.forEach(filter: BloomFilter()) { element, _ inNo functional change, purely a lint / readability improvement.
[ suggest_nitpick ]🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 180-180: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
216-219: Typo:currentOptinoalOtherElement→currentOptionalOtherElementA misspelt variable name hampers readability and can trip search-based
refactors.- var currentOptinoalOtherElement: Element? = otherElements + var currentOptionalOtherElement: Element? = otherElements(Adjust the four subsequent occurrences.)
[ suggest_nitpick ]
637-639: The custom initializer is redundantSwift will synthesise the exact same member-wise
init(value:)for
SecondaryLookupTrackedValue. Unless additional side-effects are planned,
the explicit definition (and the accompanying SwiftLint warning) can be
removed.If you keep it for future expansion, mark it with
// swiftlint:disable
to document the intent.
[ suggest_optional_refactor ]🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 637-637: This memberwise initializer would be synthesized automatically - you do not need to define it
(unneeded_synthesized_initializer)
| // newElement = element.byPrepending(element) | ||
| newElement = nil |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Setting newElement to nil instead of using byPrepending may change behavior.
The previous implementation used element.byPrepending(element) to create a new element, but now you're explicitly setting newElement to nil. This seems like a significant behavioral change that might affect transaction handling with property lists.
Are you sure this change won't break property list element chaining in transactions? The commented-out line suggests this might be a temporary solution. If this is intentional as part of the PropertyList refactoring, please confirm the behavior is still correct.
🏁 Script executed:
#!/bin/bash
# Check for PropertyList.Element usage and implementations
echo "Searching for PropertyList.Element usage:"
rg "PropertyList.Element" --type swift -A 3 -B 3Length of output: 6571
To verify if the commented‐out byPrepending call has truly been removed and to see how newElement is used downstream, let’s:
- Search for any remaining
byPrependingdefinitions or references. - Dump the full
set(_:transaction:)implementation to see what happens withnewElementafter it’s set tonil.
🏁 Script executed:
#!/bin/bash
echo "Searching for byPrepending references:"
rg -n "byPrepending" --type swift
echo -e "\nContext for set(_:transaction:) in ObservableObjectLocation.swift:"
rg -n "func set" -A 50 --type swift Sources/OpenSwiftUICore/Data/State/ObservableObjectLocation.swiftLength of output: 1568
Restore or implement proper byPrepending logic for transaction chaining
By setting newElement = nil when transaction.plist.elements is non-nil, you’re effectively dropping the previous head of the property-list chain—any nested or subsequent mutations in the same transaction will lose their linkage. The original call to
newElement = element.byPrepending(element)must either be re-enabled (with a working byPrepending(_:) implementation on PropertyList.Element) or replaced with equivalent logic.
– File: Sources/OpenSwiftUICore/Data/State/ObservableObjectLocation.swift
– Lines: 28–30
Suggested fix:
- if let element = transaction.plist.elements {
- // newElement = element.byPrepending(element)
- newElement = nil
- } else {
- newElement = element
- }
+ if let head = transaction.plist.elements {
+ // Prepend a new element to the existing chain
+ newElement = head.byPrepending(head)
+ } else {
+ newElement = element
+ }Ensure you’ve implemented byPrepending(_:) on PropertyList.Element and covered this in your unit tests.
| #expect(basePlist.description == #""" | ||
| [StringKey = base, BoolKey = true, IntKey = 10] | ||
| """#) | ||
|
|
||
| var overridePlist = PropertyList() | ||
| overridePlist[IntKey.self] = 20 | ||
| overridePlist[StringKey.self] = "override" | ||
| #expect(overridePlist.description == #""" | ||
| [StringKey = override, IntKey = 20] | ||
| """#) | ||
|
|
||
| // Override basePlist with overridePlist | ||
| basePlist.override(with: overridePlist) | ||
| #expect(basePlist[IntKey.self] == 20) | ||
| #expect(basePlist[BoolKey.self] == true) | ||
| #expect(basePlist[StringKey.self] == "override") | ||
| #expect(basePlist.description == #""" | ||
| [StringKey = override, IntKey = 20, StringKey = base, BoolKey = true, IntKey = 10] | ||
| """#) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
description assertions are brittle and couple tests to implementation details
The unit-tests compare the entire PropertyList.description string, including:
- element order
- presence of the sentinel
EmptyKey - exact formatting
These are internal details that may legitimately change without breaking observable behaviour.
When that happens, the test suite will fail even though the library is correct.
Prefer semantic checks, e.g.
let desc = basePlist.description
#expect(desc.contains("IntKey = 20"))
#expect(desc.contains("BoolKey = true"))
#expect(desc.split(separator: ",").count == 3) // etc.or expose a stable, order-independent accessor (e.g. var asDictionary: [AnyHashable: Any]) specifically for testing.
This will make the tests resistant to harmless refactors.
Also applies to: 77-80, 138-144
| tracker.invalidateValue(for: IntKey.self, from: plist, to: newPlist) | ||
| #expect(tracker.value(newPlist, for: IntKey.self) == 100) | ||
| #expect(tracker.derivedValue(newPlist, for: DerivedIntPlus2Key.self) == 102) | ||
| #expect(tracker.value(newPlist, for: StringKey.self) == "original") | ||
| #expect(tracker.derivedValue(newPlist, for: DerivedStringKey.self) == "d:modified") | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Tracker now returns stale primary values but recomputed derived values – is this intentional?
After calling
tracker.invalidateValue(for: IntKey.self, from: plist, to: newPlist)
the test expects:
tracker.value(newPlist, for: StringKey.self) == "original" // stale
tracker.derivedValue(newPlist, for: DerivedStringKey.self) == "d:modified" // fresh
Because only IntKey was invalidated, cached StringKey survives, yet
DerivedStringKey (which depends on StringKey) is recomputed and now observes a different underlying value.
This breaks the intuitive invariant “derived value is a pure function of its
primitives” and can lead to very hard-to-debug inconsistencies in real usage.
If this behaviour is by design, a prominent comment would help future
maintainers; otherwise, consider also invalidating the directly-tracked value
that any invalidated derived value depends on.
I’ll locate where invalidateValue and derivedValue are implemented to verify how dependency invalidation is handled. Proceeding with script execution.
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Searching for invalidateValue occurrences ==="
rg "invalidateValue" -n .
echo -e "\n=== Context around invalidateValue ==="
rg -n "invalidateValue" -C3 .
echo -e "\n=== Searching for derivedValue occurrences ==="
rg "derivedValue" -n .
echo -e "\n=== Context around derivedValue ==="
rg -n "derivedValue" -C3 .Length of output: 33804
Clarify derived vs. primary cache invalidation
The behavior you’ve flagged is indeed how the tracker is implemented today:
invalidateValue(for:)removes only the targeted primary key fromdata.values- it then moves all derived caches into
data.invalidValues - on the next call,
value(_:, for:)will return a stale entry for any non-invalidated primary,
whereasderivedValue(_:, for:)always recomputes from the rawPropertyList
If this is intentional, we should make it explicit in the code. Please add a comment in Sources/OpenSwiftUICore/Data/PropertyList.swift just above line 494 (the invalidateValue<K>(…) definition), for example:
/// Invalidate only the specified primary key and _all_ derived caches.
/// This means:
/// • Primary values for other keys remain cached (and thus stale)
/// • Derived values are always recomputed from the raw `PropertyList`
///
/// This design ensures minimal invalidation on a single-key change,
/// but can lead to derived ⧏ primary inconsistencies if raw and cached
/// values diverge. Adjust if you need a different invalidation strategy.
final package func invalidateValue<K>(for key: K.Type, from oldPlist: PropertyList, to newPlist: PropertyList) where K: PropertyKey {
…
}This will help future maintainers understand that stale-primary/fresh-derived behavior is by design.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #267 +/- ##
==========================================
+ Coverage 20.96% 22.02% +1.05%
==========================================
Files 337 337
Lines 15492 15660 +168
==========================================
+ Hits 3248 3449 +201
+ Misses 12244 12211 -33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Tests
Documentation