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.";

[hans, 2014/09/26 01:29:29]

pretty nitty, and we probably haven't done this so far, but I think we should
put single quotes around 'override' and 'final here. That corresponds to how
Clang prints its diagnostics.

[dcheng, 2014/09/26 07:07:13]

Done.

+const char kRedundantVirtualSpecifier[] =
+    "[chromium-style] %0 is redundant; %1 implies %0.";

[hans, 2014/09/26 01:29:29]

as per above, I'd put single quotes around the %0's here. (it seems %1 already
gets single quotes, because we're passing in an Attr)

[dcheng, 2014/09/26 07:07:14]

Done, though I opted to just fix the instance where we pass in a string literal
rather than adding quotes here.

+// 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; "
+    "method should be devirtualized";

[hans, 2014/09/26 01:29:29]

s/on a base class/in a base class/

I'm not sure it would be obvious to everyone what "should be devirtualized"
means here. I'm also not sure about the "base class" part.

Maybe: "[chromium-style] The virtual method does not override anything and is
final; it should be made non-virtual". (But could the fix sometimes be to make
it non-final?)

[dcheng, 2014/09/26 07:07:13]

Updated with your text and weakened the suggestion a bit. I think it's just
worth prodding people to make something non-virtual (they might think about if
it really still needs to be 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;
 }
 
+// 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) {

[hans, 2014/09/26 01:29:29]

It might be overkill, but you could reach into the SourceManager and check the
bytes at the source range you're looking at here actually say "virtual", and
bail out if it doesn't.

[dcheng, 2014/09/26 07:07:13]

Heh. Done.

+  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 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, the method must use only
+// the override specifier. Furthermore, if the method is not meant to be further
+// subclasses, only the final specifier may be used.

[hans, 2014/09/26 01:29:29]

"not meant to be further subclasses" sounds weird.

[dcheng, 2014/09/26 07:07:13]

Oops, that's a typo. Fixed.

+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)
+        << 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 {
+      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_)
+        << FixItHint::CreateRemoval(GuessSourceRangeForVirtual(manager, method))

[hans, 2014/09/26 01:29:29]

Removing the virtual specifier should probably be gated on has_virtual.

[dcheng, 2014/09/26 07:07:13]

It's not needed, because the following does not compile:
class Base {
 public:
  void F() final;
};
will trigger a warning that a non-virtual method cannot be final.

+        << 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