Warn about virtual methods in final classes

clang/docs/ReleaseNotes.rst

@@ -247,6 +247,9 @@ Improvements to Clang's diagnostics
 - The ``-Wsign-compare`` warning now treats expressions with bitwise not(~) and minus(-) as signed integers 
   except for the case where the operand is an unsigned integer
   and throws warning if they are compared with unsigned integers (##18878).
+- The ``-Wunnecessary-virtual-specifier`` warning has been added to warn about
+  methods which are marked as virtual inside a ``final`` class, and hence can
+  never be overridden.
 
 - Improve the diagnostics for chained comparisons to report actual expressions and operators (#GH129069).
 

clang/include/clang/Basic/DiagnosticGroups.td

@@ -372,6 +372,19 @@ def CXX11WarnInconsistentOverrideMethod :
 def CXX11WarnSuggestOverrideDestructor : DiagGroup<"suggest-destructor-override">;
 def CXX11WarnSuggestOverride : DiagGroup<"suggest-override">;
 
+def WarnUnnecessaryVirtualSpecifier : DiagGroup<"unnecessary-virtual-specifier"> {
+  code Documentation = [{
+Warns when a ``final`` class contains a virtual method (including virtual
+destructors). Since ``final`` classes cannot be subclassed, their methods
+cannot be overridden, and hence the ``virtual`` specifier is useless.
+
+The warning also detects virtual methods in classes whose destructor is
+``final``, for the same reason.
+
+The warning does not fire on virtual methods which are also marked ``override``.
+  }];
+}
+
 // Original name of this warning in Clang
 def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>;
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -2706,6 +2706,9 @@ def warn_final_dtor_non_final_class : Warning<
   InGroup<FinalDtorNonFinalClass>;
 def note_final_dtor_non_final_class_silence : Note<
   "mark %0 as '%select{final|sealed}1' to silence this warning">;
+def warn_unnecessary_virtual_specifier : Warning<
+  "virtual method %0 is inside a 'final' class and can never be overridden">,
+  InGroup<WarnUnnecessaryVirtualSpecifier>, DefaultIgnore;
 
 // C++11 attributes
 def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;

clang/lib/Sema/SemaDeclCXX.cpp

@@ -7193,10 +7193,18 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
         // class without overriding any.
         if (!M->isStatic())
           DiagnoseHiddenVirtualMethods(M);
-        if (M->hasAttr<OverrideAttr>())
+
+        if (M->hasAttr<OverrideAttr>()) {
           HasMethodWithOverrideControl = true;
-        else if (M->size_overridden_methods() > 0)
+        } else if (M->size_overridden_methods() > 0) {
           HasOverridingMethodWithoutOverrideControl = true;
+        } else {
+          // Warn on newly-declared virtual methods in `final` classes
+          if (M->isVirtualAsWritten() && Record->isEffectivelyFinal()) {
+            Diag(M->getLocation(), diag::warn_unnecessary_virtual_specifier)
+                << M;
+          }
+        }
       }
 
       if (!isa<CXXDestructorDecl>(M))

clang/test/SemaCXX/unnecessary-virtual-specifier.cpp

@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier -Wno-inconsistent-missing-override %s
+
+struct Foo final {
+  Foo() = default;
+  virtual ~Foo() = default;                      // expected-warning {{virtual method}}
+  virtual Foo& operator=(Foo& other) = default;  // expected-warning {{virtual method}}
+  virtual Foo& operator=(Foo&& other) = default; // expected-warning {{virtual method}}
+  void f();
+  virtual void f(int);                           // expected-warning {{virtual method}}
+  int g(int x) { return x; };
+  virtual int g(bool);                           // expected-warning {{virtual method}}
+  static int s();
+};
+
+struct BarBase {
+  virtual ~BarBase() = delete;
+  virtual void virt() {}
+  virtual int  virt2(int);
+  virtual bool virt3(bool);
+  int nonvirt();
+};
+
+struct Bar final : BarBase {
+  ~Bar() override = delete;
+          void virt() override {};
+  virtual int  virt2(int) override;               // `virtual ... override;` is a common pattern, so don't warn
+  virtual bool virt3(bool);                       // Already virtual in the base class; triggers
+                                                  // -Winconsistent-missing-override or -Wsuggest-override instead
+  virtual int  new_virt(bool);                    // expected-warning {{virtual method}}
+  int nonvirt();
+};