SE-0231 — Optional iteration

(Edit to add: looks like you also need

func ??<S>(lhs: S?, rhs: S) -> S where S : Sequence, S: ExpressibleByArrayLiteral {
  switch lhs {
  case .some(let s):
    return s
  case .none:
    return rhs
  }
}

In order to make sure existing sequences that are expressible by [] aren't accidentally wrapped in an AnySequence.)

I did a quick check of the compatibility suite. It was a simple regex + eyeball so I think it avoids false positives but will probably be off +- a few.

I could see 20 examples of for with ?? [] and 55 examples of if let immediately followed by use in a for. Overall 24 projects used one of these patterns, so just over a quarter of the projects.

edit: adding in ?.forEach, of which there are 15 occurrences.

Anecdotally, this is a pretty high occurrence rate based on searches for similar patterns for other reviews I've done in the past.

13 Likes

Not sure if you looked at this: did the if let cases have any else condition?

I tried to spot and exclude those. Also there were quite a few cases to exclude where the let was used as an argument to the sequence-producing function rather than the iteree itself, which I also excluded, though strictly speaking, if we had an optional for syntax, that would allow map to be used in those cases.

1 Like

In our codebase (about 100k lines of Swift mixed in with about 1M lines of ObjC) there are 13 matches for @algal's for _ in _ ?? [] regex. 6 of them involve optional chaining, and all of those are related to an optional NSView/UIView (e.g. for subview in superview?.subviews ?? []).

Our in-house Swift style is generally to guard exclude optionals as soon as is practical and avoid dealing with them. I even notice that there are at least 5 places where it might have been natural to use optional chaining with sequences (outside of a for) and I see constructions like (getOptionalSequence() ?? []).map({...}).filter({...}) instead.

5 Likes

Using @algal 's strategy:

Our codebase has ~70,000 lines of swift code, we match 11 times for the regex looking for if let statements followed by for loops. 2 is the result of poor representation of data, resulting in conditional casting, the rest could have used ?.forEach or ?? [] (though using the ?.forEach on some of those would involve unfortunate parenthesis).

5 Likes

I have 56 instance of for x in sequenceExpression ?? [] in my code base, roughly 10% of all the for loops. Half of those come from XML parsing code (where you must always worry about parts of the structure being missing). Things like this:

for noteElement in unitElement.first("notes")?.all("note") ?? [] {
    // do something with the note
}

Currently the filters for XML elements of interest all return eagerly constructed arrays, so using sequenceExpression ?? [] works nicely. It'd be better to return lazy sequences, but then all the loops would need to be wrapped like this:

if let notesElement = unitElement.first("notes") {
    for noteElement in notesElement.all("note") {
        // do something with the note
    } 
}

Also, trying to mix ?? [] with lazy sequences could trigger the the trap unearthed by @Ben_Cohen, which could make the algorithm eager again without notice. That's a concern, although it's not anything specific to for loops.

Edit: code base is about 60k lines of Swift code.

4 Likes

Using @algal 's strategy:

LOC: 47,057

Matches: 3 [All of them use an else case].

Pods matches: 9 [5 of them use an else case].

Total: 12 matches.
8 of them using else case.

Note: I don't think the change it's worth it.

1 Like

Also, some overlays and source kit:

My memory is that I reach for this pattern frequently, but I don't currently have any code on-hand that does this.

3 Likes

In 255948 lines of just Swift code, excluding empty lines and comments (collected by cloc):

Occurrence of @algal's regex

if[:space:]*let[:space:]*\w+[:space:]*=[:space:]*.*[:space:]*\{[:space:]*for

5

Occurrence of for .+ in .+ ?? \[\]: 14

In a 500k+ line codebase I have access to I see 210 results for this regex. A lot of this code was written by people not familiar with Swift idioms so probably half of those fall into the stored optional array anti-pattern, however many of them are the result of optional chains and other reasonable code patterns.

Apologies if someone has mentioned this before, but why can't we just add .orEmpty() like Kotlin has?

extension Optional where Wrapped: Sequence {
    func orEmpty() -> AnySequence<Wrapped.Element> {
        switch self {
        case nil:
            return AnySequence(EmptyCollection())
        case .some(let s):
            return AnySequence(s)
        }
    }
}

let nilAsCanBe: [Int]? = nil

for i in nilAsCanBe.orEmpty() {
    // ...
}

Trivial to implement, easy to read, no language changes, no further overloading ?. (Bikeshed the name all you want.)

Edit: Fixed very sloppy non-compiling code.

1 Like

Yeah, this was one of the possibilites that came up, which I listed in my summary. I added a link to the summary into the original post to hopefully make this more discoverable.

Interesting. It makes me wonder why Sequence.reversed() returns an array in the first place. Why not return a Sequence?

The issue AIUI is that there are overloads that do both, and ?? [] contextually leaves only the array-producing form as a possibility.

2 Likes

The pattern .+ in .+ \?\? \[\] produces only 1 true positive:

for e in n.geometry?.elements ?? [] { … }

and many false positives such as:

for set in a1 { x4.formUnion(set ?? []) }

and

let objs: [TagItem] = selectedTagName.map { selTagName in document.catalog.tagItems.filter { $0.name == selTagName } } ?? []

in our 200k line codebase.

And the pattern if[[:space:]]let[[:space:]]*\w+[[:space:]]*=[[:space:]]*.*[[:space:]]*\{[[:space:]]*for produces zero matches. I had to double check by removing the last "for" which resulted in a lot of matches so for some reason our code base doesn't have a single occurrence of if let a = b { for.

1 Like

In just over 100K lines of Swift, excluding empty lines and comments (cloc); I found 5 instances using @algal's regex for:

  • if let ... for ...
  • for ... in ...?? []

I still don't see the driving need for this; re a few of these instances could fairly easily be avoided.

2 Likes

I only see one returning Array. I mean if Sequence.reversed() returns the same Sequence type(Self)—which feels very natural—the problematic code won't compile at all. There must be good reasons that the current API returns Array instead of Self(same for sorted()), what are they?

To reverse something that doesn't provide iteration from the last to first element (not just sequences, but forward-only collections as well), you must stream all the elements into some kind of storage first, then return them in reverse order. If you're going to do that, it may as well be an array because having an array is a useful thing to have. You could instead return some kind of opaque reversed thing, but that wouldn't help – you still have to allocate memory for all those elements.

3 Likes

Re your discussion with @Ling_Wang
That trap seems very specific to the combination of .? and ?? []. Neither of the following IMO would result in this:

  • Rust's unwrap_or
  • Java's orElse
  • Scala's getOrElse

...and I certainly can't easily recreate that with a similar extension method.