Update plugin to handle new style rules for virtual annotations.

tools/clang/plugins/FindBadConstructsConsumer.cpp

Index: tools/clang/plugins/FindBadConstructsConsumer.cpp
diff --git a/tools/clang/plugins/FindBadConstructsConsumer.cpp b/tools/clang/plugins/FindBadConstructsConsumer.cpp
index a3c6e7f4f14b9316dd432cd5f6ff9bf03e1f7320..cadf872b3eb0600e171208736d2c006637631f0e 100644
--- a/tools/clang/plugins/FindBadConstructsConsumer.cpp
+++ b/tools/clang/plugins/FindBadConstructsConsumer.cpp
@@ -5,6 +5,7 @@
 #include "FindBadConstructsConsumer.h"
 
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/AST/Attr.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/Support/raw_ostream.h"
 
@@ -15,9 +16,19 @@ namespace chrome_checker {
 namespace {
 
 const char kMethodRequiresOverride[] =
-    "[chromium-style] Overriding method must be marked with OVERRIDE.";
-const char kMethodRequiresVirtual[] =
-    "[chromium-style] Overriding method must have \"virtual\" keyword.";
+    "[chromium-style] Overriding method must be marked with override or final.";
+const char kRedundantVirtualAnnotation[] =
+    "[chromium-style] %0 is redundant; %1 implies %0.";
+const char kDtorImplicitVirtual[] =
+    "[chromium-style] destructor is virtual and should be marked as such";
+const char kVirtualDtorAnnotation[] =
+    "[chromium-style] virtual destructors should not be annotated with %0";
+// http://llvm.org/bugs/show_bug.cgi?id=21051 has been filed to make this a
+// Clang warning.
+const char kBaseMethodVirtualAndFinal[] =
+    "[chromium-style] a virtual method on a base class should not also be "
+    "final; "

[Nico, 2014/09/24 17:38:06]

Once the strict checks are on, we won't need this, right? If only "virtual" or
"final" are allowed, then clang will error if you say "final" on a method that's
not virtual.

[dcheng, 2014/09/25 00:12:54]

This is, strictly speaking, unnecessary. But I think it's nice to prompt people
to devirtualize something if possible, if you end up in this state through
refactorings.

+    "method should be devirtualized";
 const char kNoExplicitDtor[] =
     "[chromium-style] Classes that are ref-counted should have explicit "
     "destructors that are declared protected or private.";
@@ -59,22 +70,45 @@ const Type* UnwrapType(const Type* type) {
   return type;
 }
 
+// Unfortunately, there doesn't seem to be a good way to determine the location
+// of the 'virtual' keyword. It's available in Declarator, but that isn't
+// accessible from the AST. So instead, make an educated guess that the first
+// token is probably the virtual keyword. Strictly speaking, this doesn't have
+// to be true, but it probably will be.
+// TODO(dcheng): Add a warning to force virtual to always appear first ;-)
+SourceRange GuessSourceRangeForVirtual(const SourceManager& manager,
+                                       const CXXMethodDecl* method) {
+  return SourceRange(method->getLocStart(),
+                     Lexer::getLocForEndOfToken(
+                         method->getLocStart(), 0, manager, LangOptions()));
+}
+
 }  // namespace
 
 FindBadConstructsConsumer::FindBadConstructsConsumer(CompilerInstance& instance,
                                                      const Options& options)
     : ChromeClassTester(instance), options_(options) {
-  // Register warning/error messages.
+  // Messages for virtual method annotations.
   diag_method_requires_override_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresOverride);
-  diag_method_requires_virtual_ =
-      diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresVirtual);
+  diag_redundant_virtual_annotation_ = diagnostic().getCustomDiagID(
+      getErrorLevel(), kRedundantVirtualAnnotation);
+  diag_dtor_implicit_virtual_ = diagnostic().getCustomDiagID(
+      getErrorLevel(), kDtorImplicitVirtual);
+  diag_virtual_dtor_annotation_ =
+      diagnostic().getCustomDiagID(getErrorLevel(), kVirtualDtorAnnotation);
+  diag_base_method_virtual_and_final_ =
+      diagnostic().getCustomDiagID(getErrorLevel(), kBaseMethodVirtualAndFinal);
+
+  // Messages for destructors.
   diag_no_explicit_dtor_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kNoExplicitDtor);
   diag_public_dtor_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kPublicDtor);
   diag_protected_non_virtual_dtor_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kProtectedNonVirtualDtor);
+
+  // Miscellaneous messages.
   diag_weak_ptr_factory_order_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kWeakPtrFactoryOrder);
   diag_bad_enum_last_value_ =
@@ -103,8 +137,7 @@ void FindBadConstructsConsumer::CheckChromeClass(SourceLocation record_location,
 
   bool warn_on_inline_bodies = !implementation_file;
 
-  // Check that all virtual methods are marked accordingly with both
-  // virtual and OVERRIDE.
+  // Check that all virtual methods are annotated with override or final.
   CheckVirtualMethods(record_location, record, warn_on_inline_bodies);
 
   CheckRefCountedDtors(record_location, record);
@@ -250,35 +283,6 @@ void FindBadConstructsConsumer::CheckCtorDtorWeight(
   }
 }
 
-void FindBadConstructsConsumer::CheckVirtualMethod(const CXXMethodDecl* method,
-                                                   bool warn_on_inline_bodies) {
-  if (!method->isVirtual())
-    return;
-
-  if (!method->isVirtualAsWritten()) {
-    SourceLocation loc = method->getTypeSpecStartLoc();
-    if (isa<CXXDestructorDecl>(method))
-      loc = method->getInnerLocStart();
-    SourceManager& manager = instance().getSourceManager();
-    FullSourceLoc full_loc(loc, manager);
-    SourceLocation spelling_loc = manager.getSpellingLoc(loc);
-    diagnostic().Report(full_loc, diag_method_requires_virtual_)
-        << FixItHint::CreateInsertion(spelling_loc, "virtual ");
-  }
-
-  // Virtual methods should not have inline definitions beyond "{}". This
-  // only matters for header files.
-  if (warn_on_inline_bodies && method->hasBody() && method->hasInlineBody()) {
-    if (CompoundStmt* cs = dyn_cast<CompoundStmt>(method->getBody())) {
-      if (cs->size()) {
-        emitWarning(cs->getLBracLoc(),
-                    "virtual methods with non-empty bodies shouldn't be "
-                    "declared inline.");
-      }
-    }
-  }
-}
-
 bool FindBadConstructsConsumer::InTestingNamespace(const Decl* record) {
   return GetNamespace(record).find("testing") != std::string::npos;
 }
@@ -300,47 +304,16 @@ bool FindBadConstructsConsumer::IsMethodInBannedOrTestingNamespace(
   return false;
 }
 
-void FindBadConstructsConsumer::CheckOverriddenMethod(
-    const CXXMethodDecl* method) {
-  if (!method->size_overridden_methods() || method->getAttr<OverrideAttr>())
-    return;
-
-  if (isa<CXXDestructorDecl>(method) || method->isPure())
-    return;
-
-  if (IsMethodInBannedOrTestingNamespace(method))
-    return;
-
-  SourceManager& manager = instance().getSourceManager();
-  SourceRange type_info_range =
-      method->getTypeSourceInfo()->getTypeLoc().getSourceRange();
-  FullSourceLoc loc(type_info_range.getBegin(), manager);
-
-  // Build the FixIt insertion point after the end of the method definition,
-  // including any const-qualifiers and attributes, and before the opening
-  // of the l-curly-brace (if inline) or the semi-color (if a declaration).
-  SourceLocation spelling_end =
-      manager.getSpellingLoc(type_info_range.getEnd());
-  if (spelling_end.isValid()) {
-    SourceLocation token_end =
-        Lexer::getLocForEndOfToken(spelling_end, 0, manager, LangOptions());
-    diagnostic().Report(token_end, diag_method_requires_override_)
-        << FixItHint::CreateInsertion(token_end, " OVERRIDE");
-  } else {
-    diagnostic().Report(loc, diag_method_requires_override_);
-  }
-}
-
-// Makes sure there is a "virtual" keyword on virtual methods.
-//
-// Gmock objects trigger these for each MOCK_BLAH() macro used. So we have a
-// trick to get around that. If a class has member variables whose types are
-// in the "testing" namespace (which is how gmock works behind the scenes),
-// there's a really high chance we won't care about these errors
+// Checks that virtual methods are correctly annotated, and have no body in a
+// header file.
 void FindBadConstructsConsumer::CheckVirtualMethods(
     SourceLocation record_location,
     CXXRecordDecl* record,
     bool warn_on_inline_bodies) {
+  // Gmock objects trigger these for each MOCK_BLAH() macro used. So we have a
+  // trick to get around that. If a class has member variables whose types are
+  // in the "testing" namespace (which is how gmock works behind the scenes),
+  // there's a really high chance we won't care about these errors
   for (CXXRecordDecl::field_iterator it = record->field_begin();
        it != record->field_end();
        ++it) {
@@ -363,9 +336,119 @@ void FindBadConstructsConsumer::CheckVirtualMethods(
     } else if (isa<CXXDestructorDecl>(*it) &&
                !record->hasUserDeclaredDestructor()) {
       // Ignore non-user-declared destructors.
+    } else if (!it->isVirtual()) {
+      continue;
+    } else {
+      CheckVirtualAnnotations(*it);
+      if (warn_on_inline_bodies)
+        CheckVirtualBodies(*it);
+    }
+  }
+}
+
+// Makes sure that virtual methods are correctly annotated. If a virtual method
+// overrides a method from a base class, the method must be annotated with
+// the override keyword. Furthermore, if the method is not meant to be further
+// subclasses, it must only be annotated with the final keyword.
+void FindBadConstructsConsumer::CheckVirtualAnnotations(
+    const CXXMethodDecl* method) {
+  bool is_override = method->size_overridden_methods() > 0;
+  bool has_virtual = method->isVirtualAsWritten();
+  OverrideAttr* override_attr = method->getAttr<OverrideAttr>();
+  FinalAttr* final_attr = method->getAttr<FinalAttr>();
+
+  if (method->isPure())
+    return;
+
+  if (IsMethodInBannedOrTestingNamespace(method))
+    return;
+
+  SourceManager& manager = instance().getSourceManager();
+
+  // Destructors should never be annotated override or final.
+  if (isa<CXXDestructorDecl>(method)) {
+    if (!has_virtual && is_override) {
+      diagnostic().Report(method->getLocStart(),
+                          diag_dtor_implicit_virtual_)
+          << FixItHint::CreateInsertion(method->getLocStart(), "virtual ");
+    }
+
+    if (!options_.strict_virtual_annotations)
+      return;
+
+    if (override_attr) {
+      diagnostic().Report(override_attr->getLocation(),
+                          diag_virtual_dtor_annotation_)
+          << override_attr
+          << FixItHint::CreateRemoval(override_attr->getRange());
+    }
+    if (final_attr) {
+      diagnostic().Report(final_attr->getLocation(),
+                          diag_virtual_dtor_annotation_)
+          << final_attr << FixItHint::CreateRemoval(final_attr->getRange());
+    }
+    return;
+  }
+
+  // Complain if a method is annotated virtual && (override || final).
+  if (has_virtual && (override_attr || final_attr) &&
+      options_.strict_virtual_annotations) {
+    diagnostic().Report(method->getLocStart(),
+                        diag_redundant_virtual_annotation_)
+        << "virtual"
+        << (override_attr ? static_cast<Attr*>(override_attr) : final_attr)
+        << FixItHint::CreateRemoval(
+               GuessSourceRangeForVirtual(manager, method));
+  }
+
+  // Complain if a method is an override and is not annotated with override or
+  // final.
+  if (is_override && !override_attr && !final_attr) {
+    SourceRange type_info_range =
+        method->getTypeSourceInfo()->getTypeLoc().getSourceRange();
+    FullSourceLoc loc(type_info_range.getBegin(), manager);
+
+    // Build the FixIt insertion point after the end of the method definition,
+    // including any const-qualifiers and attributes, and before the opening
+    // of the l-curly-brace (if inline) or the semi-color (if a declaration).
+    SourceLocation spelling_end =
+        manager.getSpellingLoc(type_info_range.getEnd());
+    if (spelling_end.isValid()) {
+      SourceLocation token_end =
+          Lexer::getLocForEndOfToken(spelling_end, 0, manager, LangOptions());
+      diagnostic().Report(token_end, diag_method_requires_override_)
+          << FixItHint::CreateInsertion(token_end, " override");
     } else {
-      CheckVirtualMethod(*it, warn_on_inline_bodies);
-      CheckOverriddenMethod(*it);
+      diagnostic().Report(loc, diag_method_requires_override_);
+    }
+  }
+
+  if (final_attr && override_attr && options_.strict_virtual_annotations) {
+    diagnostic().Report(override_attr->getLocation(),
+                        diag_redundant_virtual_annotation_)
+        << override_attr << final_attr
+        << FixItHint::CreateRemoval(override_attr->getRange());
+  }
+
+  if (final_attr && !is_override && options_.strict_virtual_annotations) {
+    diagnostic().Report(method->getLocStart(),
+                        diag_base_method_virtual_and_final_)
+        << FixItHint::CreateRemoval(GuessSourceRangeForVirtual(manager, method))
+        << FixItHint::CreateRemoval(final_attr->getRange());
+  }
+}
+
+void FindBadConstructsConsumer::CheckVirtualBodies(
+    const CXXMethodDecl* method) {
+  // Virtual methods should not have inline definitions beyond "{}". This
+  // only matters for header files.
+  if (method->hasBody() && method->hasInlineBody()) {
+    if (CompoundStmt* cs = dyn_cast<CompoundStmt>(method->getBody())) {
+      if (cs->size()) {
+        emitWarning(cs->getLBracLoc(),
+                    "virtual methods with non-empty bodies shouldn't be "
+                    "declared inline.");
+      }
     }
   }
 }