Bug 512726 - Add NS_MUST_OVERRIDE static annotation. r=bsmedberg
authorTaras Glek <tglek@mozilla.com>
Fri, 18 Sep 2009 10:26:13 -0700
changeset 32858 2047ed35d947bd114713fc2b858b3f6f75402a31
parent 32857 1e6cadb7afd013df2220de914e53afc2a18ce8ec
child 32859 51e562b1032e3ba9a69b58ac33936e05284421ec
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg
bugs512726
milestone1.9.3a1pre
Bug 512726 - Add NS_MUST_OVERRIDE static annotation. r=bsmedberg
config/static-checking-config.mk
config/static-checking.js
xpcom/analysis/must-override.js
xpcom/analysis/type-printer.js
xpcom/tests/static-checker/Makefile.in
xpcom/tests/static-checker/OverrideFail1.cpp
xpcom/tests/static-checker/OverrideFail2.cpp
xpcom/tests/static-checker/OverrideFail3.cpp
xpcom/tests/static-checker/OverrideFail4.cpp
xpcom/tests/static-checker/OverrideOK1.cpp
xpcom/tests/static-checker/OverrideOK2.cpp
--- a/config/static-checking-config.mk
+++ b/config/static-checking-config.mk
@@ -1,15 +1,16 @@
 # The entire tree should be subject to static analysis using the XPCOM
 # script. Additional scripts may be added by specific subdirectories.
 
 DEHYDRA_SCRIPT = $(topsrcdir)/config/static-checking.js
 
 DEHYDRA_MODULES = \
   $(topsrcdir)/xpcom/analysis/final.js \
+  $(topsrcdir)/xpcom/analysis/must-override.js \
   $(NULL)
 
 TREEHYDRA_MODULES = \
   $(topsrcdir)/xpcom/analysis/outparams.js \
   $(topsrcdir)/xpcom/analysis/stack.js \
   $(topsrcdir)/xpcom/analysis/flow.js \
   $(topsrcdir)/js/src/jsstack.js \
   $(topsrcdir)/layout/generic/frame-verify.js \
--- a/config/static-checking.js
+++ b/config/static-checking.js
@@ -52,16 +52,35 @@ function hasAttribute(c, attrname)
 
   for each (attr in c.attributes)
     if (attr.name == 'user' && attr.value[0] == attrname)
       return true;
 
   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',
   'process_decl',
   'process_tree_decl',
   'process_function',
   'process_tree',
   'process_cp_pre_genericize',
new file mode 100644
--- /dev/null
+++ b/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);
+}
--- a/xpcom/analysis/type-printer.js
+++ b/xpcom/analysis/type-printer.js
@@ -69,31 +69,16 @@ function publicMembers(t)
       if (member.access != "public")
         continue;
       
       yield member;
     }
   }
 }
 
-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
  * "Method"
  */
 function getShortName(decl)
 {
   let name = decl.name;
--- a/xpcom/tests/static-checker/Makefile.in
+++ b/xpcom/tests/static-checker/Makefile.in
@@ -107,31 +107,45 @@ OUTPARAMS_PASS_TESTCASES = \
   $(NULL)
 
 FLOW_PASS_TESTCASES = \
  flow_through_pass.cpp
 
 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 = \
   $(OUTPARAMS_WARNING_TESTCASES) \
   $(STACK_FAILURE_TESTCASES) \
   $(NULL)
 
 STATIC_PASS_TESTCASES = \
   $(OUTPARAMS_NS_FAILED_TESTCASES) \
   $(OUTPARAMS_PASS_TESTCASES) \
   $(STACK_PASS_TESTCASES) \
   $(FLOW_PASS_TESTCASES) \
+  $(MUST_OVERRIDE_PASS_TESTCASES) \
   $(NULL)
 
 
 include $(topsrcdir)/config/rules.mk
 
 # We want to compile each file and invert the result to ensure that
 # compilation failed.
 check:: \
new file mode 100644
--- /dev/null
+++ b/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()
new file mode 100644
--- /dev/null
+++ b/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
new file mode 100644
--- /dev/null
+++ b/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
+
new file mode 100644
--- /dev/null
+++ b/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()
+};
new file mode 100644
--- /dev/null
+++ b/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
new file mode 100644
--- /dev/null
+++ b/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
+};