From 2575114c1881722285c7962d1499fb8ada3f68e8 Mon Sep 17 00:00:00 2001 From: Rachel Brindle Date: Sat, 12 Aug 2023 20:56:07 -0700 Subject: [PATCH] Make AsyncPredicate Sendable and operate only on Sendable types (#1072) Make Predicate's closure Sendable, and make Predicate Sendable when the returning value is Sendable --- Sources/Nimble/ExpectationMessage.swift | 4 +- Sources/Nimble/Matchers/AsyncMatcher.swift | 2 +- Sources/Nimble/Matchers/Matcher.swift | 28 ++++---- .../Nimble/Matchers/PostNotification.swift | 28 +++++++- Tests/NimbleTests/SynchronousTest.swift | 65 +++++++++++++------ 5 files changed, 88 insertions(+), 39 deletions(-) diff --git a/Sources/Nimble/ExpectationMessage.swift b/Sources/Nimble/ExpectationMessage.swift index 4efda7c01..2bff900e4 100644 --- a/Sources/Nimble/ExpectationMessage.swift +++ b/Sources/Nimble/ExpectationMessage.swift @@ -1,4 +1,4 @@ -public indirect enum ExpectationMessage { +public indirect enum ExpectationMessage: Sendable { // --- Primary Expectations --- /// includes actual value in output ("expected to , got ") case expectedActualValueTo(/* message: */ String) @@ -204,7 +204,7 @@ extension FailureMessage { #if canImport(Darwin) import class Foundation.NSObject -public class NMBExpectationMessage: NSObject { +public final class NMBExpectationMessage: NSObject, Sendable { private let msg: ExpectationMessage internal init(swift msg: ExpectationMessage) { diff --git a/Sources/Nimble/Matchers/AsyncMatcher.swift b/Sources/Nimble/Matchers/AsyncMatcher.swift index 6b8000298..78db294d3 100644 --- a/Sources/Nimble/Matchers/AsyncMatcher.swift +++ b/Sources/Nimble/Matchers/AsyncMatcher.swift @@ -3,7 +3,7 @@ public protocol AsyncableMatcher: Sendable { func satisfies(_ expression: AsyncExpression) async throws -> MatcherResult } -extension Matcher: AsyncableMatcher where T: Sendable { +extension Matcher: AsyncableMatcher { public func satisfies(_ expression: AsyncExpression) async throws -> MatcherResult { try satisfies(await expression.toSynchronousExpression()) } diff --git a/Sources/Nimble/Matchers/Matcher.swift b/Sources/Nimble/Matchers/Matcher.swift index 375419e4c..8f31e1ca0 100644 --- a/Sources/Nimble/Matchers/Matcher.swift +++ b/Sources/Nimble/Matchers/Matcher.swift @@ -19,10 +19,10 @@ /// renamed `NSMatcher` to `Matcher`. In response, we decided to rename `Matcher` to /// `Matcher`. public struct Matcher { - fileprivate var matcher: (Expression) throws -> MatcherResult + fileprivate let matcher: @Sendable (Expression) throws -> MatcherResult /// Constructs a matcher that knows how take a given value - public init(_ matcher: @escaping (Expression) throws -> MatcherResult) { + public init(_ matcher: @escaping @Sendable (Expression) throws -> MatcherResult) { self.matcher = matcher } @@ -39,10 +39,12 @@ public struct Matcher { @available(*, deprecated, renamed: "Matcher") public typealias Predicate = Matcher +extension Matcher: Sendable where T: Sendable {} + /// Provides convenience helpers to defining matchers extension Matcher { /// Like Matcher() constructor, but automatically guard against nil (actual) values - public static func define(matcher: @escaping (Expression) throws -> MatcherResult) -> Matcher { + public static func define(matcher: @escaping @Sendable (Expression) throws -> MatcherResult) -> Matcher { return Matcher { actual in return try matcher(actual) }.requireNonNil @@ -50,7 +52,7 @@ extension Matcher { /// Defines a matcher with a default message that can be returned in the closure /// Also ensures the matcher's actual value cannot pass with `nil` given. - public static func define(_ message: String = "match", matcher: @escaping (Expression, ExpectationMessage) throws -> MatcherResult) -> Matcher { + public static func define(_ message: String = "match", matcher: @escaping @Sendable (Expression, ExpectationMessage) throws -> MatcherResult) -> Matcher { return Matcher { actual in return try matcher(actual, .expectedActualValueTo(message)) }.requireNonNil @@ -58,7 +60,7 @@ extension Matcher { /// Defines a matcher with a default message that can be returned in the closure /// Unlike `define`, this allows nil values to succeed if the given closure chooses to. - public static func defineNilable(_ message: String = "match", matcher: @escaping (Expression, ExpectationMessage) throws -> MatcherResult) -> Matcher { + public static func defineNilable(_ message: String = "match", matcher: @escaping @Sendable (Expression, ExpectationMessage) throws -> MatcherResult) -> Matcher { return Matcher { actual in return try matcher(actual, .expectedActualValueTo(message)) } @@ -70,7 +72,7 @@ extension Matcher { /// error message. /// /// Also ensures the matcher's actual value cannot pass with `nil` given. - public static func simple(_ message: String = "match", matcher: @escaping (Expression) throws -> MatcherStatus) -> Matcher { + public static func simple(_ message: String = "match", matcher: @escaping @Sendable (Expression) throws -> MatcherStatus) -> Matcher { return Matcher { actual in return MatcherResult(status: try matcher(actual), message: .expectedActualValueTo(message)) }.requireNonNil @@ -80,7 +82,7 @@ extension Matcher { /// error message. /// /// Unlike `simple`, this allows nil values to succeed if the given closure chooses to. - public static func simpleNilable(_ message: String = "match", matcher: @escaping (Expression) throws -> MatcherStatus) -> Matcher { + public static func simpleNilable(_ message: String = "match", matcher: @escaping @Sendable (Expression) throws -> MatcherStatus) -> Matcher { return Matcher { actual in return MatcherResult(status: try matcher(actual), message: .expectedActualValueTo(message)) } @@ -88,7 +90,7 @@ extension Matcher { } /// The Expectation style intended for comparison to a MatcherStatus. -public enum ExpectationStyle { +public enum ExpectationStyle: Sendable { case toMatch, toNotMatch } @@ -123,7 +125,7 @@ public struct MatcherResult { public typealias PredicateResult = MatcherResult /// MatcherStatus is a trinary that indicates if a Matcher matches a given value or not -public enum MatcherStatus { +public enum MatcherStatus: Sendable { /// Matches indicates if the matcher / matcher passes with the given value /// /// For example, `equals(1)` returns `.matches` for `expect(1).to(equal(1))`. @@ -181,7 +183,7 @@ public typealias PredicateStatus = MatcherStatus extension Matcher { // Someday, make this public? Needs documentation - internal func after(f: @escaping (Expression, MatcherResult) throws -> MatcherResult) -> Matcher { + internal func after(f: @escaping @Sendable (Expression, MatcherResult) throws -> MatcherResult) -> Matcher { // swiftlint:disable:previous identifier_name return Matcher { actual -> MatcherResult in let result = try self.satisfies(actual) @@ -207,7 +209,7 @@ extension Matcher { #if canImport(Darwin) import class Foundation.NSObject -public typealias MatcherBlock = (_ actualExpression: Expression) throws -> NMBMatcherResult +public typealias MatcherBlock = @Sendable (_ actualExpression: Expression) throws -> NMBMatcherResult /// Provides an easy upgrade path for custom Matchers to be renamed to Matchers @available(*, deprecated, renamed: "MatcherBlock") @@ -225,7 +227,7 @@ public class NMBMatcher: NSObject { self.init(matcher: predicate) } - func satisfies(_ expression: @escaping () throws -> NSObject?, location: SourceLocation) -> NMBMatcherResult { + func satisfies(_ expression: @escaping @Sendable () throws -> NSObject?, location: SourceLocation) -> NMBMatcherResult { let expr = Expression(expression: expression, location: location) do { return try self.matcher(expr) @@ -269,7 +271,7 @@ extension MatcherResult { } } -final public class NMBMatcherStatus: NSObject { +final public class NMBMatcherStatus: NSObject, Sendable { private let status: Int private init(status: Int) { self.status = status diff --git a/Sources/Nimble/Matchers/PostNotification.swift b/Sources/Nimble/Matchers/PostNotification.swift index 5144cc13f..6601b68a7 100644 --- a/Sources/Nimble/Matchers/PostNotification.swift +++ b/Sources/Nimble/Matchers/PostNotification.swift @@ -49,6 +49,22 @@ private let mainThread = pthread_self() private let mainThread = Thread.mainThread #endif +private final class OnlyOnceChecker: @unchecked Sendable { + var hasRun = false + let lock = NSRecursiveLock() + + func runOnlyOnce(_ closure: @Sendable () throws -> Void) rethrows { + lock.lock() + defer { + lock.unlock() + } + if !hasRun { + hasRun = true + try closure() + } + } +} + private func _postNotifications( _ matcher: Matcher<[Notification]>, from center: NotificationCenter, @@ -57,9 +73,16 @@ private func _postNotifications( _ = mainThread // Force lazy-loading of this value let collector = NotificationCollector(notificationCenter: center, names: names) collector.startObserving() - var once: Bool = false + let once = OnlyOnceChecker() return Matcher { actualExpression in + guard Thread.isMainThread else { + let message = ExpectationMessage + .expectedTo("post notifications - but was called off the main thread.") + .appended(details: "postNotifications and postDistributedNotifications attempted to run their predicate off the main thread. This is a bug in Nimble.") + return PredicateResult(status: .fail, message: message) + } + let collectorNotificationsExpression = Expression( memoizedExpression: { _ in return collector.observedNotifications @@ -69,8 +92,7 @@ private func _postNotifications( ) assert(Thread.isMainThread, "Only expecting closure to be evaluated on main thread.") - if !once { - once = true + try once.runOnlyOnce { _ = try actualExpression.evaluate() } diff --git a/Tests/NimbleTests/SynchronousTest.swift b/Tests/NimbleTests/SynchronousTest.swift index 98171f961..ef3dd2e0f 100644 --- a/Tests/NimbleTests/SynchronousTest.swift +++ b/Tests/NimbleTests/SynchronousTest.swift @@ -37,29 +37,33 @@ final class SynchronousTest: XCTestCase { } func testToProvidesActualValueExpression() { - var value: Int? - expect(1).to(Matcher.simple { expr in value = try expr.evaluate(); return .matches }) - expect(value).to(equal(1)) + let recorder = Recorder() + expect(1).to(Matcher.simple { expr in recorder.record(try expr.evaluate()); return .matches }) + expect(recorder.records).to(equal([1])) } func testToProvidesAMemoizedActualValueExpression() { - var callCount = 0 - expect { callCount += 1 }.to(Matcher.simple { expr in + let recorder = Recorder() + expect { + recorder.record(()) + }.to(Matcher.simple { expr in _ = try expr.evaluate() _ = try expr.evaluate() return .matches }) - expect(callCount).to(equal(1)) + expect(recorder.records).to(haveCount(1)) } func testToProvidesAMemoizedActualValueExpressionIsEvaluatedAtMatcherControl() { - var callCount = 0 - expect { callCount += 1 }.to(Matcher.simple { expr in - expect(callCount).to(equal(0)) + let recorder = Recorder() + expect { + recorder.record(()) + }.to(Matcher.simple { expr in + expect(recorder.records).to(beEmpty()) _ = try expr.evaluate() return .matches }) - expect(callCount).to(equal(1)) + expect(recorder.records).to(haveCount(1)) } func testToMatchAgainstLazyProperties() { @@ -76,29 +80,29 @@ final class SynchronousTest: XCTestCase { } func testToNotProvidesActualValueExpression() { - var value: Int? - expect(1).toNot(Matcher.simple { expr in value = try expr.evaluate(); return .doesNotMatch }) - expect(value).to(equal(1)) + let recorder = Recorder() + expect(1).toNot(Matcher.simple { expr in recorder.record(try expr.evaluate()); return .doesNotMatch }) + expect(recorder.records).to(equal([1])) } func testToNotProvidesAMemoizedActualValueExpression() { - var callCount = 0 - expect { callCount += 1 }.toNot(Matcher.simple { expr in + let recorder = Recorder() + expect { recorder.record(()) }.toNot(Matcher.simple { expr in _ = try expr.evaluate() _ = try expr.evaluate() return .doesNotMatch }) - expect(callCount).to(equal(1)) + expect(recorder.records).to(haveCount(1)) } func testToNotProvidesAMemoizedActualValueExpressionIsEvaluatedAtMatcherControl() { - var callCount = 0 - expect { callCount += 1 }.toNot(Matcher.simple { expr in - expect(callCount).to(equal(0)) + let recorder = Recorder() + expect { recorder.record(()) }.toNot(Matcher.simple { expr in + expect(recorder.records).to(beEmpty()) _ = try expr.evaluate() return .doesNotMatch }) - expect(callCount).to(equal(1)) + expect(recorder.records).to(haveCount(1)) } func testToNegativeMatches() { @@ -129,3 +133,24 @@ final class SynchronousTest: XCTestCase { } } } + +private final class Recorder: @unchecked Sendable { + private var _records: [T] = [] + private let lock = NSRecursiveLock() + + var records: [T] { + get { + lock.lock() + defer { + lock.unlock() + } + return _records + } + } + + func record(_ value: T) { + lock.lock() + self._records.append(value) + lock.unlock() + } +}