Skip to content

SequenceAllSatisfy is mapped to SequenceContainsWhere, and not SequenceAllSatisfy #1626

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.

Sign up for GitHub

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

Open
AZero13 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

SequenceAllSatisfy is mapped to SequenceContainsWhere, and not SequenceAllSatisfy #1626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/FoundationEssentials/Predicate/Archiving/PredicateCodableConfiguration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ extension PredicateCodableConfiguration {
configuration.allowPartialType(PredicateExpressions.Range<PredicateExpressions.Value<Int>, PredicateExpressions.Value<Int>>.self, identifier: "PredicateExpressions.Range")
configuration.allowPartialType(PredicateExpressions.SequenceContains<PredicateExpressions.Value<[Int]>, PredicateExpressions.Value<Int>>.self, identifier: "PredicateExpressions.SequenceContains")
configuration.allowPartialType(PredicateExpressions.SequenceContainsWhere<PredicateExpressions.Value<[Int]>, PredicateExpressions.Value<Bool>>.self, identifier: "PredicateExpressions.SequenceContainsWhere")
configuration.allowPartialType(PredicateExpressions.SequenceContainsWhere<PredicateExpressions.Value<[Int]>, PredicateExpressions.Value<Bool>>.self, identifier: "PredicateExpressions.SequenceAllSatisfy")
configuration.allowPartialType(PredicateExpressions.SequenceAllSatisfy<PredicateExpressions.Value<[Int]>, PredicateExpressions.Value<Bool>>.self, identifier: "PredicateExpressions.SequenceAllSatisfy")
Copy link
Contributor

@jmschonfeld jmschonfeld Dec 5, 2025

Choose a reason for hiding this comment

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

Hmm, we might have a compatibility issue here that we need to address. I believe as it stands today, this registers SequenceContainsWhere with both the PredicateExpressions.SequenceContainsWhere and PredicateExpressions.SwquenceAllSatisfy identifiers. PredicateCodableConfiguration will take the last identifier registered when the same type is registered with multiple identifiers, so today I think we've been encoding/decoding PredicateExpressions.SequenceContainsWhere with the PredicateExpressions.SequenceAllSatisfy identifier. Changing this would break any existing archives that already have a serialized SequenceContainsWhere stored under the SequenceAllSatisfy identifier. It would also break sending a serialized archive from a newer runtime to an application that deserializes it with an older runtime.

I'm not sure how many of those archives exist today in the wild. We can try to quantify that, but we might need to instead think about a compatible way to move forward even if these identifiers are incorrect so that we can solve the problem (allowing serialization of SequenceAllSatisfy) without breaking existing serializations of SequenceContainsWhere. Perhaps we leave a comment here about the unfortunate identifier mismatch and use a new identifier for the real SequenceAllSatisfy type?

Copy link
Contributor Author

@AZero13 AZero13 Dec 5, 2025

Choose a reason for hiding this comment

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

Are you okay with the new identifier route

Copy link
Contributor

@jmschonfeld jmschonfeld Dec 9, 2025

Choose a reason for hiding this comment

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

Yeah that's probably the best way to go. Let's update the SequenceContainsWhere line above to use the incorrect identifier (with a comment explaining why the identifier doesn't match) and then for SequenceAllSatisfy we can come up with a new identifier to use.

In terms of a new identifier since the appropriate one is "taken" in existing archives, maybe something like PredicateExpressions.SequenceAllSatisfy.corrected? @parkera @itingliu do you have any suggestions? I'm not sure if we've had a pattern before around naming and evolving archive keys that were incorrect historically that we should follow here.

configuration.allowPartialType(PredicateExpressions.SequenceStartsWith<PredicateExpressions.Value<[Int]>, PredicateExpressions.Value<[Int]>>.self, identifier: "PredicateExpressions.SequenceStartsWith")
configuration.allowPartialType(PredicateExpressions.SequenceMaximum<PredicateExpressions.Value<[Int]>>.self, identifier: "PredicateExpressions.SequenceMaximum")
configuration.allowPartialType(PredicateExpressions.SequenceMinimum<PredicateExpressions.Value<[Int]>>.self, identifier: "PredicateExpressions.SequenceMinimum")
Expand Down
13 changes: 13 additions & 0 deletions Tests/FoundationEssentialsTests/PredicateCodableTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,19 @@ private struct PredicateCodableTests {
}
}

@Test func sequenceAllSatisfyRoundTrip() throws {
let predicate = #Predicate<[Int]> {
$0.allSatisfy { $0 > 0 }
}

let decoded = try _encodeDecode(predicate, for: StandardConfig.self)

#expect(try predicate.evaluate([1, 2, 3]))
#expect(try decoded.evaluate([1, 2, 3]))
#expect(try !predicate.evaluate([-1, 2, 3]))
#expect(try !decoded.evaluate([-1, 2, 3]))
}

@Test func disallowedKeyPath() throws {
var predicate = #Predicate<Object> {
$0.f
Expand Down