[clang-tidy] Fix misplaced fix-its in modernize-use-override

[localspook]

This removes some of the incredibly painful manual lexing in this check, yay!

Fixes #138486. Also fixes the following case with = delete("...") in C++26: https://godbolt.org/z/dddfY15Wq.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Victor Chernyakin (localspook)

Changes

This removes some of the incredibly painful manual lexing in this check, yay!

Fixes #138486. Also fixes the following case with = delete("...") in C++26: https://godbolt.org/z/dddfY15Wq.

---

Full diff: https://github.com/llvm/llvm-project/pull/172196.diff

4 Files Affected:

• (modified) clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp (+9-71)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
• (added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-cxx26.cpp (+11)
• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp (+9-3)

diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
index e2e7bba46f2c4..3bcfd9ad6bc27 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
@@ -141,90 +141,28 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
   if (!FileRange.isValid())
     return;
 
-  // FIXME: Instead of re-lexing and looking for specific macros such as
-  // 'ABSTRACT', properly store the location of 'virtual' and '= 0' in each
-  // FunctionDecl.
+  // FIXME: Instead of re-lexing and looking for the 'virtual' token,
+  // store the location of 'virtual' in each FunctionDecl.
   SmallVector<Token, 16> Tokens = parseTokens(FileRange, Result);
 
   // Add 'override' on inline declarations that don't already have it.
   if (!HasFinal && !HasOverride) {
-    SourceLocation InsertLoc;
-    std::string ReplacementText = (OverrideSpelling + " ").str();
-    const SourceLocation MethodLoc = Method->getLocation();
-
-    for (const Token T : Tokens) {
-      if (T.is(tok::kw___attribute) &&
-          !Sources.isBeforeInTranslationUnit(T.getLocation(), MethodLoc)) {
-        InsertLoc = T.getLocation();
-        break;
-      }
-    }
-
-    if (Method->hasAttrs()) {
-      for (const clang::Attr *A : Method->getAttrs()) {
-        if (!A->isImplicit() && !A->isInherited()) {
-          const SourceLocation Loc =
-              Sources.getExpansionLoc(A->getRange().getBegin());
-          if ((!InsertLoc.isValid() ||
-               Sources.isBeforeInTranslationUnit(Loc, InsertLoc)) &&
-              !Sources.isBeforeInTranslationUnit(Loc, MethodLoc))
-            InsertLoc = Loc;
-        }
-      }
-    }
-
-    if (InsertLoc.isInvalid() && Method->doesThisDeclarationHaveABody() &&
-        Method->getBody() && !Method->isDefaulted()) {
-      // For methods with inline definition, add the override keyword at the
-      // end of the declaration of the function, but prefer to put it on the
-      // same line as the declaration if the beginning brace for the start of
-      // the body falls on the next line.
-      ReplacementText = (" " + OverrideSpelling).str();
-      auto *LastTokenIter = std::prev(Tokens.end());
-      // When try statement is used instead of compound statement as
-      // method body - insert override keyword before it.
-      if (LastTokenIter->is(tok::kw_try))
-        LastTokenIter = std::prev(LastTokenIter);
-      InsertLoc = LastTokenIter->getEndLoc();
-    }
-
-    if (!InsertLoc.isValid()) {
-      // For declarations marked with "= 0" or "= [default|delete]", the end
-      // location will point until after those markings. Therefore, the override
-      // keyword shouldn't be inserted at the end, but before the '='.
-      if (Tokens.size() > 2 &&
-          (getText(Tokens.back(), Sources) == "0" ||
-           Tokens.back().is(tok::kw_default) ||
-           Tokens.back().is(tok::kw_delete)) &&
-          getText(Tokens[Tokens.size() - 2], Sources) == "=") {
-        InsertLoc = Tokens[Tokens.size() - 2].getLocation();
-        // Check if we need to insert a space.
-        if ((Tokens[Tokens.size() - 2].getFlags() & Token::LeadingSpace) == 0)
-          ReplacementText = (" " + OverrideSpelling + " ").str();
-      } else if (getText(Tokens.back(), Sources) == "ABSTRACT")
-        InsertLoc = Tokens.back().getLocation();
-    }
-
-    if (!InsertLoc.isValid()) {
-      InsertLoc = FileRange.getEnd();
-      ReplacementText = (" " + OverrideSpelling).str();
-    }
-
     // If the override macro has been specified just ensure it exists,
     // if not don't apply a fixit but keep the warning.
     if (OverrideSpelling != "override" &&
         !Context.Idents.get(OverrideSpelling).hasMacroDefinition())
       return;
 
-    Diag << FixItHint::CreateInsertion(InsertLoc, ReplacementText);
+    Diag << FixItHint::CreateInsertion(
+        Lexer::getLocForEndOfToken(
+            Method->getTypeSourceInfo()->getTypeLoc().getEndLoc(), 0, Sources,
+            getLangOpts()),
+        (" " + OverrideSpelling).str());
   }
 
-  if (HasFinal && HasOverride && !AllowOverrideAndFinal) {
-    const SourceLocation OverrideLoc =
-        Method->getAttr<OverrideAttr>()->getLocation();
+  if (HasFinal && HasOverride && !AllowOverrideAndFinal)
     Diag << FixItHint::CreateRemoval(
-        CharSourceRange::getTokenRange(OverrideLoc, OverrideLoc));
-  }
+        Method->getAttr<OverrideAttr>()->getLocation());
 
   if (HasVirtual) {
     for (const Token Tok : Tokens) {
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d1fb1cba3e45a..8979e393f8363 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -508,6 +508,11 @@ Changes in existing checks
   on Windows when the check was enabled with a 32-bit :program:`clang-tidy`
   binary.
 
+- Improved :doc:`modernize-use-override
+  <clang-tidy/checks/modernize/use-override>` by fixing an issue where
+  the check would sometimes suggest inserting ``override`` in an invalid
+  place.
+
 - Improved :doc:`modernize-use-scoped-lock
   <clang-tidy/checks/modernize/use-scoped-lock>` check by fixing a crash
   on malformed code (common when using :program:`clang-tidy` through
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-cxx26.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-cxx26.cpp
new file mode 100644
index 0000000000000..e0ced1def9b6a
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-cxx26.cpp
@@ -0,0 +1,11 @@
+// RUN: %check_clang_tidy -std=c++26-or-later %s modernize-use-override,cppcoreguidelines-explicit-virtual-functions %t
+
+struct Base {
+  virtual void f() = delete("");
+};
+
+struct Derived : Base {
+  virtual void f() = delete("");
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual'
+  // CHECK-FIXES: void f() override = delete("");
+};
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp
index 8e19fdd4a0a92..c33b8ae2b24fc 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp
@@ -48,6 +48,8 @@ struct Base {
   virtual void t() throw();
 
   virtual void il(IntPair);
+
+  virtual void u(int x __attribute__((unused))) {}
 };
 
 struct SimpleCases : public Base {
@@ -82,11 +84,11 @@ struct SimpleCases : public Base {
 
   virtual void f()=0;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
-  // CHECK-FIXES: void f() override =0;
+  // CHECK-FIXES: void f() override=0;
 
   virtual void f2() const=0;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
-  // CHECK-FIXES: void f2() const override =0;
+  // CHECK-FIXES: void f2() const override=0;
 
   virtual void g() ABSTRACT;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
@@ -132,6 +134,10 @@ struct SimpleCases : public Base {
   virtual       /*      */ void g2();
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual'
   // CHECK-FIXES: /*      */ void g2() override;
+
+  virtual void u(int x __attribute__((unused)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual'
+  // CHECK-FIXES: void u(int x __attribute__((unused))) override;
 };
 
 // CHECK-MESSAGES-NOT: warning:
@@ -308,7 +314,7 @@ struct MembersOfSpecializations : public Base2 {
   // CHECK-FIXES: void a() override;
 };
 template <> void MembersOfSpecializations<3>::a() {}
-void ff() { MembersOfSpecializations<3>().a(); };
+void ff() { MembersOfSpecializations<3>().a(); }
 
 // In case try statement is used as a method body,
 // make sure that override fix is placed before try keyword.

[zeyi2]

Nit: May intoduce excessive whitespace? Although formatting is the job of clang-format.
(Fine if left unaddressed)

[localspook]

Hmm, I can't think of a case where the added space is undesired; looking through the tests, the resulting formatting looks correct in all the // CHECK-FIXES lines.

[vbvictor]

Lgtm