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.

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

[stdlib] Set, Dictionary: Fix (cocoa, native) equatable implementation #33093

Closed
wants to merge 1 commit into from

Conversation

AndrewSB
Copy link
Contributor

I was looking through the implementation of set and this change felt off to me.

@lorentey it looks like you changed this last, what do you think?

@theblixguy theblixguy requested a review from lorentey July 24, 2020 15:25
@@ -415,7 +415,7 @@ extension Set: Equatable {
case (true, false):
return lhs._variant.asNative.isEqual(to: rhs._variant.asCocoa)
case (false, true):
return rhs._variant.asNative.isEqual(to: lhs._variant.asCocoa)
Copy link
Member

Choose a reason for hiding this comment

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

I was knee deep in Set guts trying to figure out how this managed to remain undetected for so long, but it turns out the original code is correct! Unlike the rest of the cases, here isEqual(to:) is called on rhs, not lhs. (_CocoaSet.isEqual(to:) does not support a native argument, so this line wouldn't compile with the suggested change.)

This is way too subtle. I blame the terrible naming convention lhs and rhs -- naming these parameters left and right would've highlighted the swap much better. Would you like to rename them?

Copy link
Contributor Author

@AndrewSB AndrewSB Jul 25, 2020

Choose a reason for hiding this comment

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

oh! I see, in this case the lhs and rhswere reversed! The naming does make it hard to notice that. Let me reorder them instead, sorry for the false alarm

@AndrewSB AndrewSB changed the title [stdlib] Set: Fix (cocoa, native) equatable implementation [stdlib] Set, Dictionary: Fix (cocoa, native) equatable implementation Jul 27, 2020
@AndrewSB AndrewSB requested a review from lorentey July 27, 2020 19:31
@AndrewSB
Copy link
Contributor Author

AndrewSB commented Aug 3, 2020

@lorentey I think this is ready for another pass & potentially to be merged

@lorentey
Copy link
Member

lorentey commented Aug 4, 2020

@AndrewSB The current version won't work, either, for the same reason I wrote above:

_CocoaSet.isEqual(to:) does not support a native argument, so this line wouldn't compile with the suggested change.

Stepping back, what concrete problem are you addressing with this PR? Have you verified that your change builds & runs locally before submitting this for review?

@lorentey
Copy link
Member

lorentey commented Aug 4, 2020

@swift-ci smoke test

@lorentey
Copy link
Member

lorentey commented Aug 4, 2020

13:34:51 
/Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/stdlib/public/core/Dictionary.swift:1585:60: error: cannot convert value of type '_NativeDictionary<Key, Value>' to expected argument type '__CocoaDictionary'
13:34:51       return lhs._variant.asCocoa.isEqual(to: rhs._variant.asNative)
13:34:51                                                            ^
13:34:51 /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/stdlib/public/core/Set.swift:418:60: error: cannot convert value of type '_NativeSet<Element>' to expected argument type '__CocoaSet'
13:34:51       return lhs._variant.asCocoa.isEqual(to: rhs._variant.asNative)
13:34:51                                                            ^

@gribozavr
Copy link
Collaborator

Assuming there is a bug in this code that this PR fixes, please add tests for it -- thanks!

@lorentey
Copy link
Member

@AndrewSB I'm closing this as it isn't building in this stage, and I don't see what problem these changes are addressing. Feel free to submit a new PR if there is an issue to be resolved.

@lorentey lorentey closed this Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants