Spurious results in self = [self -init] test

[davidchisnall]

Bugzilla Link	9189
Version	trunk
OS	All
CC	@akyrtzi,@tkremenek

Extended Description

The missing "self = [(super or self) init...]" test in the static analyser looks like a nice idea, but I've run it on our codebase, seen over a thousand reports, and so far every single one of them that I've checked has been a false positive, which makes the test a bit less useful.

One common case is in -initWithCoder:. Classes implementing this method should only call [super initWithCoder:] if the superclass implements the NSCoding protocol.

I'm not sure what the best way of solving this is. Ideally, we want two extra annotations:

1. Specifying a designated initialiser for a class, so that all subclasses are required to call that initialiser

2. Specify that an initialiser is expected to only call itself in the superclass (annotation on the method declaration in the protocol). This is true for -initWithCoder: and a few other things. This could actually be a more general annotation, saying that a subclass implementation of any method is expected to call the superclass version if it exists, but with a special case for init*: methods, where it overrides the need to call [{self,super} init].

[davidchisnall]

assigned to @tkremenek

[tkremenek]

The missing "self = [(super or self) init...]" test in the static analyser
looks like a nice idea, but I've run it on our codebase, seen over a thousand
reports, and so far every single one of them that I've checked has been a false
positive, which makes the test a bit less useful.

One common case is in -initWithCoder:. Classes implementing this method should
only call [super initWithCoder:] if the superclass implements the NSCoding
protocol.

David: Just to get a clear picture, are most of the false positives symptomatic of a handful of cases not reasoned about in the checker? It would really be helpful to see some of these examples to make the dialogue more concrete.

[davidchisnall]

The complaint about -initWithCoder: not calling -init (which it shouldn't) is quite common. I think we get one report of that nature for almost every class that implements NSCoding but has a superclass that doesn't.

There are also a lot of cases where, as an optimisation, subclasses don't call [super init] in their designated initalizer, because the person writing them knew that this method was empty.

This is technically incorrect, but you'll see a lot of cases of if even in Apple's Foundation (replace [NSObject init] in a category and you'll see a lot of cases where you'd expect it to be called but it isn't). It's pretty common to not bother calling [super init] in cases where:

1. The superclass is NSObject, or
2. The superclasses are all in this framework

I'm not sure what the best solution is here. We'd rather not add a load of [super init] things that walk the class hierarchy doing nothing, but we'd also like to avoid just turning this check off.

[tkremenek]

The complaint about -initWithCoder: not calling -init (which it shouldn't) is
quite common. I think we get one report of that nature for almost every class
that implements NSCoding but has a superclass that doesn't.

I think the initWithCode case should be specially handled, if it isn't already. I'd prefer a separate bug report for that (so we can outline a specific, concrete issue and track it's resolution). I'm fine with marking this bug blocked on that one.

There are also a lot of cases where, as an optimisation, subclasses don't call
[super init] in their designated initalizer, because the person writing them
knew that this method was empty.

This is technically incorrect, but you'll see a lot of cases of if even in
Apple's Foundation (replace [NSObject init] in a category and you'll see a lot
of cases where you'd expect it to be called but it isn't). It's pretty common
to not bother calling [super init] in cases where:

1. The superclass is NSObject, or

This is probably something we can special case. I'd personally prefer to discuss it with the Cocoa teams first before we relax that restriction in the checker.

2. The superclasses are all in this framework

That's very domain specific knowledge. Possibly an annotation?

I'm not sure what the best solution is here. We'd rather not add a load of
[super init] things that walk the class hierarchy doing nothing, but we'd also
like to avoid just turning this check off.

We're investigating ways to suppress false positives/warnings for individual checks. That may be one possible solution, but that requires annotating individual -init implementations, which isn't necessarily ideal.

[akyrtzi]

There are also a lot of cases where, as an optimisation, subclasses don't call
[super init] in their designated initalizer, because the person writing them
knew that this method was empty.

This is technically incorrect, but you'll see a lot of cases of if even in
Apple's Foundation (replace [NSObject init] in a category and you'll see a lot
of cases where you'd expect it to be called but it isn't). It's pretty common
to not bother calling [super init] in cases where:

1. The superclass is NSObject, or
2. The superclasses are all in this framework

Since all the false positives for this are in the "I know I don't have to call [super init]" category, how about we don't warn if no [super init] is called, but do if it is called but the result is not set to self:

{
return self; // #​1 - no warning.
}

{
[super init];
return self; // #​2 - warn.
}

But warn for #​1, if there is an annotation for a designated initializer in super class.

[davidchisnall]

Since all the false positives for this are in the "I know I don't have to call
[super init]" category, how about we don't warn if no [super init] is called,
but do if it is called but the result is not set to self:

Yes, that actually sounds like the correct solution. As a first approximation, I think that would give more helpful results than the current one.

A second pass would probably be to provide an annotation for the designated initializer and require that:

• Every other init method (except -initWithCoder: - maybe as a special case for NSCoding, or maybe via another annotation) called [self designatedInitializer]

• The designated initialiser in the subclass, or every initialiser if there is no designated initialiser, calls the superclass's designated initialiser.

[tkremenek]

Since all the false positives for this are in the "I know I don't have to call
[super init]" category, how about we don't warn if no [super init] is called,
but do if it is called but the result is not set to self:

Yes, that actually sounds like the correct solution. As a first approximation,
I think that would give more helpful results than the current one.

I think this is a reasonable start. We can also provide a variant of the checker that is more aggressive for clients that want stricter checking even in the cases where no '[super initXXX]' occurs.

[tkremenek]

Since all the false positives for this are in the "I know I don't have to call
[super init]" category, how about we don't warn if no [super init] is called,
but do if it is called but the result is not set to self:

Yes, that actually sounds like the correct solution. As a first approximation,
I think that would give more helpful results than the current one.

I think this is a reasonable start. We can also provide a variant of the
checker that is more aggressive for clients that want stricter checking even in
the cases where no '[super initXXX]' occurs.

I implemented this, but found that it actually missed a fair number of bugs. It seems like a throwing the baby out with the bath water.

Other than the -initWithCoder: case, adding the annotations that David suggested (designated initializer, tc.) might be helpful, but that seems like a broader solution that requires a ton of work from the user. Perhaps we can make this more turnkey with just a handful of heuristics?

[davidchisnall]

Since all the false positives for this are in the "I know I don't have to call
[super init]" category, how about we don't warn if no [super init] is called,
but do if it is called but the result is not set to self:

Yes, that actually sounds like the correct solution. As a first approximation,
I think that would give more helpful results than the current one.

I think this is a reasonable start. We can also provide a variant of the
checker that is more aggressive for clients that want stricter checking even in
the cases where no '[super initXXX]' occurs.

I implemented this, but found that it actually missed a fair number of bugs.
It seems like a throwing the baby out with the bath water.

Other than the -initWithCoder: case, adding the annotations that David
suggested (designated initializer, tc.) might be helpful, but that seems like a
broader solution that requires a ton of work from the user. Perhaps we can
make this more turnkey with just a handful of heuristics?

Looking at the GNUstep code base, there are a lot of cases where you are not expected to call [super init]. This happens whenever you see the class cluster pattern (i.e. all over the Foundation framework and most other nontrivial ObjC frameworks). The -init method in the abstract class will frequently throw an exception.

A general annotation saying 'don't call this if you override it' that could be applied to any method, along with the converse ('always call this if you override it') might be helpful for silencing the warnings. I'm not sure what the best solution for NSCoding is - maybe treat that as a special case with an NSCoding checker (which could also check that the -encodeWithCoder: and -initWithCoder: methods are functional inverses).

For designated initialisers, the heuristic is the -init* method that takes the most arguments. By convention, all other initialisers (including -init) are expected to call this, and this is the only method that should call [super -init] (or whatever the designated initialiser is in the superclass). The annotation would probably need to handle the case where there is no designated initialiser, and the case where there is no initialiser that should be called in subclasses.

With this in mind, I wonder if the real solution would be a pattern annotation on classes. For example, providing pattern(class_cluster) and pattern(singleton) attributes that would appear before the @​interface directive, allowing the initialiser to understand that this class follows special rules for that pattern. This would then allow us to do more specific checks to see whether the class actually implemented the pattern correctly.

[davidchisnall]

Since all the false positives for this are in the "I know I don't have to call
[super init]" category, how about we don't warn if no [super init] is called,
but do if it is called but the result is not set to self:

Yes, that actually sounds like the correct solution. As a first approximation,
I think that would give more helpful results than the current one.

I think this is a reasonable start. We can also provide a variant of the
checker that is more aggressive for clients that want stricter checking even in
the cases where no '[super initXXX]' occurs.

I implemented this, but found that it actually missed a fair number of bugs.
It seems like a throwing the baby out with the bath water.

Other than the -initWithCoder: case, adding the annotations that David
suggested (designated initializer, tc.) might be helpful, but that seems like a
broader solution that requires a ton of work from the user. Perhaps we can
make this more turnkey with just a handful of heuristics?

Rerunning the analyser, we're now up to 715 reports from this checker, on a codebase that was reporting 653 yesterday with a 90%+ false positive rate.

A very simple heuristic that might be sensible would be to disable the check for any class that is a direct subclass of NSObject / Object / NSProxy. These classes only declare one instance variable (isa), so their -init methods are likely to be irrelevant and it's safe to ignore them (it's bad style, but it's so widespread that getting everyone to 'fix' their code in a way that slows it down and doesn't alter the semantics is not likely to be successful).

[tkremenek]

Since all the false positives for this are in the "I know I don't have to call
[super init]" category, how about we don't warn if no [super init] is called,
but do if it is called but the result is not set to self:

Yes, that actually sounds like the correct solution. As a first approximation,
I think that would give more helpful results than the current one.

I think this is a reasonable start. We can also provide a variant of the
checker that is more aggressive for clients that want stricter checking even in
the cases where no '[super initXXX]' occurs.

I implemented this, but found that it actually missed a fair number of bugs.
It seems like a throwing the baby out with the bath water.

Other than the -initWithCoder: case, adding the annotations that David
suggested (designated initializer, tc.) might be helpful, but that seems like a
broader solution that requires a ton of work from the user. Perhaps we can
make this more turnkey with just a handful of heuristics?

Rerunning the analyser, we're now up to 715 reports from this checker, on a
codebase that was reporting 653 yesterday with a 90%+ false positive rate.

Unless the code has change, I don't know why the count would be higher. We should strictly be emitting less warnings now. Are you using TOT Clang?

[tkremenek]

A very simple heuristic that might be sensible would be to disable the check
for any class that is a direct subclass of NSObject / Object / NSProxy. These
classes only declare one instance variable (isa), so their -init methods are
likely to be irrelevant and it's safe to ignore them (it's bad style, but it's
so widespread that getting everyone to 'fix' their code in a way that slows it
down and doesn't alter the semantics is not likely to be successful).

With the modern Objective-C runtime, there is no guarantee that these classes will always have only one instance variable (isa) in the future.

[davidchisnall]

A very simple heuristic that might be sensible would be to disable the check
for any class that is a direct subclass of NSObject / Object / NSProxy. These
classes only declare one instance variable (isa), so their -init methods are
likely to be irrelevant and it's safe to ignore them (it's bad style, but it's
so widespread that getting everyone to 'fix' their code in a way that slows it
down and doesn't alter the semantics is not likely to be successful).

With the modern Objective-C runtime, there is no guarantee that these classes
will always have only one instance variable (isa) in the future.

That's true in the technical sense, but there's a phenominal amount of code in the wild that doesn't bother calling [super init] in these classes (even in Apple frameworks), so making a change that required this call would break so much code that it's not really feasible.