[1.x] Fix unrecognized selector crash on static linking #803
Conversation
|
@zcsipler Could you try this branch? |
Generated by 🚫 danger |
b2713f8 to
ed9d1ca
Compare
Extension methods or properties for NSObject subclasses are somehow invisible from the Objective-C runtime on static linking, so let's make a wrapper class.
ed9d1ca to
8af4712
Compare
There was a problem hiding this comment.
Could we fix this by using the -ObjC linker flag? https://stackoverflow.com/questions/6629979/what-does-the-objc-linker-flag-do
|
Oh I completely forgot the linker flag! 💦 Thanks for the tip! Indeed, adding the flag to test targets which use Quick (e.g. However, forcing consumers to add the flag to their test targets just for this internal implementation detail will/may not be ideal I think. What do you think? Is that an acceptable cost to use static linking? |
There was a problem hiding this comment.
However, forcing consumers to add the flag to their test targets just for this internal implementation detail will/may not be ideal I think. What do you think? Is that an acceptable cost to use static linking?
I don't mind hosting the method under a name we control, but I think it still makes sense to preserve the existing API. Left a couple of suggestions, lemme know what you think!
| @@ -22,12 +21,25 @@ public extension NSString { | |||
| return invalidCharacters | |||
| }() | |||
|
|
|||
| @objc(qck_c99ExtendedIdentifier) | |||
There was a problem hiding this comment.
Because there's a workaround, I think we should preserve this public API.
There was a problem hiding this comment.
I'm okay to preserve this for 1.x but I'd like to hide this in 2.x, because I assume the reason why this API is public is to expose it to the generated header. We can now expose internal Swift APIs to the generated header thanks to #791, so there will be no need to make it public.
There was a problem hiding this comment.
Can we explicitly deprecate it?
There was a problem hiding this comment.
I tried adding @available(*, deprecated) but __attribute__ ((deprecated)) is not exposed to the generated header unfortunately.
| /// from the Objective-C runtime on static linking, so let's make a wrapper class. | ||
| /// | ||
| /// See: https://github.com/Quick/Quick/issues/785 | ||
| @objc |
There was a problem hiding this comment.
Might be a good idea to prefix this new class?
There was a problem hiding this comment.
I agree, QCK prefix should be added.
| var c99ExtendedIdentifier: String { | ||
| let validComponents = components(separatedBy: NSString.invalidCharacters) | ||
| let result = validComponents.joined(separator: "_") | ||
|
|
||
| return result.isEmpty ? "_" : result | ||
| } | ||
| } | ||
|
|
||
| /// Extension methods or properties for NSObject subclasses are somehow invisible | ||
| /// from the Objective-C runtime on static linking, so let's make a wrapper class. |
There was a problem hiding this comment.
Because we know that -ObjC will expose these to the runtime, this comment might need an update.
|
@sharplet Thanks for the detailed review! 💖 I updated the pull request. |
|
Thanks!
I will try to update |
Extension methods or properties for NSObject subclasses are somehow invisible from the Objective-C runtime on static linking, so let's make a wrapper class.Extension methods or properties for NSObject subclasses are invisible from the Objective-C runtime on static linking unless the consumers add
-ObjClinker flag, so let's make a wrapper class to mitigate that situation.Fixes #785.