gnustep / libobjc2

Objective-C runtime library intended for use with Clang.
http://www.gnustep.org/
MIT License
426 stars 116 forks source link

Instance variable with the same name as a superclass' instance variable is not allowed #246

Closed madsmtm closed 9 months ago

madsmtm commented 9 months ago

Compiling and running the following works on Apple platforms, but fails on GNUStep, since the second class_addIvar checks if any ivars are present before adding it. Doing such a check is correct, but the bug is that it checks the superclass hierarchy as well, instead of only checking itself.

#import <objc/runtime.h>

int main(int argc, const char * argv[]) {
    Class cls1 = objc_allocateClassPair(objc_getClass("NSObject"), "MyClass", 0);
    class_addIvar(cls1, "myIvar", sizeof(int), log2(_Alignof(int)), @encode(int));
    objc_registerClassPair(cls1);

    Class cls2 = objc_allocateClassPair(cls1, "MySubClass", 0);
    class_addIvar(cls2, "myIvar", sizeof(int), log2(_Alignof(int)), @encode(int));
    objc_registerClassPair(cls2);

    return 0;
}
davidchisnall commented 9 months ago

I’m curious what code you have that triggers this. Having ivars with the same name is almost always a terrible idea. It will break KVC / KVO in exciting ways and anything else that depends on various reflection APIs.

We can allow it, but that won’t prevent the subtle breakage that you’ll get if you actually rely on it.

madsmtm commented 9 months ago

Having ivars with the same name is almost always a terrible idea.

The problem is that of forward compatibility; you do not have any guarantee that the class you want to subclass has not registered the ivar you want to use already. E.g. let's say I'm subclassing NSView, and it works fine for my version of AppKit/libs-gui, but in a later version it adds an ivar internally with a name that I happened to be using.

The only other way I see of solving this would be to prefix each variable with my class' name, something like "MyClass_myIvar" and "MySubClass_myIvar", which seems like an ugly solution, but maybe the only real way to prevent this problem?

I’m curious what code you have that triggers this.

Concretely, I'm developing an interoperability layer between Objective-C and Rust (feedback welcome), and am exposing an interface to objc_allocateClassPair/objc_registerClassPair as the main way for users to declare classes. (In the future, I may try to generate the same statics that clang does, but it's a lot easier to start by working with the runtime rather than around it).

It will break KVC / KVO in exciting ways and anything else that depends on various reflection APIs.

Can you give some details on what you might think would break? I'd especially be interested to know if it causes undefined behaviour.

davidchisnall commented 9 months ago

It will break KVC / KVO in exciting ways and anything else that depends on various reflection APIs.

Can you give some details on what you might think would break? I'd especially be interested to know if it causes undefined behaviour.

I don't think it's undefined behaviour, but things like object_setInstanceVariable will now act on the new ivar. If a superclass has opted into key-value coding exposing the instance variable directly, then the KVC infrastructure will new one. If the superclass is using the KVC setters to trigger KVO notifications, but is directly reading from the ivar then this will cause it to be in an unexpected state (it thinks it's set the ivar, it has in fact set your ivar, so both the subclass and the superclass have unexpected values).

This gets even more confusing if the instance variables have different types. If you call class_setInstanceVariable knowing the type from the superclass, but they don't match, then you will get undefined behaviour. For example, imagine:

  1. Superclass has an ivar foo that is a long.
  2. Subclass adds an ivar foo that is an id.
  3. Code that knows about the superclass ivar knows calls class_setInstanceVariable with the value 42.
  4. The runtime finds foo with type id and calls objc_retain on the new value.
  5. 42 is not a valid pointer, the runtime crashes in objc_retain

A lot of Objective-C code depends (directly or indirectly) on these reflection functions working. I'm hesitant to remove a bug-finding feature without a concrete example of something that it breaks, given that it helps users avoid shipping code that will fail in subtle and hard-to-debug ways.

madsmtm commented 9 months ago

Honestly sounds like everything should be using class_setInstanceVariable with the precise class that the ivar was declared on, and that object_setInstanceVariable should be discouraged, but I digress.

My point still stands that none of this is forwards compatible, but I guess such issues are rare enough that it's not worth handling, especially if Objective-C elsewhere implicitly relies on ivar names being unique within a class hierarchy.

Thanks for answering my questions!