Missing dealloc analysis

I noticed the addition of missing dealloc analysis in r53075. It's a good idea to check that dealloc is implemented and that it always calls [super dealloc], but there're couple of points I would like to mention:

1. It should be disabled in GC environment, because dealloc methods are not called under GC

2. In non-GC environment, the primary function of dealloc method is to update retain counts of variables the object being dealloced is pointing to. So that usually means that dealloc must be implemented when an object has some ivars. Dealloc is not always required, and there're lot of classes which don't need it (e.g. singleton classes). Currently missing dealloc analysis requires every class to implement dealloc and it's causing a lot of false positives.

I'm not completely sure if absence of ivars should be the only factor which disables missing dealloc analysis, but at least in my case it would suppress a lot of false positives.

dealloc is also used to nullify the pointer in some other class that have a weak reference on the disposed instance.

You can have an object without ivar that is declare delegate of an other object and should call -[setDelegate:nil] on dealloc.
You have the same issue with notification listeners, that have to unregister when they are disposed.

And so, defining if dealloc should be implemented or not is not an easy task.

One minor point:

GCC emits a warning if -dealloc does not call [super dealloc], which is very frustrating when you are creating a root class (or a category on a root class) for which super is invalid, since it seems impossible to suppress the warning. Please don't replicate this bug!

David

Right now the check only applies to ObjCImplementationDecls (@implementation for those not familiar with clang internals). The check walks the super chain to see if the class is a subclass of NSObject. Classes that don't subclass NSObject (including root classes) are skipped by this check.

I'd love to replicate some of these great observations in the check itself. I'm not a Cocoa programmer, so if anyone would be kind to provide some self-contained code examples (that we can potentially add to the Clang testsuite) that would be awesome.

The goal of the check is to be pragmatic, not perfect. Ideally the check will have a modest false positive rate at the risk of some false negatives. If the check isn't useful than nobody will pay attention to it.

Hi Nikita,

I noticed the addition of missing dealloc analysis in r53075. It's a
good idea to check that dealloc is implemented and that it always
calls [super dealloc], but there're couple of points I would like to
mention:

1. It should be disabled in GC environment, because dealloc methods
are not called under GC

Done:

   http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080630/006371.html

2. In non-GC environment, the primary function of dealloc method is to
update retain counts of variables the object being dealloced is
pointing to. So that usually means that dealloc must be implemented
when an object has some ivars. Dealloc is not always required, and
there're lot of classes which don't need it (e.g. singleton classes).
Currently missing dealloc analysis requires every class to implement
dealloc and it's causing a lot of false positives.

I'm not completely sure if absence of ivars should be the only factor
which disables missing dealloc analysis, but at least in my case it
would suppress a lot of false positives.

Done:

   http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080630/006373.html

Both commits are now rolled into checker-55 on the Clang website.

These are both great suggestions. I remembered the GC/non-GC issue this morning before I read your email, and I can easily see how not checking to see if a class contains any ivars would lead to many false positives. I also believe that checking for ivars should also handle the Singleton design pattern (please correct me if I am wrong).

Please keep these great comments/suggestions coming! The goal is to have most checks have a false positive rate to not exceed 20%, and incorporating simple heuristics like these is often a huge win in cutting down false positives.

Ted

Hi Nikita,

I noticed the addition of missing dealloc analysis in r53075. It's a
good idea to check that dealloc is implemented and that it always
calls [super dealloc], but there're couple of points I would like to
mention:

1. It should be disabled in GC environment, because dealloc methods
are not called under GC

Done:

  http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080630/006371.html

2. In non-GC environment, the primary function of dealloc method is to
update retain counts of variables the object being dealloced is
pointing to. So that usually means that dealloc must be implemented
when an object has some ivars. Dealloc is not always required, and
there're lot of classes which don't need it (e.g. singleton classes).
Currently missing dealloc analysis requires every class to implement
dealloc and it's causing a lot of false positives.

I'm not completely sure if absence of ivars should be the only factor
which disables missing dealloc analysis, but at least in my case it
would suppress a lot of false positives.

Done:

  http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080630/006373.html

Both commits are now rolled into checker-55 on the Clang website.

These are both great suggestions. I remembered the GC/non-GC issue
this morning before I read your email, and I can easily see how not
checking to see if a class contains any ivars would lead to many false
positives. I also believe that checking for ivars should also handle
the Singleton design pattern (please correct me if I am wrong).

Why do you think singleton class need a special case ?

in obj-c a singleton class is usually implemented using a class method that return a shared instance:

+ (id)sharedInstance {
  static Foo *sharedInstance = nil;
  if (!sharedInstance) {
    sharedInstance = [[Foo alloc] init];
  }
  return sharedInstance;
}

optionaly you can also override +alloc, -init, -retain, -release to prevent creation of other instance, and destruction of the shared instance, but this is not recommanded (except if you have a good reason to do so).

It's a good practice to release ivars in a singleton dealloc, even if dealloc may never be call (like that, if needed, you can create other instance of this class).

So I don't think it need a special case, but I maybe wrong too.

This is especially true when you consider subclassing. A singleton can not guarantee that its subclasses will also be singletons, and so should release all of its ivars when it is -dealloc'd, or risk causing leaks if it is subclassed.

If a singleton needs to have shared resources that are also shared with subclasses then it should store them in file statics, no ivars.

David

This is especially true when you consider subclassing. A singleton can not guarantee that its subclasses will also be singletons, and so should release all of its ivars when it is -dealloc'd, or risk causing leaks if it is subclassed.

I think the current logic in the checker handles this. The check is obviated for a class if it doesn't contain any ivars, but the check is applied to every class, including subclasses of classes that didn't contain ivars. Consequently, it should handle the case where singletons are subclassed.

If a singleton needs to have shared resources that are also shared with subclasses then it should store them in file statics, no ivars.

Right. That's how I thought the Singleton pattern was typically implemented.

Yes, this is true, so the current "absence of ivars disables dealloc check" functionality should work with singletons as well, so no special cases needed. My original statement ("e.g. singletons don't need dealloc") was incorrect. But anyway, thanks for correction and quick implementation of these checks.