Objective-C: Instance variable used while 'self' not set... but it is

Question

I've been tasked with cleaning up some Clang errors in a code base. I am very new to iPhone development and Objective C, but have found most of the problems trivial... this one is stumping me though, when I'm sure its something embarrassing.

From a ZAttributedString class:

- (id)initWithAttributedString:(ZAttributedString *)attr {
    NSParameterAssert(attr != nil);
    if ((self = [super init])) {
        _buffer = [attr->_buffer mutableCopy];
        _attributes = [[NSMutableArray alloc] initWithArray:attr->_attributes copyItems:YES];
    }
    return self;
}

The clang warning is "Instance variable used while 'self' is not set to the result of '[super or self] init...]', with the dereferencing of attr's _buffer attribute being highlighted.

If it helps, the warning also seems to mention that the problem is found when calling from this method:

- (id)copyWithZone(NSZone *)zone {
    return [(ZAttributedString *)[ZAttributedString allocWithZone:zone] initWithAttributedString:self];
}

Can anyone please explain to me what exactly the defect is here?

TIA!

Lustrum

Answer 1

Do not use -> to access instance variables, especially when the ivar is from some other object.

Do this:

_buffer = [[attr string] mutableCopy];

Same goes for that nasty attr->_attributes. Apparently, ZAttributedStringexposesattributes` as a property in the private header.

---

That compiler warning does seem, at the very most optimistic, entirely misleading and, likely, quite wrong in description. Filing a bug to have that clarified would be useful.

---

Note that @maddy's claim that using -> to access the instance variables directly in the attr string passed as it acts like a copy constructor is incorrect.

The incoming attr may be a ZAttributedString instance or an instance of a subclass or, really, an instance of any class that implements the same interface as ZAttributedString. Thus, you really must go through the accessors to guarantee that you are grabbing the correct state.

Now, as an implementation detail, ZAttributedString could require that the inbound instance be a non-subclassed instance of ZAttributedString, but it should use isMemberOfClass: to assert that requirement (and, please, don't do that).

The only spot where direct ivar access is sometimes used to pull state from another object is in the implementation of copyWithZone:, but that is exceedingly fragile and oft leads to whacky broken behavior. In fact, copyWithZone: (outside of the various plist compatible value classes) has been rife with fragility and the source of many many many bugs.

Gehenna

Actually, this code is from the ZAttributedString class so it is perfectly correct to use -> to access ivars from another instance of the same class. This is appropriate in such code when you are basically creating a "copy constructor". – Gamba

Not really correct, even in this case. If it were the -copy method itself, then maybe (even that is a really rough edge in ObjC's history -- NSCopyObject() has caused no end of pain). But not in the DI. The class may have logic on the getters that do something and that in-bound instance may very likely be a subclass with behaviors unknown. – Gehenna

@bbum: "The incoming attr may be a ZAttributedString instance or an instance of a subclass" self could also be an instance of a subclass, so by that argument, you can never access instance variables, which is obviously not true. – Tumefy

"Do not use -> to access instance variables, especially when the ivar is from some other object." Most object-oriented languages allows people to access other objects' instance variables, and their tutorials often show it being done. You might argue that that's a bad idea, but it's not such an unusual thing in general. – Tumefy

@Tumefy The class of self vs. attr is entirely irrelevant. In an init method, the state of the object is guaranteed to be undefined, therefore direct set of ivars is the correct pattern to employ exactly because the subclass's initializer has yet to execute (typically). What other languages do doesn't matter. – Gehenna

The key is to consider overridden setter logic; if a subclass's setter has logic based on state assume in its initializer, the super cannot use the potentially overridden setter because said initializer is unlikely to have been executed. While the setter could do initialization first, that is atypical and fragile in and of itself. The setting of initial state should lack side-effects and that requires direct setting of said state. – Gehenna

@bbum: okay, consider any method other than an init method. According to your logic, it is never okay to access instance variables in any of those methods? – Tumefy

For self? Direct access without the -> is common (but sometimes has to be refactored into method calls later) and recommended in init/dealloc. Other object? Never outside of the exceedingly rare, oft misguided, performance case, when using a class as a struct (very rare, also oft misguided), and in copyWithZone: (which is historically fragile and a source of both bugs and pain when subclassing). – Gehenna

Answer 2

It seems like you are seeing the exact same bug as this: "[Bug 15092] New: static analyzer false positive: reports instance variable used while 'self' is not set to the result of [(super or self)] init". It has a very similar code attached to reproduce the bug.

If you run that code in Xcode 4.6.3 you can verify that it gives the same false warning as you are seeing.

The bug was resolved with the comment:

This is fixed in trunk, or at least mostly fixed -- there are still a few edge cases where the warning will fire, but not your project.

(Dave, for now all of the main analyzer engineers do work at Apple, so there's no real need to file duplicates. The LLVM people who don't work at Apple don't have access to Apple Clang, which ships with Xcode, and this fix didn't make Xcode 4.6. You can also get newer checker builds from http://clang-analyzer.llvm.org)

As you can see the bug is fixed but still present in Xcode 4.6. Hold out for the next version of Xcode and the analyzer warning should be gone.

Capitoline