theos / logos

Preprocessor that simplifies Objective-C hooking.
https://theos.dev/docs/logos
Other
204 stars 34 forks source link

Theos should raise an exception when hooking a non-implemented selector #49

Open codyd51 opened 5 years ago

codyd51 commented 5 years ago

It's easy to make a mistake with theos by placing a method-hook under the wrong %hook class, or adding a typo to a selector name. In this case, the hooks will silently fail, and the methods will not be swizzled, leading to confusion of whether the target methods are being invoked at all.

Instead, theos should loudly fail when I provide an 'invalid hook'. This would catch this bug much earlier in development and reduce headaches.

leptos-null commented 5 years ago

Logging for this was removed to avoid initializing the class.

uroboro commented 4 years ago

If you mean at runtime, it’s coverec by the previous comment. If you mean at compile time this would require some heavy lifting to parse the code, the sdk and figure out if there’s an error somewhere. It would be even more complex if a tweak supports multiple versions where classes or methods change on major or minor updates.

codyd51 commented 4 years ago

Of course I agree that doing this statically is a non-starter.

But, it seems totally doable dynamically and a valuable QoL improvement. I understand the justification to want to avoid the hooking platform initializing a class, but this isn't a serious blocker and can be worked around:

Ctrl-f CLS_INITIALIZED

Personally, I struggle to think of a bug caused by a class being initialized too early - its dependency chain of other classes should already have been +loaded, for example. But, if you're sure you want to handle the edge case, either of these might work:

  1. Hooking a class auto-creates an +initialize hook which does the MSHookMessageEx

  2. The code above might be wrapped by if (CLS_GETINFO(objc_getClass("classToHook"), CLS_INITIALIZED)) { ... }

codyd51 commented 4 years ago

I now see bugs caused by the behavior mentioned in the commit message. I think both approaches above would avoid it.