Collection conformance for LeftValues and RightValues

[DandyLyons]

This PR adds Collection conformance to LeftValues and RightValues.

Design Considerations

The intention is to map the values:

• leftValues -> LeftValues._ltr.keys
• rightValues -> RightValues._rtl.keys

Hopefully this should allow the user to just treat a BijectiveDictionary as if it were a vanilla Dictionary, except they would use .leftValues and .rightValues instead of .keys and .values.

Order Preservation

I noticed that the docs now mention that LeftValues does not guarantee the order and that it might guarantee it in a future release. I think it makes sense not to guarantee the order, since the vanilla Dictionary.Keys does not guarantee the order.

[ladvoc]

Your code looks good. I just see one issue; accessing leftValues or rightValues using a BijectiveDictionary<Left, Right>.Index doesn't compile:

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

print(dict.leftValues[dict.startIndex])
print(dict.rightValues[dict.startIndex])
// error: Subscript 'subscript(_:)' requires that 
// 'BijectiveDictionary<String, Int>.Index' conform to 'RangeExpression'

However, this is possible with Dictionary:

var dict = [
	"a": 1,
        "b": 2,
        "c": 3
]
print(dict.keys[dict.startIndex])
print(dict.values[dict.startIndex])

I need to do some investigation to determine the source of this issue. Please let me know if you have any ideas. I would recommend taking a look at TreeDictionary+Keys.swift from the swift-collections package; I found this to be a helpful resource when I first implemented LeftValues and RightValues.

[DandyLyons]

Thanks for catching that. We should add it to the tests.

Is it worth looking into if we should make our Index conform to RangeExpression?

[ladvoc]

@DandyLyons, please review PR #8. I realized there is a bug in the existing implementation that prevents Collection from being properly implemented. Once this is fixed, you will be able to complete this PR.

[ladvoc]

The Collection implementation for LeftValues and RightValues should be indexed by BijectiveDictionary<Left, Right>.Index, not Dictionary<Left, Right>.Index. Even though BijectiveDictionary is currently backed by standard library dictionaries, this could change in the future, so it is best not to expose any subtypes of Dictionary in the public API.

[DandyLyons]

Sounds good. I'll push a revision.

[DandyLyons]

Could I back it by a LeftValues.Index instead of a BijectiveDictionary<Left,Right>.Index? Otherwise, I have an infinite recursion of types that I need to figure out how to resolve.

I was thinking of using a similar pattern to what you use here:

BijectiveDictionary/Sources/BijectiveDictionary/BijectiveDictionary+Collection.swift

Line 12 in 233a2f6

@frozen public struct Index: Comparable {

[ladvoc]

LeftValues.Index should be a type alias of BijectiveDictionary<Left,Right>.Index. See if this works:

extension BijectiveDictionary.LeftValues: Collection {
    
    public typealias Index = BijectiveDictionary<Left, Right>.Index
    
    @inlinable public var startIndex: Index { Index(_ltr.startIndex) }
    @inlinable public var endIndex: Index { Index(_ltr.endIndex) }
    
    @inlinable
    public func index(after i: Index) -> Index {
        Index(_ltr.index(after: i._ltrIndex))
    }
    
    @inlinable
    public subscript(position: Index) -> Left {
        _ltr[position._ltrIndex].key
    }
}

[DandyLyons]

BijectiveDictionary<Left,Right>.Index does not itself conform to Collection. It's possible that we could make it conform to Collection but that has some unforeseen consequences that I'm still discovering. For one, it creates ambiguity that I need to resolve.

There are multiple routes that I'm exploring to search for a solution. But I have a few questions:

[DandyLyons]

Should BijectiveDictionary.Index be defined as a concrete struct like this:

BijectiveDictionary/Sources/BijectiveDictionary/BijectiveDictionary+Collection.swift

Line 12 in 9b84532

@frozen public struct Index: Comparable {

Or should it be an internal type with a public interface through a typealias. If we use the second approach, then this could help me to disambiguate, but it is a pretty large refactor that I don't want to start without your design approval.

[DandyLyons]

But then yet another approach would be that we should not reuse BijectiveDictionary<Left, Right>.Index to also serve as the Index for BijectiveDictionary.LeftValues (as suggested here). If we did so, then it would be serving double duty, and I suspect that those duties could conflict with each other in the future. We might discover requirements for one that would be incompatible with requirements for the other.

So another approach would be to design a concrete BijectiveDictionary.LeftValues.Index type. This type could largely follow the pattern of BijectiveDictionary.Index but would be free to make separate changes as duties require.

[ladvoc]

In this case, I think it makes sense for BijectiveDictionary<Left, Right>.Index to also index LeftValues and RightValues since these are views into the dictionary collection and share the same indices. Using the same type also captures this relationship through the type system. Dictionary from the standard library and specialized dictionary types like TreeDictionary from Swift Collections also use this approach for their Keys and Values collections.

Please let me know if this answers your question.

[DandyLyons]

That makes a lot of sense and I didn't consider that it would preserve the type relationship.

I'll look more at the Dictionary and TreeDictionary implementation and hopefully that should help me better understand how to implement this.

[DandyLyons]

I apologize. This was completely accidental. I synced my fork so I could get the PR #8 that you merged and somehow this closed my pull request and force pushed and closed the PR.

[DandyLyons]

The Collection conformance now has the change that you requested.

P.S. Sorry this took so long.
This ended up being way simpler than I thought it was, and your code was almost the entire fix. I mistakenly thought that the type of _ltr also needed to change, which then created a bunch of other errors.

[ladvoc]

No problem, there's no deadlines here. Looks good to merge.