[clang-tidy] Fix RenamerClangTidy checks trying to emit a fix that isnt a valid identifier

[njames93]

Summary

Addresses https://bugs.llvm.org/show_bug.cgi?id=48230.
Handle the case when the Fixup suggested isn't a valid c/c++ identifer.

[gribozavr2]

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp

526

I don't think we need to tell that to the user. When Clang can't provide a fix, it just silently omits it. It does not help the user to know. that Clang tried, but failed.

(This message could be also read as "this code is so bad it can't be fixed"...)

[njames93]

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp

526

So just return an empty string.
The user will see the normal diagnostic message but there will be no fix-its available.

[gribozavr2]

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp

526

Agreed.

[njames93]

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp

526

Maybe a slightly better idea is to use the same message as the empty fix up case
; cannot be fixed automatically. WDYT?

[gribozavr2]

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp

526

Oh I see, this checker has a number of messages like that... Well, feel free to keep it, but what seems weird to me as a user is that for an input name of "_0Bad" the checker automatically chose somehow "0_Bad" (why?), and then concluded that it is not a good choice. This looks like irrelevant information because the new name is weird. I can totally see how reminding the user that "Break"->"break" is not easily possible because the latter is a keyword.

Feel free to ignore me since this checker already produces messages like this.

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h

enum class ShouldFixStatus {
     /// so we can't fix it automatically.
     ConflictsWithKeyword,
 
     /// The fixup will conflict with a macro
     /// definition, so we can't fix it
     /// automatically.
     ConflictsWithMacroDefinition,
 
+    /// The fixup results in an identifier that is not a valid c/c++ identifier.
+    FixInvalidIdentifier,
+
     /// Values pass this threshold will be ignored completely
     /// i.e no message, no fixup.
     IgnoreFailureThreshold,
 
     /// If the identifier was used or declared within a macro we
     /// won't offer a fixup for safety reasons.
     InsideMacro,
   };

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp

 //===--- RenamerClangTidyCheck.cpp - clang-tidy ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "RenamerClangTidyCheck.h"
 #include "ASTUtils.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/CharInfo.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/PointerIntPair.h"
 
 #define DEBUG_TYPE "clang-tidy"
 
if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
     const IdentifierTable &Idents = Decl->getASTContext().Idents;
     auto CheckNewIdentifier = Idents.find(Info.Fixup);
     if (CheckNewIdentifier != Idents.end()) {
       const IdentifierInfo *Ident = CheckNewIdentifier->second;
       if (Ident->isKeyword(getLangOpts()))
         Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;
       else if (Ident->hasMacroDefinition())
         Failure.FixStatus = ShouldFixStatus::ConflictsWithMacroDefinition;
+    } else if (!isValidIdentifier(Info.Fixup)) {
+      Failure.FixStatus = ShouldFixStatus::FixInvalidIdentifier;
     }
 
     Failure.Info = std::move(Info);
     addUsage(Decl, Range);
   }
 }
 
 void RenamerClangTidyCheck::checkMacro(SourceManager &SourceMgr,
void RenamerClangTidyCheck::expandMacro(const Token &MacroNameTok,
 
   SourceRange Range(MacroNameTok.getLocation(), MacroNameTok.getEndLoc());
   addUsage(ID, Range);
 }
 
 static std::string
 getDiagnosticSuffix(const RenamerClangTidyCheck::ShouldFixStatus FixStatus,
                     const std::string &Fixup) {
-  if (Fixup.empty())
+  if (Fixup.empty() ||
+      FixStatus == RenamerClangTidyCheck::ShouldFixStatus::FixInvalidIdentifier)
     return "; cannot be fixed automatically";
   if (FixStatus == RenamerClangTidyCheck::ShouldFixStatus::ShouldFix)
     return {};
   if (FixStatus >=
       RenamerClangTidyCheck::ShouldFixStatus::IgnoreFailureThreshold)
     return {};
   if (FixStatus == RenamerClangTidyCheck::ShouldFixStatus::ConflictsWithKeyword)
     return "; cannot be fixed because '" + Fixup +
            "' would conflict with a keyword";
   if (FixStatus ==
       RenamerClangTidyCheck::ShouldFixStatus::ConflictsWithMacroDefinition)
     return "; cannot be fixed because '" + Fixup +
            "' would conflict with a macro definition";
-
   llvm_unreachable("invalid ShouldFixStatus");
 }
 
 void RenamerClangTidyCheck::onEndOfTranslationUnit() {
   for (const auto &Pair : NamingCheckFailures) {
     const NamingCheckId &Decl = Pair.first;
     const NamingCheckFailure &Failure = Pair.second;
 
     if (Failure.Info.KindName.empty())
       continue;
 

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-bugfix-name-conflicts.cpp

int func2(int Foo) {
   // CHECK-FIXES: {{^}}int func2(int Foo) {{{$}}
   if (Foo == 1) {
     // CHECK-FIXES: {{^}}  if (Foo == 1) {{{$}}
     return 2;
   }
 
   return 0;
 }
+
+int func3(int _0Bad) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for parameter '_0Bad'; cannot be fixed automatically [readability-identifier-naming]
+  // CHECK-FIXES: {{^}}int func3(int _0Bad) {{{$}}
+  if (_0Bad == 1) {
+    // CHECK-FIXES: {{^}}  if (_0Bad == 1) {{{$}}
+    return 2;
+  }
+  return 0;
+}