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

Fixes an issue where bridging an Objective-C type called Optional into a Swift-based project causes LLDB issues #274

Merged
merged 4 commits into from
Sep 25, 2018

Conversation

tprevost-phunware
Copy link
Contributor

@tprevost-phunware tprevost-phunware commented Jun 27, 2018

I've found that bridging an Objective-C protocol called Optional into a Swift-based project causes LLDB to sometimes not be able to distinguish between Optimizely's Optional and Swift.Optional. For example, if you set a breakpoint inside a closure, then add

[weak self]

to the closure, then hit the breakpoint during execution and try to po anything in the debugger, you'll get this:

error: :1:11: error: 'Optional' is ambiguous for type lookup in this context
extension Optional where Wrapped: $__lldb_context {
^~~~~~~~

Swift.Optional:1:13: note: found this candidate
public enum Optional : ExpressibleByNilLiteral {
^

OptimizelySDKCore.Optional:1:17: note: found this candidate
public protocol Optional {
^

error: :18:5: error: value of type 'Optional' has no member '$__lldb_wrapped_expr_8'
$__lldb_injected_self.$__lldb_wrapped_expr_8(
^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~

This PR simply renames Optional to OPTLYOptional, which I have found fixes the issue.

I tried using NS_SWIFT_NAME(OPTLYOptional), but that still causes LLDB to spit out errors like the above, because NS_SWIFT_NAME(OPTLYOptional) seems to only create a typealias for Optimizely's Optional type and does not actually replace the name at all.

Have I run all the tests to make sure they still pass? Yes. Well, the OptimizelySDKCoreiOS and OptimizelySDKiOS tests at least. I couldn't find a way to just run all the tests. I tried some of the tvOS tests, but they wouldn't even build, saying they could not find the file <OptimizelySDKShared/OPTLYDatafileConfig.h>. 😕

…es LLDB to sometimes not be able to distinguish between Swift.Optional and OptimizelySDKiOS.Optional.
@tprevost-phunware
Copy link
Contributor Author

I've also made a bug report here.

@tprevost-phunware tprevost-phunware changed the title Fix an issue where importing an Objective-C type called Optional... Fixes an issue where importing an Objective-C type called Optional causes LLDB issues Jun 28, 2018
@tprevost-phunware tprevost-phunware changed the title Fixes an issue where importing an Objective-C type called Optional causes LLDB issues Fixes an issue where bridging an Objective-C type called Optional into a Swift-based project causes LLDB issues Jun 28, 2018
Copy link
Contributor

@thomaszurkan-optimizely thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

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

LGTM

@tprevost-phunware
Copy link
Contributor Author

Actually, this may not be needed anymore since the Swift LLDB team has apparently fixed this issue here: apple/swift-lldb#750

Copy link
Contributor

@thomaszurkan-optimizely thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

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

@tprevost-phunware thank you so much for your contribution. I would have made the same change. Can you please sign this and I will merge your changes?
https://docs.google.com/a/optimizely.com/forms/d/e/1FAIpQLSf9cbouWptIpMgukAKZZOIAhafvjFCV8hS00XJLWQnWDFtwtA/viewform

@tprevost-phunware
Copy link
Contributor Author

@thomaszurkan-optimizely Sure thing. Done!

@optibot
Copy link
Contributor

optibot commented Aug 8, 2018

Can one of the admins verify this patch?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.555% when pulling e488185 on tprevost-phunware:master into f421f49 on optimizely:master.

@thomaszurkan-optimizely thomaszurkan-optimizely merged commit 428ad1d into optimizely:master Sep 25, 2018
AbdurRafay pushed a commit that referenced this pull request Oct 1, 2018
* master:
  update changelog for new release (#322)
  add test script and fix broken carthage build (#321)
  Prepare for2.1.1 (#318)
  add EventTagUtil to universal project (#314)
  Fix an issue where importing an Objective-C type called Optional causes LLDB to sometimes not be able to distinguish between Swift.Optional and OptimizelySDKiOS.Optional. (#274)
  remove unused protocol (#313)
  fix(whitelisting): Invalid whitelisted variation stops bucketing (#308)
@asaschachar
Copy link
Contributor

@tprevost-phunware Thanks for the patch to the Optimizely Objective-C SDK! Hopefully it's now a bit easier to use for the Swift projects out there. Feel free to reach out to me at asa@optimizely.com if you are at all interested in joining Optimizely full-time!

@tprevost-phunware
Copy link
Contributor Author

No prob, @asaschachar! Glad I could help! Yeah I'll see if we can update to the latest version of the Optimizely SDK on our project (we're still on 1.5.0 unfortunately) to see this fix in action! Oh and thanks for the offer! I'm not looking for a job at the moment, but I'll keep Optimizely in mind for the future! :)

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.

5 participants