Bug 512726 - Add NS_MUST_OVERRIDE static annotation. r=bsmedberg

--HG--
extra : rebase_source : 4f3034c93cc76c7504d1cfb21953c495c35d068f

config/static-checking-config.mk

@@ -5,6 +5,7 @@ DEHYDRA_SCRIPT = $(topsrcdir)/config/static-checking.js
 
 DEHYDRA_MODULES = \
   $(topsrcdir)/xpcom/analysis/final.js \
+  $(topsrcdir)/xpcom/analysis/must-override.js \
   $(NULL)
 
 TREEHYDRA_MODULES = \

config/static-checking.js

@@ -57,6 +57,25 @@ function hasAttribute(c, attrname)
   return false;
 }
 
+// This is useful for detecting method overrides
+function signaturesMatch(m1, m2)
+{
+  if (m1.shortName != m2.shortName)
+    return false;
+  
+  let p1 = m1.type.parameters;
+  let p2 = m2.type.parameters;
+  
+  if (p1.length != p2.length)
+    return false;
+  
+  for (let i = 0; i < p1.length; ++i)
+    if (p1[i] !== p2[i])
+      return false;
+  
+  return true;
+}
+
 const forward_functions = [
   'process_type',
   'process_tree_type',

xpcom/analysis/must-override.js

@@ -0,0 +1,48 @@
+/*
+ * Detect classes that should have overridden members of their parent
+ * classes but didn't.
+ *
+ * Example:
+ *
+ * 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 C : S { virtual void g(); }; // ERROR: must override f()
+ * struct D : S { virtual void f(int); }; // ERROR: different overload
+ * struct E : A { }; // ok: A's definition of f() is good for subclasses
+ * struct F : B { }; // ERROR: B's definition of f() is still must-override
+ *
+ * We don't care if you define the method or not.
+ */
+
+function get_must_overrides(cls)
+{
+  let mos = {};
+  for each (let base in cls.bases)
+    for each (let m in base.type.members)
+      if (hasAttribute(m, 'NS_must_override')) 
+       mos[m.shortName] = m;
+
+  return mos;
+}
+
+function process_type(t)
+{
+  if (t.isIncomplete || (t.kind != 'class' && t.kind != 'struct'))
+    return;
+
+  let mos = get_must_overrides(t);
+  for each (let m in t.members) {
+    let mos_m = mos[m.shortName]
+    if (mos_m && signaturesMatch(mos_m, m))
+      delete mos[m.shortName];
+  }
+
+  for each (let u in mos)
+    error(t.kind + " " + t.name + " must override " + u.name, t.loc);
+}

xpcom/analysis/type-printer.js

@@ -74,21 +74,6 @@ function publicMembers(t)
   }
 }
 
-function signaturesMatch(m1, m2)
-{
-  let p1 = m1.type.parameters;
-  let p2 = m2.type.parameters;
-  
-  if (p1.length != p2.length)
-    return false;
-  
-  for (let i = 0; i < p1.length; ++i)
-    if (p1[i] !== p2[i])
-      return false;
-  
-  return true;
-}
-
 /**
  * Get the short name of a decl name. E.g. turn
  * "MyNamespace::MyClass::Method(int j) const" into

xpcom/tests/static-checker/Makefile.in

@@ -112,9 +112,22 @@ FLOW_PASS_TESTCASES = \
 FLOW_FAILURE_TESTCASES = \
  flow_through_fail.cpp
 
+MUST_OVERRIDE_PASS_TESTCASES = \
+  OverrideOK1.cpp \
+  OverrideOK2.cpp \
+  $(NULL)
+
+MUST_OVERRIDE_FAILURE_TESTCASES = \
+  OverrideFail1.cpp \
+  OverrideFail2.cpp \
+  OverrideFail3.cpp \
+  OverrideFail4.cpp \
+  $(NULL)
+
 STATIC_FAILURE_TESTCASES = \
   $(FINAL_FAILURE_TESTCASES) \
-  $(FLOW_FAILURE_TESTCASES)
+  $(FLOW_FAILURE_TESTCASES) \
+  $(MUST_OVERRIDE_FAILURE_TESTCASES) \
   $(NULL)
 
 STATIC_WARNING_TESTCASES = \
@@ -127,6 +140,7 @@ STATIC_PASS_TESTCASES = \
   $(OUTPARAMS_PASS_TESTCASES) \
   $(STACK_PASS_TESTCASES) \
   $(FLOW_PASS_TESTCASES) \
+  $(MUST_OVERRIDE_PASS_TESTCASES) \
   $(NULL)
 
 

xpcom/tests/static-checker/OverrideFail1.cpp

@@ -0,0 +1,7 @@
+#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

@@ -0,0 +1,8 @@
+#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

@@ -0,0 +1,10 @@
+#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

@@ -0,0 +1,13 @@
+#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

@@ -0,0 +1,10 @@
+#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

@@ -0,0 +1,24 @@
+#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
+};