Bug 767563 - Add a clang static checker, part 2: Implement the MOZ_MUST_OVERRIDE attribute. r=ehsan

build/clang-plugin/Makefile.in

@@ -15,6 +15,7 @@ CPPSRCS := \
 	$(NULL)
 
 TESTSRCS := \
+	TestMustOverride.cpp \
 	$(NULL)
 
 OBJS := $(patsubst %.cpp,%.o,$(CPPSRCS))

build/clang-plugin/clang-plugin.cpp

@@ -23,6 +23,66 @@ public:
   virtual void HandleTranslationUnit(ASTContext &ctx) {
     TraverseDecl(ctx.getTranslationUnitDecl());
   }
+
+  static bool hasCustomAnnotation(const Decl *d, const char *spelling) {
+    AnnotateAttr *attr = d->getAttr<AnnotateAttr>();
+    if (!attr)
+      return false;
+
+    return attr->getAnnotation() == spelling;
+  }
+
+  bool VisitCXXRecordDecl(CXXRecordDecl *d) {
+    // We need definitions, not declarations
+    if (!d->isThisDeclarationADefinition()) return true;
+
+    // Look through all of our immediate bases to find methods that need to be
+    // overridden
+    typedef std::vector<CXXMethodDecl *> OverridesVector;
+    OverridesVector must_overrides;
+    for (CXXRecordDecl::base_class_iterator base = d->bases_begin(),
+         e = d->bases_end(); base != e; ++base) {
+      // The base is either a class (CXXRecordDecl) or it's a templated class...
+      CXXRecordDecl *parent = base->getType()
+        .getDesugaredType(d->getASTContext())->getAsCXXRecordDecl();
+      // The parent might not be resolved to a type yet. In this case, we can't
+      // do any checking here. For complete correctness, we should visit
+      // template instantiations, but this case is likely to be rare, so we will
+      // ignore it until it becomes important.
+      if (!parent) {
+        continue;
+      }
+      parent = parent->getDefinition();
+      for (CXXRecordDecl::method_iterator M = parent->method_begin();
+          M != parent->method_end(); ++M) {
+        if (hasCustomAnnotation(*M, "moz_must_override"))
+          must_overrides.push_back(*M);
+      }
+    }
+
+    for (OverridesVector::iterator it = must_overrides.begin();
+        it != must_overrides.end(); ++it) {
+      bool overridden = false;
+      for (CXXRecordDecl::method_iterator M = d->method_begin();
+          !overridden && M != d->method_end(); ++M) {
+        // The way that Clang checks if a method M overrides its parent method
+        // is if the method has the same name but would not overload.
+        if (M->getName() == (*it)->getName() &&
+            !CI.getSema().IsOverload(*M, (*it), false))
+          overridden = true;
+      }
+      if (!overridden) {
+        unsigned overrideID = Diag.getDiagnosticIDs()->getCustomDiagID(
+            DiagnosticIDs::Error, "%0 must override %1");
+        unsigned overrideNote = Diag.getDiagnosticIDs()->getCustomDiagID(
+            DiagnosticIDs::Note, "function to override is here");
+        Diag.Report(d->getLocation(), overrideID) << d->getDeclName() <<
+          (*it)->getDeclName();
+        Diag.Report((*it)->getLocation(), overrideNote);
+      }
+    }
+    return true;
+  }
 };
 
 class MozCheckAction : public PluginASTAction {

build/clang-plugin/tests/TestMustOverride.cpp

@@ -0,0 +1,63 @@
+#define MOZ_MUST_OVERRIDE __attribute__((annotate("moz_must_override")))
+// Ignore warnings not related to static analysis here
+#pragma GCC diagnostic ignored "-Woverloaded-virtual"
+
+struct S {
+  virtual void f() MOZ_MUST_OVERRIDE; // expected-note {{function to override is here}}
+  virtual void g() MOZ_MUST_OVERRIDE;
+  virtual void h() MOZ_MUST_OVERRIDE; // expected-note {{function to override is here}}
+};
+struct C : S { // expected-error {{'C' must override 'f'}} expected-error {{'C' must override 'h'}}
+  virtual void g() MOZ_MUST_OVERRIDE; // expected-note {{function to override is here}}
+  virtual void h(int);
+  void q() MOZ_MUST_OVERRIDE; // expected-note {{function to override is here}}
+};
+struct D : C { // expected-error {{'D' must override 'g'}} expected-error {{'D' must override 'q'}}
+  virtual void f();
+};
+
+struct Base {
+  virtual void VirtMethod() MOZ_MUST_OVERRIDE; // expected-note {{function to override is here}}
+  void NonVirtMethod() MOZ_MUST_OVERRIDE; // expected-note {{function to override is here}}
+  static void StaticMethod() MOZ_MUST_OVERRIDE;
+};
+
+struct DoesNotPropagate : Base {
+  virtual void VirtMethod();
+  void NonVirtMethod();
+  static void StaticMethod();
+};
+
+struct Final : DoesNotPropagate { };
+
+struct Propagates : Base {
+  virtual void VirtMethod() MOZ_MUST_OVERRIDE; // expected-note {{function to override is here}}
+  void NonVirtMethod() MOZ_MUST_OVERRIDE; // expected-note {{function to override is here}}
+  static void StaticMethod() MOZ_MUST_OVERRIDE; // expected-note {{function to override is here}}
+};
+
+struct FailsFinal : Propagates { }; // expected-error {{'FailsFinal' must override 'VirtMethod'}} expected-error {{'FailsFinal' must override 'NonVirtMethod'}} expected-error {{'FailsFinal' must override 'StaticMethod'}}
+
+struct WrongOverload : Base { // expected-error {{'WrongOverload' must override 'VirtMethod'}} expected-error {{'WrongOverload' must override 'NonVirtMethod'}}
+  virtual void VirtMethod() const;
+  void NonVirtMethod(int param);
+  static void StaticMethod();
+};
+
+namespace A { namespace B { namespace C {
+  struct Param {};
+  struct Base {
+    void f(Param p) MOZ_MUST_OVERRIDE; // expected-note {{function to override is here}}
+  };
+}}}
+
+struct Param {};
+
+struct Derived : A::B::C::Base {
+  typedef A::B::C::Param Typedef;
+  void f(Typedef t);
+};
+
+struct BadDerived : A::B::C::Base { // expected-error {{'BadDerived' must override 'f'}}
+  void f(Param p);
+};

xpcom/tests/static-checker/OverrideFail1.cpp

@@ -1,7 +0,0 @@
-#include "nscore.h"
-
-struct S {
-  virtual NS_MUST_OVERRIDE void f();
-  virtual void g();
-};
-struct C : S { virtual void g(); }; // ERROR: must override f()

xpcom/tests/static-checker/OverrideFail2.cpp

@@ -1,8 +0,0 @@
-#include "nscore.h"
-
-struct S {
-  virtual NS_MUST_OVERRIDE void f();
-  virtual void g();
-};
-
-struct D : S { virtual void f(int); }; // ERROR: different overload

xpcom/tests/static-checker/OverrideFail3.cpp

@@ -1,10 +0,0 @@
-#include "nscore.h"
-
-struct S {
-  virtual NS_MUST_OVERRIDE void f();
-  virtual void g();
-};
-
-struct B : S { virtual NS_MUST_OVERRIDE void f(); }; // also ok
-struct F : B { }; // ERROR: B's definition of f() is still must-override
-

xpcom/tests/static-checker/OverrideFail4.cpp

@@ -1,13 +0,0 @@
-#include "nscore.h"
-
-struct Base {
-  NS_MUST_OVERRIDE void f();
-};
-
-struct Intermediate : Base {
-  NS_MUST_OVERRIDE void f();
-};
-
-struct Derived : Intermediate {
-  // error: must override Intermediate's f()
-};

xpcom/tests/static-checker/OverrideOK1.cpp

@@ -1,10 +0,0 @@
-#include "nscore.h"
-
-struct S {
-  virtual NS_MUST_OVERRIDE void f();
-  virtual void g();
-};
-
-struct A : S { virtual void f(); }; // ok
-struct B : S { virtual NS_MUST_OVERRIDE void f(); }; // also ok
-struct E : A { }; // ok: A's definition of f() is good for subclasses

xpcom/tests/static-checker/OverrideOK2.cpp

@@ -1,24 +0,0 @@
-#include "nscore.h"
-
-struct Base {
-  NS_MUST_OVERRIDE virtual void f();  // normal case
-  NS_MUST_OVERRIDE void g();          // virtual not required
-  NS_MUST_OVERRIDE static void h();   // can even be static
-};
-
-void Base::f() {} // can be defined, or not, don't care
-
-struct Derived1 : Base { // propagates override annotation
-  NS_MUST_OVERRIDE virtual void f();
-  NS_MUST_OVERRIDE void g();
-  NS_MUST_OVERRIDE static void h();
-};
-
-struct Derived2 : Derived1 { // doesn't propagate override annotation
-  virtual void f();
-  void g();
-  static void h();
-};
-
-struct Derived3 : Derived2 { // doesn't have to override anything
-};

xpcom/tests/static-checker/OverrideOK3-NamespaceTypedef.cpp

@@ -1,16 +0,0 @@
-#include "nscore.h"
-
-namespace A {
-namespace B {
-namespace C {
-  struct Param {};
-
-  class Base {
-    NS_MUST_OVERRIDE virtual void f(Param p) {}
-  };
-}}}
-
-class Derived : public A::B::C::Base {
-  typedef A::B::C::Param P;
-  void f(P p) {}
-};