Use OrderedSet for resolving products

[giginet]

Fix CI for Xcode 16.

The test on the current main is broken with Xcode 16.
https://github.com/giginet/Scipio/actions/runs/9854164954/job/27206328940

2024-07-09T09:11:47.0455430Z /Users/runner/work/Scipio/Scipio/Tests/ScipioKitTests/DescriptionPackageTests.swift:76: error: -[ScipioKitTests.DescriptionPackageTests testBuildProductsInCreateMode] : XCTAssertEqual failed: ("["Logging", "MyTarget", "Logging", "MyTarget", "ExecutableTarget", "MyPlugin"]") is not equal to ("["Logging", "MyTarget", "ExecutableTarget", "MyPlugin"]") - Order of the resolved products should be correct

According to the build log, resolveBuildProducts returns duplicated BuildProduct.

It's better to use OrderedSet here to remove duplications and keep the order.

Strangely, this has not been produced with Swift 5.10.

[giginet]

@dmhts You wished to avoid using OrderedSet, but I think it's better to use it here.
#130

What do you think?

[dmhts]

My concern about using OrderedSet was caused but not understanding at the time the real reason for the crash explained in #130, which was the violation of the Hashable requirements in the first place.
The reason for that should be fixed by now with this PR when we've implemented correct custom conformance for both Hashable (in my PR) and Equatable (in your PR). I'm not sure if OrderedSet is even needed here.

Of course, it wouldn't hurt, but wouldn't resolveBuildProducts always produce an array of unique build products anyway? I think topologicalSort always outputs an array of [Hashable] without duplicates.
Or do you use OrderedSet here just for some extra peace of mind here?

[giginet]

Oh, As you said, I confirmed it's enough to add the == override to fix this error without modifying it here.

OrderedSet may have issues. It causes runtime crash 🤔
However, I prefer to use this to clarify it doesn't have duplicated products if we can use it.

[dmhts]

OrderedSet may have issues. It causes runtime crash

That's right, but it's important to note that it might happen only in the debug builds as it's an assertion (to catch early certain performance-related issues). So I wouldn't be worried about the crash in the first place.

However, I prefer to use this to clarify it doesn't have duplicated products if we can use it.

I still believe using OrderedSet can be theoretically harmful, by preventing the identification of some subtle potential issues in the resolveBuildProducts implementation in the future. Like it happened just right now. If you had used OrderedSet you would've never spotted the issue.

[dmhts]

As an alternative you can still convert the output to OrderedSet but at least add before that an assert that would check that the output from the topological sort contains no duplicates.

That would guard against both unforeseen changes on SwiftPM's side and forwarding unintended output to Scipio's clients.

[giginet]

Thank you. If we use Set and runtime assertion here, it's difficult to write tests for this, because the order will be missing on the test case.

I'll try to use OrderSet, but I understand your concern about using it, so I'll keep paying attention.

[dmhts]

If we use Set and runtime assertion here, it's difficult to write tests for this

Nope, I meant OrderedSet + the runtime assertion. An assertion is also a form of a unit test if you will. Something like assert(Set(products).count == products.count) could catch any issues with duplicates in the function implementation for debug builds and would run along with the main unit tests.

But you decide, of course.

[giginet]

@dmhts You introduced the new hash method on #132.
However, I found there are some cases where models are not equal even if the hashable value is the same.
It causes Set can't work correctly even if hashValue s are identical.

So we need this override.

[dmhts]

Ah, thank you, I should've implemented it in my PR! Absolutely, I bet the default synthesized conformance to Equatable also uses the newly introduced build triple (BuildTriple), which might be different for the same target depending on its parent (as I described in the comment a bit below). It precisely explains why two objects with the same hash still might not be considered equal.

[dmhts]

Strangely, this has not been produced with Swift 5.10.

@giginet Agree, I believe there might've been some changes introduced in Swift 6.0 regarding the default Equatable conformance automatically synthesized by the compiler. At least, this hypothesis would explain the observed behavior.

[dmhts]

I'll approve it, even though my approval doesn't count. :)