ladvoc / BijectiveDictionary

A specialized dictionary structure offering bijective mapping and bidirectional O(1) access.
https://jacobgelman.com/posts/swift-bijective-dictionary
MIT License
2 stars 1 forks source link

Fix inconsistent indexing between `LeftValues` and `RightValues` #8

Closed ladvoc closed 1 week ago

ladvoc commented 1 week ago

I noticed something I overlooked when I first implemented LeftValues and RightValues. In a standard Dictionary, Keys and Values share the same indices. That is, as long as the dictionary is not mutated and the indices remain stable, index $i$ corresponds to the same element regardless if accessed from keys or values:

var dict = [
    "a": 1,
    "b": 2,
    "c": 3
]

print(dict.keys[dict.startIndex])  // prints "b"
print(dict.values[dict.startIndex])  // prints "2"

dict["d"] = 4  // mutation invalidates indices

print(dict.keys[dict.startIndex]) // prints "a"
print(dict.values[dict.startIndex]) // prints "1"

This is the expected behavior for index-based access in a Dictionary. However, BijectiveDictionary's current implementation has a bug. The reason the docs mention that order is not guaranteed for leftValues and rightValues is because LeftValues uses _ltr and RightValues uses _rtl. However, since _rtl is a separate dictionary, it has different indices. This means index $i$ refers to a different element in leftValues and rightValues. This behavior is unexpected and is not consistent with Dictionary.

Taking another look at this, there is no reason RightValues can’t also use _ltr; this pull request modifies RightValues accordingly.

DandyLyons commented 1 week ago

In a standard Dictionary, Keys and Values share the same indices. That is, as long as the dictionary is not mutated and the indices remain stable, index i corresponds to the same element regardless if accessed from keys or values

I'm not certain that this statement is true. See this from the docs:

The order of key-value pairs in a dictionary is stable between mutations but is otherwise unpredictable.

A dictionary’s indices stay valid across additions to the dictionary as long as the dictionary has enough capacity to store the added values without allocating more buffer. When a dictionary outgrows its buffer, existing indices may be invalidated without any notification. https://developer.apple.com/documentation/swift/dictionary#Iterating-Over-the-Contents-of-a-Dictionary

In any case, perhaps it does make sense to use _ltr for RightValues. If so, that would mean that we don't have to worry about RightValues and LeftValues being out of sync or having mismatched order pairs.

However, I'm not sure if that could have any negative performance implications for certain use cases. But all current tests are passing with this PR.

This is probably good to go, but I think we should add more test coverage.

DandyLyons commented 1 week ago

I added some test coverage. For this. It looks like I don't have permission to push my test coverage changes directly to this PR. I can PR my added tests after we merge this PR. This PR looks good to go.

👍🏼

ladvoc commented 1 week ago

You are right, my explanation implies that the indices get invalidated on each mutation which doesn’t always happen. But in any case, while the indices are valid, a given index should refer to the same left-right pair.

I will go ahead and merge this now. I also agree more testing is needed—especially performance testing; I will revise issue #4 to track this.