Skip to content

Relax generic constraint of array operator from Collection to Sequence #636

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

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

Conversation

omochi
Copy link

@omochi omochi commented May 4, 2025

Changes

The array operator ~~ is currently defined to take a Collection as its right-hand side operand, but a Sequence is sufficient, so this constraint is relaxed.

Motivation

With this change, it becomes convenient to use the operator with values like some Sequence, for example:

func users(ids: some Sequence<Int>) async throws -> [User] {
  try await User.query(on: db)
    .filter(\.id ~~ ids)
    .all()
}

In server-side data manipulation logic, it’s common to use Sequences that are not Collections, such as .uniqued() from swift-algorithms, so being able to use some Sequence is convenient.

Compatibility

Since Collection is a sub-protocol of Sequence, this change to the generic signature is a pure relaxation and remains fully source-compatible.

@omochi omochi requested a review from gwynne as a code owner May 4, 2025 05:46
@gwynne
Copy link
Member

gwynne commented May 4, 2025

I seem to remember that there was a reason I didn't do this last time I was tinkering with the value operators, but unfortunately I don't remember what that reason was. At the very least, there is definitely a conceptual issue in that Sequences are allowed to be unbounded and thus contain effectively infinite elements, which obviously does not make sense for a database query (and can even theoretically open a window for potential DoS attacks). By requiring the more restrictive Collection, which must have a finite bound, we at the very least prevent Fluent users from accidentally creating infinite loops in the query machinery.

I don't think this was the only reason I didn't do it, though - there's definitely very obvious utility in allowing the use of algorithms such as .uniqued(). Another issue I might have run into is that this change can be source-breaking, if a caller were referencing a method which had overloads that returned both a Sequence and a Collection; the constraint to Collection would thus resolve the ambiguity by inference. Granted, this is a very unlikely scenario, as disambiguating overloads solely by return type is a very rare pattern, but in the strictest sense, it is a source compatibility break.

I need to investigate this a bit further before making a final decision, but I should say that I'm leaning towards avoiding it, if only out of an abundance of caution.

@omochi
Copy link
Author

omochi commented May 4, 2025

Regarding the risk of infinite length with Sequence,
even a finite Collection can represent a very large number of elements.
For example, if 2^63 elements were provided, the system would take practically forever to iterate over them,
and it would likely run out of memory before that.
So in terms of resistance to attacks, I believe there isn’t much practical difference.

It’s true that, strictly speaking, this change is not source-compatible.
While the cases where this would cause issues are rare, if such a break is still considered unacceptable,
I would be happy to see this change introduced in a future major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants