[cfe-dev] Additional annotations for static analysis (Objective C designated initializers)

[Ted Kremenek]

On Nov 1, 2008, at 10:47 PM, Louis Gerbarg wrote:

> Anyway, let me know what you think. I think it should be stylistically
> consistent with the rest of the codebase, but C++ is a little rusty
> (Imagine that, someone trying to enhance the Objective C analyzer
> doesn't write much C++ ;-)

Here's some comments on the actual check.  Comments are inline.  One  
meta comment: the LLVM coding convention is to stay within 80 columns;  
there are a few cases in the patch where the lines are slightly too  
long.


Index: Driver/AnalysisConsumer.cpp
===================================================================
--- Driver/AnalysisConsumer.cpp	(revision 58556)
+++ Driver/AnalysisConsumer.cpp	(working copy)
@@ -431,6 +431,11 @@
                     mgr.getLangOptions(), BR);
  }

+static void ActionWarnObjCDesignatedInitializer(AnalysisManager& mgr) {
+  BugReporter BR(mgr);
+   
CheckObjCDesignatedInitializers 
(cast<ObjCImplementationDecl>(mgr.getCodeDecl()), BR);
+}

Looks good.  Stay within 80 cols.  ;-)

Index: Driver/Analyses.def
===================================================================
--- Driver/Analyses.def	(revision 58556)
+++ Driver/Analyses.def	(working copy)
+ANALYSIS(WarnObjCDesignatedInitializer, "warn-objc-designated- 
initializer",
+ "Warn about Objective-C classes that incorrectly use designated  
initializers",
+  ObjCImplementation)

Looks good!



Analysis/CheckObjCDesignatedInitializers.cpp
===================================================================
--- lib/Analysis/CheckObjCDesignatedInitializers.cpp	(revision 0)
+++ lib/Analysis/CheckObjCDesignatedInitializers.cpp	(revision 0)
...
+using namespace clang;
+
+// FIXME: This should check the control flow to make sure that all
+// paths go through the designated initializer

By "all paths", you mean within an initXXX method that calls [self  
initYYY], where initYYY is the designated initializer?

+
+static bool scan_init_method(Stmt* S,
+                             IdentifierInfo* SelfII,
+                             ObjCInterfaceDecl *C) {
+
+  if (ObjCMessageExpr* ME = dyn_cast<ObjCMessageExpr>(S)) {
+    if (ObjCMethodDecl *MD = C->lookupInstanceMethod(ME- 
 >getSelector()))
+      if (MD->getAttr<DesignatedInitializerAttr>())
+        if (ME->getReceiver())
+          if (Expr* Receiver = ME->getReceiver()->IgnoreParenCasts())
+            if (SelfII) {
+
+              // Self is defined, check to make sure it goes through  
self's
+              // desginated initializer

Spello: s/desginated/designated/.

+              if (DeclRefExpr* E = dyn_cast<DeclRefExpr>(Receiver))
+                if (E->getDecl()->getIdentifier() == SelfII)
+                  return true;

There is unfortunately a difference between the "self" variable which  
as implicit parameter to a method and any variable that can be named  
self.  It is perfectly okay to the following in Objective-C:

(id) myMethod: {
    int self = 42;
}

I would check:

   if (E->getDecl() == MD->getSelfDecl())

You also don't need SelfII in this case either.  Use a flag instead to  
indicate you are looking for self/super (more below).

+            } else {
+
+              // Check to make sure we go through super's designated
+              // initializer
+              if (PredefinedExpr* E =  
dyn_cast<PredefinedExpr>(Receiver))
+                if (E->getIdentType() == PredefinedExpr::ObjCSuper)
+                  return true;
+            }

Recently the represention of 'super' was changed to it's own AST node:

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

Just check: isa<ObjCSuperExpr>(Receiver->ignoreParenCasts())

+
+  // Recurse to children.
+  for (Stmt::child_iterator I = S->child_begin(), E= S->child_end();  
I!=E; ++I)
+    if (*I && scan_init_method(*I, SelfII, C))
+      return true;
+
+  return false;
+}

No SelfII needed in the recursion.


+
+void clang::CheckObjCDesignatedInitializers(ObjCImplementationDecl* ID,
+                                       BugReporter& BR) {
+
+  ObjCInterfaceDecl* D = ID->getClassInterface();
+  ObjCInterfaceDecl* C = D->getSuperClass();

Looks good.

+  bool hasDI = false;
+  bool superHasDI = false;

Looks good.
+
+  // Check if self or super has declared designated initializers
+  for (ObjCInterfaceDecl::instmeth_iterator I=D->instmeth_begin(),
+       E=D->instmeth_end(); I!=E; ++I) {
+
+    ObjCMethodDecl* M = *I;
+    if (M->getAttr<DesignatedInitializerAttr>()) {
+      hasDI = true;
+      break;
+    }
+  }

Looks great.

+
+  if (C) {
+    for (ObjCInterfaceDecl::instmeth_iterator I=C->instmeth_begin(),
+         E=C->instmeth_end(); I!=E; ++I) {
+
+      ObjCMethodDecl* M = *I;
+      if (M->getAttr<DesignatedInitializerAttr>()) {
+        superHasDI = true;
+        break;
+      }
+    }
+  }

Looks good.  Consider refactoring these two for loops into a static  
method that returns a bool.

+
+  if (!hasDI && !superHasDI)
+    return;
+
+  ASTContext& Ctx = BR.getContext();
+  IdentifierInfo* SelfII = &Ctx.Idents.get("self");

SelfII not needed.

+
+  for (ObjCImplementationDecl::instmeth_iterator I=ID- 
 >instmeth_begin(),
+       E=ID->instmeth_end(); I!=E; ++I) {
+
+    ObjCMethodDecl* M = *I;
+    ObjCMethodDecl* IM = D->lookupInstanceMethod(M->getSelector());
+
+    // Check if it is an init method
+    if (!strncmp("init", M->getSelector().getName().c_str(), 4)) {
+      Stmt* S = M->getBody();
+      if (S) {
+

Consider:

if (IM->getAttr<DesignatedInitializerAttr>()) {
   if (superHasDI) { ... }
}
else if (hasDI) {

}

This makes the logic a little more simple, and doesn't require two  
calls to getAttr().


+        // Check if this is designated initializer
+        if (superHasDI && IM->getAttr<DesignatedInitializerAttr>()) {
+
+          // Make sure we go through super's designated init
+          if (!scan_init_method(S, NULL, C)) {

Instead of passing NULL, pass a flag indicating that you are looking  
for super, since you don't need SelfII (and it doesn't work in the  
general case).

+            std::string buf;
+            llvm::raw_string_ostream os(buf);
+
+            os << "The designated initializer method '-[" << D- 
 >getName() << " "
+               << IM->getSelector().getName() << "]' does not call a  
designated "
+               << "initializer for superclass '" << C->getName() <<  
"'";
+
+            BR.EmitBasicReport("Incorrect super initializer",
+                               os.str().c_str(), M->getLocStart());
+          }
+        }

Looks great!

+	
+        // Check if this goes through a designated initializer
+        if (hasDI && !IM->getAttr<DesignatedInitializerAttr>()) {
+
+          // Make sure we go through self's designated init
+          if (!scan_init_method(S, SelfII, D)) {

Again, use a flag.

+            std::string buf;
+            llvm::raw_string_ostream os(buf);
+
+            os << "The initializer method '-[" << D->getName() << " "
+               << IM->getSelector().getName()
+               << "]' does not call a designated initializer";
+
+            BR.EmitBasicReport("Incorrect initializer",
+                               os.str().c_str(), M->getLocStart());
+          }

Looks great!

In the future we can refine this check to use the information from the  
path-sensitive engine.  This will also handle corner cases where self/ 
super get aliases (which should never happen in practice, but you  
never know).

You can also add a "bug category" in addition to a bug type.  This  
could be something like "Coding Convention (Designated  
Initializers)".  It's just a way to further group reports in scan- 
view's table of reports.  The category just follows the bug type, and  
precedes the diagnostic.

This patch is really awesome.  I'm really impressed.