Update plugin to handle new style rules for virtual annotations.

tools/clang/plugins/FindBadConstructsConsumer.cpp

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "FindBadConstructsConsumer.h"
 
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/AST/Attr.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 
 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 kRedundantVirtualSpecifier[] =
+    "[chromium-style] %0 is redundant; %1 implies %0.";
+// http://llvm.org/bugs/show_bug.cgi?id=21051 has been filed to make this a
+// Clang warning.
+const char kBaseMethodVirtualAndFinal[] =
+    "[chromium-style] The virtual method does not override anything and is "
+    "final; consider making it non-virtual.";
 const char kNoExplicitDtor[] =
     "[chromium-style] Classes that are ref-counted should have explicit "
     "destructors that are declared protected or private.";
 const char kPublicDtor[] =
     "[chromium-style] Classes that are ref-counted should have "
     "destructors that are declared protected or private.";
 const char kProtectedNonVirtualDtor[] =
     "[chromium-style] Classes that are ref-counted and have non-private "
     "destructors should declare their destructor virtual.";
 const char kWeakPtrFactoryOrder[] =
@@ skipping 21 matching lines @@
 // any namespace qualifiers. This is similar to desugaring, except that for
 // ElaboratedTypes, desugar will unwrap too much.
 const Type* UnwrapType(const Type* type) {
   if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type))
     return UnwrapType(elaborated->getNamedType().getTypePtr());
   if (const TypedefType* typedefed = dyn_cast<TypedefType>(type))
     return UnwrapType(typedefed->desugar().getTypePtr());
   return type;
 }
 
+FixItHint FixItRemovalForVirtual(const SourceManager& manager,
+                                 const CXXMethodDecl* method) {
+  // 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 range(method->getLocStart(),
+                    Lexer::getLocForEndOfToken(
+                        method->getLocStart(), 0, manager, LangOptions()));
+  // Get the spelling loc just in case it was expanded from a macro...
+  SourceRange spelling_range(manager.getSpellingLoc(range.getBegin()),
+                             manager.getSpellingLoc(range.getEnd()));
+  // Sanity check that the text looks like virtual.
+  StringRef text = clang::Lexer::getSourceText(
+      CharSourceRange::getCharRange(spelling_range), manager, LangOptions());
+  if (!text.startswith("virtual"))

[hans, 2014/09/27 00:36:10]

Any reason not to check if (text != "virtual") ?

[dcheng, 2014/09/28 18:35:39]

I was worried about whitespace, but I forgot about StringRef::trim. However,
while investigating this a bit more, I found out I had a logic error. For
macros, the SourceRange I was passing to getSourceText() was returning the text
from the beginning of the macro definition to the end of where the macro was
expanded.

I changed this to just use token SourceRanges instead of char SourceRanges. I
thought this would break with macros that expanded to more than just virtual
(see the definition of method H() in virtual_base_method_also_final.cpp), but
Clang somehow knows to filter out that fixit hint.

+    return FixItHint();
+  return FixItHint::CreateRemoval(range);
+}
+
 }  // namespace
 
 FindBadConstructsConsumer::FindBadConstructsConsumer(CompilerInstance& instance,
                                                      const Options& options)
     : ChromeClassTester(instance), options_(options) {
-  // Register warning/error messages.
+  // Messages for virtual method specifiers.
   diag_method_requires_override_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresOverride);
-  diag_method_requires_virtual_ =
-      diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresVirtual);
+  diag_redundant_virtual_specifier_ =
+      diagnostic().getCustomDiagID(getErrorLevel(), kRedundantVirtualSpecifier);
+  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_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kBadLastEnumValue);
 
   // Registers notes to make it easier to interpret warnings.
   diag_note_inheritance_ =
       diagnostic().getCustomDiagID(DiagnosticsEngine::Note, kNoteInheritance);
   diag_note_implicit_dtor_ =
       diagnostic().getCustomDiagID(DiagnosticsEngine::Note, kNoteImplicitDtor);
   diag_note_public_dtor_ =
       diagnostic().getCustomDiagID(DiagnosticsEngine::Note, kNotePublicDtor);
   diag_note_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID(
       DiagnosticsEngine::Note, kNoteProtectedNonVirtualDtor);
 }
 
 void FindBadConstructsConsumer::CheckChromeClass(SourceLocation record_location,
                                                  CXXRecordDecl* record) {
   bool implementation_file = InImplementationFile(record_location);
 
   if (!implementation_file) {
     // Only check for "heavy" constructors/destructors in header files;
     // within implementation files, there is no performance cost.
     CheckCtorDtorWeight(record_location, record);
   }
 
   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);
 
   if (options_.check_weak_ptr_factory_order)
     CheckWeakPtrFactoryMembers(record_location, record);
 }
 
 void FindBadConstructsConsumer::CheckChromeEnum(SourceLocation enum_location,
                                                 EnumDecl* enum_decl) {
@@ skipping 125 matching lines @@
                   "destructor.");
     } else if (CXXDestructorDecl* dtor = record->getDestructor()) {
       if (dtor->hasInlineBody()) {
         emitWarning(dtor->getInnerLocStart(),
                     "Complex destructor has an inline body.");
       }
     }
   }
 }
 
-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;
 }
 
 bool FindBadConstructsConsumer::IsMethodInBannedOrTestingNamespace(
     const CXXMethodDecl* method) {
   if (InBannedNamespace(method))
     return true;
   for (CXXMethodDecl::method_iterator i = method->begin_overridden_methods();
        i != method->end_overridden_methods();
        ++i) {
     const CXXMethodDecl* overridden = *i;
     if (IsMethodInBannedOrTestingNamespace(overridden) ||
         InTestingNamespace(overridden)) {
       return true;
     }
   }
 
   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) {
     CXXRecordDecl* record_type = it->getTypeSourceInfo()
                                      ->getTypeLoc()
                                      .getTypePtr()
                                      ->getAsCXXRecordDecl();
     if (record_type) {
       if (InTestingNamespace(record_type)) {
         return;
       }
     }
   }
 
   for (CXXRecordDecl::method_iterator it = record->method_begin();
        it != record->method_end();
        ++it) {
     if (it->isCopyAssignmentOperator() || isa<CXXConstructorDecl>(*it)) {
       // Ignore constructors and assignment operators.
     } else if (isa<CXXDestructorDecl>(*it) &&
                !record->hasUserDeclaredDestructor()) {
       // Ignore non-user-declared destructors.
+    } else if (!it->isVirtual()) {
+      continue;
     } else {
-      CheckVirtualMethod(*it, warn_on_inline_bodies);
-      CheckOverriddenMethod(*it);
+      CheckVirtualSpecifiers(*it);
+      if (warn_on_inline_bodies)
+        CheckVirtualBodies(*it);
     }
   }
 }
+
+// Makes sure that virtual methods use the most appropriate specifier. If a
+// virtual method overrides a method from a base class, only the override
+// specifier should be used. If the method should not be overridden by derived
+// classes, only the final specifier should be used.
+void FindBadConstructsConsumer::CheckVirtualSpecifiers(
+    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;
+
+  if (isa<CXXDestructorDecl>(method) && !options_.strict_virtual_specifiers)
+    return;
+
+  SourceManager& manager = instance().getSourceManager();
+
+  // Complain if a method is annotated virtual && (override || final).
+  if (has_virtual && (override_attr || final_attr) &&
+      options_.strict_virtual_specifiers) {
+    diagnostic().Report(method->getLocStart(),
+                        diag_redundant_virtual_specifier_)
+        << "'virtual'"
+        << (override_attr ? static_cast<Attr*>(override_attr) : final_attr)
+        << FixItRemovalForVirtual(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 {
+      diagnostic().Report(loc, diag_method_requires_override_);
+    }
+  }
+
+  if (final_attr && override_attr && options_.strict_virtual_specifiers) {
+    diagnostic().Report(override_attr->getLocation(),
+                        diag_redundant_virtual_specifier_)
+        << override_attr << final_attr
+        << FixItHint::CreateRemoval(override_attr->getRange());
+  }
+
+  if (final_attr && !is_override && options_.strict_virtual_specifiers) {
+    diagnostic().Report(method->getLocStart(),
+                        diag_base_method_virtual_and_final_)
+        << FixItRemovalForVirtual(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.");
+      }
+    }
+  }
+}
 
 void FindBadConstructsConsumer::CountType(const Type* type,
                                           int* trivial_member,
                                           int* non_trivial_member,
                                           int* templated_non_trivial_member) {
   switch (type->getTypeClass()) {
     case Type::Record: {
       // Simplifying; the whole class isn't trivial if the dtor is, but
       // we use this as a signal about complexity.
       if (TypeHasNonTrivialDtor(type))
@@ skipping 294 matching lines @@
               record->getTypeForDecl()->getAsCXXRecordDecl()) {
             weak_ptr_factory_location = iter->getLocation();
           }
         }
       }
     }
   }
 }
 
 }  // namespace chrome_checker