Skip to content

Make ObjC isEqual: delegate to Equatable when Hashable isn't available #68720

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

Merged
merged 4 commits into from
Sep 29, 2023

Conversation

tbkka
Copy link
Contributor

@tbkka tbkka commented Sep 23, 2023

When a Swift struct gets bridged to Obj-C, we box it into an opaque _SwiftValue Obj-C object. This object previously supported the Obj-C isEqual: and hash selectors by dispatching to the Swift Hashable conformance, if present.

This does not work if the Swift struct conforms to Equatable but does not conform to Hashable. This case seems to have been overlooked in PR #4124.

This PR extends the earlier work to support isEqual: by first checking for a Hashable conformance, then falling back on an Equatable conformance if there is no Hashable conformance.

Resolves rdar://114294889

When a Swift struct gets bridged to Obj-C, we box it into an opaque
`_SwiftValue` Obj-C object.  This object previously supported the
Obj-C `isEqual:` and `hash` selectors by dispatching to the Swift
Hashable conformance, if present.

This does not work if the Swift struct conforms to Equatable but
does not conform to Hashable.  This case seems to have been
overlooked in PR swiftlang#4124.

This PR extends the earlier work to support `isEqual:` by
first checking for a Hashable conformance, then falling back
on an Equatable conformance if there is no Hashable conformance.

Resolves rdar://114294889
@tbkka tbkka requested a review from mikeash September 23, 2023 01:04
@tbkka
Copy link
Contributor Author

tbkka commented Sep 23, 2023

@mikeash Any ideas how we might extend the caching here to support the Equatable case? There are two pointers in the _SwiftValue header for the "base Hashable type" and the Hashable conformance. I'd like to avoid adding two more pointers for Equatable, but I'm a little unsure if it's a good idea to try reusing the existing pointers.

One possibility: Use a couple of bits of those pointers as booleans to flag whether the rest of the pointer is for Hashable or Equatable. I'm not sure how best to do that while dodging all the other folks (pac, etc) that are trying to exploit those bits.

@mikeash
Copy link
Contributor

mikeash commented Sep 25, 2023

Very nice! Those pointers don't have PAC (although that raises the question of whether they should... they serve a similar purpose to an isa/metadata pointer), but there aren't always high bits available anyway, for example on 32-bit.

These pointers are aligned to the pointer size, so the low 2-3 bits (depending on pointer size) are available, and nothing else is trying to use them (yet). You can use the low bit as a flag to indicate whether you have Hashable or Equatable. We have a local copy of LLVM's PointerUnion at stdlib/include/llvm/ADT/PointerUnion.h which takes care of the low-level details, if you want to use that.

@tbkka
Copy link
Contributor Author

tbkka commented Sep 28, 2023

@swift-ci Please test

@tbkka
Copy link
Contributor Author

tbkka commented Sep 28, 2023

These pointers are aligned to the pointer size, so the low 2-3 bits (depending on pointer size) are available, and nothing else is trying to use them (yet).

That's what I needed to know. Thanks!

PointerUnion's magic failed me here, so I ended up flogging the low-order bit manually. Seems okay so far.

Copy link
Contributor

@mikeash mikeash left a comment

Choose a reason for hiding this comment

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

Looks great, just some small comments.

This simplified a lot of things.  (Thanks @mikeash for the
suggestion!)

Also reorganized the code a bit.
@tbkka
Copy link
Contributor Author

tbkka commented Sep 28, 2023

Fixed the mysterious build problem (with help from @etcwilde ) and improved the cache handling (following a suggestion from @mikeash ). Let's see if this flies now...

@tbkka
Copy link
Contributor Author

tbkka commented Sep 28, 2023

@swift-ci Please test

2 similar comments
@tbkka
Copy link
Contributor Author

tbkka commented Sep 28, 2023

@swift-ci Please test

@rintaro
Copy link
Member

rintaro commented Sep 28, 2023

@swift-ci Please test

@bnbarham
Copy link
Contributor

@swift-ci please test Linux platform

@tbkka
Copy link
Contributor Author

tbkka commented Sep 29, 2023

Sourcekit-lsp failure on Linux seems unrelated?

@tbkka
Copy link
Contributor Author

tbkka commented Sep 29, 2023

@swift-ci Please test Linux platform

@bnbarham
Copy link
Contributor

It's unrelated, Alex merged the fix for it last night. Sorry about that!

@tbkka tbkka merged commit b35987f into swiftlang:main Sep 29, 2023
tbkka added a commit to tbkka/swift that referenced this pull request Oct 27, 2023
If a Swift type implements Equatable and/or Hashable and
we then pass that object into ObjC, we want ObjC
`isEqual:` and `hashValue` to use that.  This allows
ObjC code to build ObjC collections of Swift objects.

* Support for Hashable struct/enum types was implemented in swiftlang#4124
* Support for Equatable struct/enum types was implemented in swiftlang#68720
* This implements support for Hashable and Equatable _class_ types

Caveats:
1. This does a lot of dynamic lookup work for each operation, so is
   inherently rather slow.  Unlike the struct/enum case, there is no convenient
   place to cache the conformance information, so it's not clear that there is a
   viable way to make it significantly faster.
2. This is a behavioral change to low-level support code.  There is a
   risk of breaking code that may be relying on the old behavior.
tbkka added a commit to tbkka/swift that referenced this pull request Nov 2, 2023
Update PR swiftlang#68720 with lessons learned in reviewing swiftlang#69464

Background:

* SwiftValue can expose Swift value types (structs/enums)
  to ObjC by wrapping them in an Obj-C object on the heap

* SwiftObject is the Obj-C type that Swift class objects all
  inherit from (when viewed from Obj-C).  This allows arbitrary
  Swift class objects to be passed into Obj-C.

History:

* PR swiftlang#4124 made Obj-C `-hash` and `-isEqual:` work for SwiftValue for Hashable Swift types

* PR swiftlang#68720 extended SwiftValue to also support Equatable Swift types

* PR swiftlang#69464 added similar support to SwiftObject

In the process of working through swiftlang#69464, we found a better way
to handle an ObjC request for `-hash` for a type that is Swift
Equatable but not Hashable.  This PR updates SwiftValue to use
the same approach.  The approach considers three cases:

1. A Hashable type can forward both `-hash` and `-isEqual:` to
   the Swift object.

2. A type that is neither Equatable nor Hashable can implement
   `-isEqual:` as the identity check and `-hash` as returning
   the address of the object in memory.

3. A type is that Equatable but not Hashable is more complex.

In this last case, we can easily forward `-isEqual:` to the
Equatable conformance but ObjC also requires us to
always provide a compatible `-hash` implementation.
The only way to do this is to have `-hash` return a constant,
but that is a very bad idea in general, so we're also including
a log message whenever we see a request for `-hash` on
a Swift value that is Equatable but not Hashable.
To limit performance problems from the logging itself, we
emit the log message only once for each type.
@tbkka tbkka deleted the tbkka-SwiftValue-Equatable branch August 1, 2024 16:38
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.

4 participants