address PR comments

chloe-yeo · chloe-yeo · commit 705d181455f6 · 2025-09-23T10:34:01.000-07:00
diff --git a/Sources/FoundationEssentials/ProgressManager/ProgressFraction.swift b/Sources/FoundationEssentials/ProgressManager/ProgressFraction.swift
@@ -92,7 +92,6 @@ internal struct ProgressFraction : Sendable, Equatable, CustomDebugStringConvert
             return ProgressFraction(double: operation(lhs.fractionCompleted, rhs.fractionCompleted), overflow: true)
         }
 
-        //TODO: rdar://148758226 Overflow check 
         if let lcm = _leastCommonMultiple(lhsTotal, rhsTotal) {
             let result = overflowOperation(lhs.completed * (lcm / lhsTotal), rhs.completed * (lcm / rhsTotal))
             if result.overflow {
diff --git a/Sources/FoundationEssentials/ProgressManager/ProgressManager+Interop.swift b/Sources/FoundationEssentials/ProgressManager/ProgressManager+Interop.swift
@@ -71,7 +71,7 @@ extension Progress {
     
     // MARK: Cycle detection
     private func isCycle(reporter: ProgressReporter, visited: Set<ProgressManager> = []) -> Bool {
-        if self._parent() == nil {
+        guard let parent = self._parent() else {
             return false
         }
         
@@ -132,12 +132,9 @@ internal final class SubprogressBridge: Sendable {
                 return
             }
             
-            switch observerState {
-            case .fractionUpdated(let totalCount, let completedCount):
-                // This needs to change totalUnitCount before completedUnitCount otherwise progressBridge will finish and mess up the math
-                self.progressBridge.totalUnitCount = Int64(totalCount)
-                self.progressBridge.completedUnitCount = Int64(completedCount)
-            }
+            // This needs to change totalUnitCount before completedUnitCount otherwise progressBridge will finish and mess up the math
+            self.progressBridge.totalUnitCount = Int64(observerState.totalCount)
+            self.progressBridge.completedUnitCount = Int64(observerState.completedCount)
         }
     }
 }
@@ -164,11 +161,8 @@ internal final class ProgressReporterBridge: Sendable {
                 return
             }
             
-            switch observerState {
-            case .fractionUpdated(let totalCount, let completedCount):
-                self.progressBridge.totalUnitCount = Int64(totalCount)
-                self.progressBridge.completedUnitCount = Int64(completedCount)
-            }
+            self.progressBridge.totalUnitCount = Int64(observerState.totalCount)
+            self.progressBridge.completedUnitCount = Int64(observerState.completedCount)
         }
     }
     
@@ -183,18 +177,18 @@ internal final class NSProgressBridge: Progress, @unchecked Sendable {
 
     init(manager: ProgressManager, progress: Progress, portion: Int) {
         self.manager = manager
-        self.managerBridge = ProgressManager(totalCount: Int(progress.totalUnitCount))
+        self.managerBridge = ProgressManager(totalCount: Int(clamping: progress.totalUnitCount))
         self.progress = progress
         super.init(parent: nil, userInfo: nil)
         
         managerBridge.setCounts { completed, total in
-            completed = Int(progress.completedUnitCount)
+            completed = Int(clamping: progress.completedUnitCount)
         }
         
         let position = manager.addChild(
             child: managerBridge,
             portion: portion,
-            childFraction: ProgressFraction(completed: Int(completedUnitCount), total: Int(totalUnitCount))
+            childFraction: ProgressFraction(completed: Int(clamping: completedUnitCount), total: Int(clamping: totalUnitCount))
         )
         managerBridge.addParent(parent: manager, positionInParent: position)
     }
@@ -203,8 +197,8 @@ internal final class NSProgressBridge: Progress, @unchecked Sendable {
     // so that the parent that gets updated is the ProgressManager parent
     override func _updateChild(_ child: Foundation.Progress, fraction: _NSProgressFractionTuple, portion: Int64) {
         managerBridge.setCounts { completed, total in
-            completed = Int(fraction.next.completed)
-            total = Int(fraction.next.total)
+            completed = Int(clamping: fraction.next.completed)
+            total = Int(clamping: fraction.next.total)
         }
         managerBridge.markSelfDirty()
     }
@@ -213,8 +207,9 @@ internal final class NSProgressBridge: Progress, @unchecked Sendable {
 @available(FoundationPreview 6.4, *)
 extension ProgressManager {
     // Keeping this as an enum in case we have other states to track in the future.
-    internal enum ObserverState {
-        case fractionUpdated(totalCount: Int, completedCount: Int)
+    internal struct ObserverState {
+        var totalCount: Int
+        var completedCount: Int
     }
     
     internal struct InteropObservation {
diff --git a/Sources/FoundationEssentials/ProgressManager/ProgressManager+State.swift b/Sources/FoundationEssentials/ProgressManager/ProgressManager+State.swift
@@ -139,7 +139,7 @@ extension ProgressManager {
 
         /// Returns nil if `self` was instantiated without total units;
         /// returns a `Int` value otherwise.
-        internal func getTotalCount() -> Int? {
+        internal var totalCount: Int? {
 #if FOUNDATION_FRAMEWORK
             if let interopTotalCount = interopType?.totalCount {
                 return interopTotalCount
@@ -150,7 +150,7 @@ extension ProgressManager {
         
         /// Returns 0 if `self` has `nil` total units;
         /// returns a `Int` value otherwise.
-        internal mutating func getCompletedCount() -> Int {
+        internal mutating func completedCount() -> Int {
 #if FOUNDATION_FRAMEWORK
             if let interopCompletedCount = interopType?.completedCount {
                 return interopCompletedCount
@@ -160,7 +160,7 @@ extension ProgressManager {
             return selfFraction.completed
         }
         
-        internal mutating func getFractionCompleted() -> Double {
+        internal mutating func fractionCompleted() -> Double {
 #if FOUNDATION_FRAMEWORK
             if let interopFractionCompleted = interopType?.fractionCompleted {
                 return interopFractionCompleted
@@ -170,7 +170,7 @@ extension ProgressManager {
             return overallFraction.fractionCompleted
         }
         
-        internal func getIsIndeterminate() -> Bool {
+        internal var isIndeterminate: Bool {
 #if FOUNDATION_FRAMEWORK
             if let interopIsIndeterminate = interopType?.isIndeterminate {
                 return interopIsIndeterminate
@@ -179,7 +179,7 @@ extension ProgressManager {
             return selfFraction.isIndeterminate
         }
         
-        internal mutating func getIsFinished() -> Bool {
+        internal mutating func isFinished() -> Bool {
 #if FOUNDATION_FRAMEWORK
             if let interopIsFinished = interopType?.isFinished {
                 return interopIsFinished
@@ -227,15 +227,15 @@ extension ProgressManager {
             switch interopType {
             case .interopObservation(let observation):
                 observation.subprogressBridge?.manager.notifyObservers(
-                    with: .fractionUpdated(
+                    with: ObserverState(
                         totalCount: selfFraction.total ?? 0,
                         completedCount: selfFraction.completed
                     )
                 )
                 
                 if let _ = observation.reporterBridge {
                     notifyObservers(
-                        with: .fractionUpdated(
+                        with: ObserverState(
                             totalCount: selfFraction.total ?? 0,
                             completedCount: selfFraction.completed
                         )
@@ -251,6 +251,9 @@ extension ProgressManager {
         
         // MARK: Mark paths dirty
         internal mutating func markChildDirty(at position: Int) -> [ParentState]? {
+            guard position >= 0 && position < children.count else {
+                return nil
+            }
             guard !children[position].isDirty else {
                 return nil
             }
@@ -259,66 +262,105 @@ extension ProgressManager {
         }
         
         internal mutating func markChildDirty(property: MetatypeWrapper<Int, Int>, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].childPropertiesInt[property]?.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: MetatypeWrapper<UInt64, UInt64>, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].childPropertiesUInt64[property]?.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: MetatypeWrapper<Double, Double>, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].childPropertiesDouble[property]?.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: MetatypeWrapper<String?, [String?]>, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].childPropertiesString[property]?.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: MetatypeWrapper<URL?, [URL?]>, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].childPropertiesURL[property]?.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: MetatypeWrapper<UInt64, [UInt64]>, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].childPropertiesUInt64Array[property]?.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: MetatypeWrapper<Duration, Duration>, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].childPropertiesDuration[property]?.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: ProgressManager.Properties.TotalFileCount.Type, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].totalFileCount.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: ProgressManager.Properties.CompletedFileCount.Type, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].completedFileCount.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: ProgressManager.Properties.TotalByteCount.Type, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].totalByteCount.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: ProgressManager.Properties.CompletedByteCount.Type, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].completedByteCount.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: ProgressManager.Properties.Throughput.Type, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].throughput.isDirty = true
             return parents
         }
         
         internal mutating func markChildDirty(property: ProgressManager.Properties.EstimatedTimeRemaining.Type, at position: Int) -> [ParentState] {
+            guard position >= 0 && position < children.count else {
+                return parents
+            }
             children[position].estimatedTimeRemaining.isDirty = true
             return parents
         }
diff --git a/Sources/FoundationEssentials/ProgressManager/ProgressManager.swift b/Sources/FoundationEssentials/ProgressManager/ProgressManager.swift
@@ -40,15 +40,15 @@ internal import _FoundationCollections
     public var totalCount: Int? {
         self.access(keyPath: \.totalCount)
         return state.withLock { state in
-            state.getTotalCount()
+            state.totalCount
         }
     }
     
     /// The completed units of work.
     public var completedCount: Int {
         self.access(keyPath: \.completedCount)
         let (children, completedCount) =  state.withLock { state in
-            (state.children.compactMap { $0.child }, state.getCompletedCount())
+            (state.children.compactMap { $0.child }, state.completedCount())
         }
         for child in children {
             child.access(keyPath: \.completedCount)
@@ -63,7 +63,7 @@ internal import _FoundationCollections
         self.access(keyPath: \.totalCount)
         self.access(keyPath: \.completedCount)
         let (children, fractionCompleted) = state.withLock { state in
-            (state.children.compactMap { $0.child }, state.getFractionCompleted())
+            (state.children.compactMap { $0.child }, state.fractionCompleted())
         }
         for child in children {
             child.access(keyPath: \.totalCount)
@@ -77,7 +77,7 @@ internal import _FoundationCollections
     public var isIndeterminate: Bool {
         self.access(keyPath: \.totalCount)
         return state.withLock { state in
-            state.getIsIndeterminate()
+            state.isIndeterminate
         }
     }
     
@@ -87,7 +87,7 @@ internal import _FoundationCollections
         self.access(keyPath: \.totalCount)
         self.access(keyPath: \.completedCount)
         let (children, isFinished) = state.withLock { state in
-            (state.children.compactMap { $0.child }, state.getIsFinished())
+            (state.children.compactMap { $0.child }, state.isFinished())
         }
         for child in children {
             child.access(keyPath: \.totalCount)

Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,6 @@ internal struct ProgressFraction : Sendable, Equatable, CustomDebugStringConvert`
`92`	`92`	`return ProgressFraction(double: operation(lhs.fractionCompleted, rhs.fractionCompleted), overflow: true)`
`93`	`93`	`}`
`94`	`94`
`95`		`- //TODO: rdar://148758226 Overflow check`
`96`	`95`	`if let lcm = _leastCommonMultiple(lhsTotal, rhsTotal) {`
`97`	`96`	`let result = overflowOperation(lhs.completed * (lcm / lhsTotal), rhs.completed * (lcm / rhsTotal))`
`98`	`97`	`if result.overflow {`
Original file line number	Diff line number	Diff line change
`@@ -71,7 +71,7 @@ extension Progress {`
`71`	`71`
`72`	`72`	`// MARK: Cycle detection`
`73`	`73`	`private func isCycle(reporter: ProgressReporter, visited: Set<ProgressManager> = []) -> Bool {`
`74`		`- if self._parent() == nil {`
	`74`	`+ guard let parent = self._parent() else {`
`75`	`75`	`return false`
`76`	`76`	`}`
`77`	`77`
`@@ -132,12 +132,9 @@ internal final class SubprogressBridge: Sendable {`
`132`	`132`	`return`
`133`	`133`	`}`
`134`	`134`
`135`		`- switch observerState {`
`136`		`- case .fractionUpdated(let totalCount, let completedCount):`
`137`		`- // This needs to change totalUnitCount before completedUnitCount otherwise progressBridge will finish and mess up the math`
`138`		`- self.progressBridge.totalUnitCount = Int64(totalCount)`
`139`		`- self.progressBridge.completedUnitCount = Int64(completedCount)`
`140`		`- }`
	`135`	`+ // This needs to change totalUnitCount before completedUnitCount otherwise progressBridge will finish and mess up the math`
	`136`	`+ self.progressBridge.totalUnitCount = Int64(observerState.totalCount)`
	`137`	`+ self.progressBridge.completedUnitCount = Int64(observerState.completedCount)`
`141`	`138`	`}`
`142`	`139`	`}`
`143`	`140`	`}`
`@@ -164,11 +161,8 @@ internal final class ProgressReporterBridge: Sendable {`
`164`	`161`	`return`
`165`	`162`	`}`
`166`	`163`
`167`		`- switch observerState {`
`168`		`- case .fractionUpdated(let totalCount, let completedCount):`
`169`		`- self.progressBridge.totalUnitCount = Int64(totalCount)`
`170`		`- self.progressBridge.completedUnitCount = Int64(completedCount)`
`171`		`- }`
	`164`	`+ self.progressBridge.totalUnitCount = Int64(observerState.totalCount)`
	`165`	`+ self.progressBridge.completedUnitCount = Int64(observerState.completedCount)`
`172`	`166`	`}`
`173`	`167`	`}`
`174`	`168`
`@@ -183,18 +177,18 @@ internal final class NSProgressBridge: Progress, @unchecked Sendable {`
`183`	`177`
`184`	`178`	`init(manager: ProgressManager, progress: Progress, portion: Int) {`
`185`	`179`	`self.manager = manager`
`186`		`- self.managerBridge = ProgressManager(totalCount: Int(progress.totalUnitCount))`
	`180`	`+ self.managerBridge = ProgressManager(totalCount: Int(clamping: progress.totalUnitCount))`
`187`	`181`	`self.progress = progress`
`188`	`182`	`super.init(parent: nil, userInfo: nil)`
`189`	`183`
`190`	`184`	`managerBridge.setCounts { completed, total in`
`191`		`- completed = Int(progress.completedUnitCount)`
	`185`	`+ completed = Int(clamping: progress.completedUnitCount)`
`192`	`186`	`}`
`193`	`187`
`194`	`188`	`let position = manager.addChild(`
`195`	`189`	`child: managerBridge,`
`196`	`190`	`portion: portion,`
`197`		`- childFraction: ProgressFraction(completed: Int(completedUnitCount), total: Int(totalUnitCount))`
	`191`	`+ childFraction: ProgressFraction(completed: Int(clamping: completedUnitCount), total: Int(clamping: totalUnitCount))`
`198`	`192`	`)`
`199`	`193`	`managerBridge.addParent(parent: manager, positionInParent: position)`
`200`	`194`	`}`
`@@ -203,8 +197,8 @@ internal final class NSProgressBridge: Progress, @unchecked Sendable {`
`203`	`197`	`// so that the parent that gets updated is the ProgressManager parent`
`204`	`198`	`override func _updateChild(_ child: Foundation.Progress, fraction: _NSProgressFractionTuple, portion: Int64) {`
`205`	`199`	`managerBridge.setCounts { completed, total in`
`206`		`- completed = Int(fraction.next.completed)`
`207`		`- total = Int(fraction.next.total)`
	`200`	`+ completed = Int(clamping: fraction.next.completed)`
	`201`	`+ total = Int(clamping: fraction.next.total)`
`208`	`202`	`}`
`209`	`203`	`managerBridge.markSelfDirty()`
`210`	`204`	`}`
`@@ -213,8 +207,9 @@ internal final class NSProgressBridge: Progress, @unchecked Sendable {`
`213`	`207`	`@available(FoundationPreview 6.4, *)`
`214`	`208`	`extension ProgressManager {`
`215`	`209`	`// Keeping this as an enum in case we have other states to track in the future.`
`216`		`- internal enum ObserverState {`
`217`		`- case fractionUpdated(totalCount: Int, completedCount: Int)`
	`210`	`+ internal struct ObserverState {`
	`211`	`+ var totalCount: Int`
	`212`	`+ var completedCount: Int`
`218`	`213`	`}`
`219`	`214`
`220`	`215`	`internal struct InteropObservation {`