Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use OrderedSet for resolving products #135

Merged
merged 1 commit into from
Jul 11, 2024
Merged

Use OrderedSet for resolving products #135

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions Sources/ScipioKit/DescriptionPackage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ struct DescriptionPackage {
}

extension DescriptionPackage {
func resolveBuildProducts() throws -> [BuildProduct] {
func resolveBuildProducts() throws -> OrderedSet<BuildProduct> {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

What do you think?

Copy link
Contributor

@dmhts dmhts Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dmhts dmhts Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dmhts dmhts Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dmhts dmhts Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let targetsToBuild = try targetsToBuild()
var products = try targetsToBuild.flatMap(resolveBuildProduct(from:))

Expand Down Expand Up @@ -186,7 +186,7 @@ extension DescriptionPackage {
}
}

return products.reversed()
return OrderedSet(products.reversed())
}

private func targetsToBuild() throws -> [ScipioResolvedModule] {
Expand Down Expand Up @@ -267,6 +267,11 @@ struct BuildProduct: Hashable, Sendable {
target.underlying as? ScipioBinaryModule
}

static func == (lhs: Self, rhs: Self) -> Bool {
lhs.target.name == rhs.target.name &&
lhs.package.identity == rhs.package.identity
}
Comment on lines +270 to +273
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


func hash(into hasher: inout Hasher) {
// Important: Relevant for swift-6.0+ toolchain versions. For the versions below
// this change has no effect as SwiftPM provides its own proper `Hashable`
Expand Down
Loading