Enable non-lexigraphic equivalence checking for Compositions #6
Conversation
dfed
force-pushed
the
dfed--unordered-composition-equivalence
branch
from
4cdd5e0 to
7d1d71e
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6 +/- ##
==========================================
+ Coverage 97.73% 97.74% +0.01%
==========================================
Files 37 37
Lines 7864 7909 +45
==========================================
+ Hits 7686 7731 +45
Misses 178 178
|
| @@ -249,6 +257,8 @@ public enum TypeDescription: Codable, Hashable, Comparable, Sendable { | |||
| case typeDescription | |||
| /// The value for this key is the associated value of type [TypeDescription] | |||
| case typeDescriptions | |||
| /// The value for this key is the associated value of type Set<TypeDescription> | |||
| case unorderedTypeDescriptions | |||
There was a problem hiding this comment.
I'm not convinced this is a necessary addition. While the extra specificity is nice... the underlying json representations of a Set and Array are identical (aside from ordering).
So my options are:
- Introduce this new key
- Update the documentation on
case typeDescriptionsto state that the value here is either an array or set.
I'm currently leaning towards the second option.
There was a problem hiding this comment.
Yeah alright talked myself into it and pushed up 8afe45e. Open to feedback, and could be convinced to revert.
| @@ -27,7 +27,7 @@ public enum TypeDescription: Codable, Hashable, Comparable, Sendable { | |||
| /// A nested type with possible generics. e.g. Array.Element or Swift.Array<Element> | |||
| indirect case nested(name: String, parentType: TypeDescription, generics: [TypeDescription]) | |||
| /// A composed type. e.g. Identifiable & Equatable | |||
| indirect case composition([TypeDescription]) | |||
| indirect case composition(Set<TypeDescription>) | |||
There was a problem hiding this comment.
A downside of using a Set here is that we no longer know the written order of the types of the composition in the source code. We could offset that by utilizing an array still, and then doing set comparisons in a manually written == implementation, but:
- Doing just-in-time
Setcomparisons can get expensive when doing a lot of comparisons - Maintaining a manually written
==implementation sounds... not fun
So I'm pretty bullish that this approach is good enough for our needs. Open to feedback and alternative thoughts of course.
There was a problem hiding this comment.
There's always OrderedSet from swift-collections, but then you'd be adding a dependency.
There was a problem hiding this comment.
yup! OrderedSet would solve the just-in-time Set comparison problem. But I'd still need the manually written == which is a bit painful. Might be worth it though?
There was a problem hiding this comment.
I have no qualms with a dependency, especially one that already supports Linux 😄 https://swiftpackageindex.com/apple/swift-collections
There was a problem hiding this comment.
I'm gonna write a new type that houses both an array and a set, with its own == implementation, and use that.
| return types | ||
| .map { $0.asSource } | ||
| // Sort the result to ensure stable code generation. | ||
| .sorted() |
There was a problem hiding this comment.
we'll get rid of this line when we introduce the new type
dfed
force-pushed
the
dfed--unordered-composition-equivalence
branch
from
c9a9fa1 to
4280857
Compare
| private let set: Set<Element> | ||
|
|
||
| public static func == (lhs: UnorderedComparingArray, rhs: UnorderedComparingArray) -> Bool { | ||
| lhs.set == rhs.set |
There was a problem hiding this comment.
Duh, easier than I thought
There was a problem hiding this comment.
Hardest part was coming up with a name for the type. I'm not convinced I did a great job there.
| @@ -425,6 +429,38 @@ extension ExprSyntax { | |||
| } | |||
| } | |||
|
|
|||
| // MARK: - UnorderedComparingArray | |||
|
|
|||
| public struct UnorderedComparingArray<Element: Codable & Hashable & Sendable>: Codable, Hashable, Sendable, Collection { | |||
There was a problem hiding this comment.
What do you think about changing the name? Having a subscript for an unordered array seems a bit confusing.
There was a problem hiding this comment.
All sequences have subscripts, and sequences are not necessarily ordered logically (though their order is stable). It's a quirk of Foundation.
There was a problem hiding this comment.
But I am super open to other names
There was a problem hiding this comment.
Going with UnorderedComparingCollection
Today, SafeDI considers
Foo & BarandBar & Footo be discrete types. However, the compiler does not consider these types to be discrete, and therefore neither should we.This change enables treating the above compositions as equivalent by utilizing an unordered
Setto store the associated types of aTypeDescription.composition.