Nathan Manceaux-Panot

Whoa: just found a crashing bug in Retcon, that I wrote *four years* ago. It’s easy to trigger, too: just ⌘↓ a commit that’s right above the very first commit in the history.

(it’s an array indexing bug: the commit has no parent now, so the lookup fails. these days I use `safeIndex:` everywhere.)

May 11 at 9:37amWeb
Show threadNathan Manceaux-PanotMay 11
If you’re wondering what four years looks like in Retcon’s repo, just look at that scrollbar thumb! The app was still named Bluebird, and deleting commits had been added the day just before. Simpler times.
Show threadNathan Manceaux-PanotMay 11
The code understands nil parents, so the fix is trivial. Four years in the making!
Show threadRob NapierMay 11

@Cykelero this fix is making it even harder to find your real bug (which still exists) and may lead to data loss due to operating on the wrong item. You likely should track them by ID rather than index.

Edit: I originally read “commit” as a verb here, but whatever you do next has the same problem of having the wrong value.

Show threadPaulMay 12

@Cykelero seeing this finally made me make my copy and paste 'safe' extension into a package 😅

Mostly so whenever I join a new company and see they use '.contains' for theirs, I can point them to these benchmarks https://github.com/brzzdev/swift-safe-subscript#benchmarks

GitHub - brzzdev/swift-safe-subscript: Safe, O(1) [safe:] subscript access for any Swift collection.

Safe, O(1) [safe:] subscript access for any Swift collection. - brzzdev/swift-safe-subscript

GitHub
Show threadNathan Manceaux-PanotMay 13
@brzz Neat! I do wonder what the performance difference is within an actual codebase. It’s probably only visible is more extreme cases, with very frequent subscripting, but there’s no reason to skip on that faster way to check, anyway.
Show threadChristian TietzeMay 11
@Cykelero weird how these kinds of errors never go away :)
Show threadNathan Manceaux-PanotMay 11

@ctietze For a few years I’ve been extremely defensive in my code (nothing ever gets force-unwrapped, or indexed, etc), so hopefully I’m not writing a lot of new crashing bugs!

It’s unwieldy code sometimes, but the app crashing in the middle of a complex rebase is just unacceptable; too much mental context immediately invalidated.

Show threadRob NapierMay 11

@Cykelero safeIndex patterns point to your problem. If you’re not sure that the index actually comes from the collection, then you have to become sure, not add a partial fallback.

If your collection can change independently of your stored index, then the index is invalid. “Safe”Index only does anything if it happens to also be out of range. It could just as easily be valid but point to the wrong item.

It is generally not safe to store indexes of mutable collections.

Show threadNathan Manceaux-PanotMay 11
@cocoaphony I see what you mean. Things here are all good: the `selectedViewCommitIndices` value we’re reading is a computed property; it reads `tableView.selectedRowIndexes`, and converts them to `cachedViewCommits` indices, by performing lookups in this same collection. So it’s by definition always up to date!
Show threadNathan Manceaux-PanotMay 11

@cocoaphony Here, the expected value really is nil. Having a nil parent means that a commit is root; when moving a commit down, if it becomes the first commit in the history (i.e. the root), then it should be assigned a nil parent.

This code has been around for a while, and I’m really not one to write things I don’t (think that I) understand. I very much appreciate the vigilance though!

And, maybe that was also your point, an explicit `idx == endIndex` check would lock things down even better

Show threadRob NapierMay 11
@Cykelero I don't think you want to add yet another index check. You want to treat indexes as opaque. Even that +2 is a bit risky. You mentioned that you are "converting" table view indexes into cachedViewCommits indexes, and that step is probably wrong due to a race condition. If you have a a cache, it's probably wiser to manage it as a dictionary and look things up by an identifier rather than trying to maintain parallel arrays. Parallel arrays are really hard to keep right in concurrent code.
Show threadRob NapierMay 11
@Cykelero if it were definitionally correct, it wouldn’t crash and you would reach for safeIndex. The reason it’s failing is very likely that the array your indexing into has changed (probably been set to empty) and this code was called before the table has refreshed. They’re not in sync. (This is a very common mistake that was also common in ObjC.) Guarding that the index “seems valid” (is in range) just indicates that you’re not certain it’s in sync. Better to fix the race.
Show threadNathan Manceaux-PanotMay 11

@cocoaphony No, that’s not it. Here we *want* to get nil as a result.

We *intentionally* are trying to read `lastExistingCommitIndex + 1`; that’s the correct location, and for that location, the correct output is nil.

A more explicit way to write this would be:

let newParentIndex = [...] + 2
let newParent =
newParentIndex == cachedViewCommits.endIndex
? nil
: cachedViewCommits[newParentIndex]

Show threadNathan Manceaux-PanotMay 11

@cocoaphony There’s no race condition; almost all of Retcon is fully single-threaded! (for reasons)

All the data manipulated in this bit of code is computed *from* cachedViewCommits; nothing can ever be stale here, because everything is derived (recomputed) on every access. I am very wary of caching issues.

Other portions of the app (most anything that’s not UI) indeed rely on retrieving things by ID. Here’s we’re manipulating a list, literally, which does imply indexing into that list

Show threadRob NapierMay 11
@Cykelero Being single-threaded doesn't prevent race conditions. You can still have something modify your collection between the time you store an index and the time you read the index. That's been a cause of bugs using table views for years (even before GCD).
Show threadRob NapierMay 11
@Cykelero I see what you're doing there. I don't love the "+ 2" (it screams of fragile code) but yeah, checking explicitly that "this is the last element because the next index is endIndex" is correct.
Show threadNathan Manceaux-PanotMay 11
@cocoaphony Agreed with the +2, it’s already a magic number at this point. It’s tolerated here because it’s UI code, at the very leaf end of the call tree.
Show threadJeffreyMay 11
@Cykelero for a programming language where optionals play such an important role, the fact that array index accessors are not optional by default has always felt like an oversight to me…
Show threadNathan Manceaux-PanotMay 12
@jeffreykuiken Yeah, same! But I think it’s intentional, there’s a forum post somewhere I think, explaining their rationale
Show threadJeffreyMay 12
@Cykelero interesting, I’ll see if I can find it sometime!
Show threadNathan Manceaux-PanotMay 12
@jeffreykuiken I can't find the post right now, but there is this short mention in the docs: https://github.com/swiftlang/swift-evolution/blob/main/commonly_proposed.md#strings-characters-and-collection-types
swift-evolution/commonly_proposed.md at main · swiftlang/swift-evolution

This maintains proposals for changes and user-visible enhancements to the Swift Programming Language. - swiftlang/swift-evolution

GitHub
Show threadJeffreyMay 12
@Cykelero thanks, that’s an interesting read!