[clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel599]

Summary

This patch fixes 'Bug 41120'
https://bugs.llvm.org/show_bug.cgi?id=41120
When clang-tidy encounter a fixup name that would become a keyword, it just display a warning with a reason and doesn't attempt to fix it.
Please let me know if this patch is OK, Thanks.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

865

Please don't use auto when type is not spelled in same statement or not iterator.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h

68

Does value really matter?

99

Please use default member initialization and = default;

[Daniel599]

code fixes according to code-review comments

[MyDeveloperDay]

This should really be a full context diff -U999999999

[Daniel599]

added -U999999 to diff cmd

[MyDeveloperDay]

I logged the original bug and I like it!

I think the warning is better than mutating with a prefix, Thank you.

I'll let the code owners approve it, but you have my vote!

[Daniel599]

In D68539#1696864, @MyDeveloperDay wrote:

I logged the original bug and I like it!

I think the warning is better than mutating with a prefix, Thank you.

I'll let the code owners approve it, but you have my vote!

glad to hear it :)
I saw this issue when I submitted mine (https://bugs.llvm.org/show_bug.cgi?id=43306) which for now seems to be a harder fix, still looking for solution there.

[MyDeveloperDay]

I saw this issue when I submitted mine (https://bugs.llvm.org/show_bug.cgi?id=43306) which for now seems to be a harder fix, still looking for a solution there.

oh boy!...are you gonna have to look at every variable/macro in scope and compare?

[Daniel599]

In D68539#1699288, @MyDeveloperDay wrote:

I saw this issue when I submitted mine (https://bugs.llvm.org/show_bug.cgi?id=43306) which for now seems to be a harder fix, still looking for a solution there.

oh boy!...are you gonna have to look at every variable/macro in scope and compare?

Don't know yet, I thought about using Sema::CheckShadow or Sema::LookupName, but I can't find a Sema instance, not sure how those methods effect performance

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

865

Can this be const IdentifierTable&?

868

What if changing it would switch to using a macro instead of a keyword? e.g.,

#define foo 12

void func(int Foo); // Changing Foo to foo would be bad, no?

936

Diagnostics are not full sentences, so the full stop and capitalization are wrong here. I think this should be combined with the diagnostic above using a %select.

auto Diag = diag(Decl.first, "invalid case style for %0 '%1'%select{; cannot be fixed because '%3' would conflict with a keyword|}2")
                  << Failure.KindName << Decl.second << (Failure.FixStatus != ShouldFixStatus::ConflictsWithKeyword) << Failure.Fixup;

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h

69

if -> If

72

Missing a full stop at the end of the comment.

88

This seems a bit confusing to me. It seems like the reason to not generate a fixit is being used to determine whether to diagnose at all, which seems incorrect despite sort of being what the old code did.

89

Drop the parens.

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

16

Please add the newline to the end of the file.

clang/include/clang/Basic/IdentifierTable.h

584

Is this actually needed? Pretty sure you can use std::find(table.begin(), table.end(), Name); at the call site (or something similar).

[Daniel599]

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

868

That's another type of bug, just like the one I found https://bugs.llvm.org/show_bug.cgi?id=43306
I don't aim on solving all of them in one patch, my patch just fixes keywords.
Also, I don't think my patch makes the above situation worse.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h

88

I`m sorry, I didn't understand you.
I tried to keep the old behavior of "cannot fix inside macro" and that's why I needed the method ShouldNotify.
Do you suggest another idea or other code flow?

clang/include/clang/Basic/IdentifierTable.h

584

Yes, because std::find would be slower here compare to HashTable.find

[Daniel599]

code-review fixes

[Daniel599]

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

868

Regarding your comment about macro name, I can fix it using IdentifierInfo::hasMacroDefinition.
Should I fix it in this patch? I`ll add another value to ShouldFixStatus and another error message

[Daniel599]

Added check for macro definition, please re-review the enum ShouldFixStatus as now I use it as a switch-case within the diagnostic message.
Thanks.

[Daniel599]

Hi, any updates regarding my patch? issues to fix?
Thanks.

[aaron.ballman]

LGTM aside from a minor nit.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

868

Thanks for fixing this!

869–873

You can elide the curly braces.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h

88

I see now that the reason we don't diagnose is because the construct is within a macro, which is a pretty reasonable thing to do, so I'm no longer confused. :-)

[Daniel599]

removed curly braces

[aaron.ballman]

Do you need someone to commit this on your behalf?

[Daniel599]

In D68539#1723629, @aaron.ballman wrote:

Do you need someone to commit this on your behalf?

Yes that will be great,
Thank you :)

[aaron.ballman]

Thank you for the patch! I've commit on your behalf in e477988309dbde214a6d16ec690a416882714aac

[Daniel599]

In D68539#1727503, @aaron.ballman wrote:

Thank you for the patch! I've commit on your behalf in e477988309dbde214a6d16ec690a416882714aac

Thank you for the commit,
just a little question. shouldn't the test file clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp be added?

[aaron.ballman]

In D68539#1727759, @Daniel599 wrote:

In D68539#1727503, @aaron.ballman wrote:

Thank you for the patch! I've commit on your behalf in e477988309dbde214a6d16ec690a416882714aac

Thank you for the commit,
just a little question. shouldn't the test file clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp be added?

It absolutely should have been, but I don't see why it didn't get added. I've fixed it in 7b6174bb147957b4695023ae57c95ca07af7b917, thank you for bringing it to my attention!

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h

NamingStyle(llvm::Optional<CaseType> Case, const std::string &Prefix,
                 const std::string &Suffix)
         : Case(Case), Prefix(Prefix), Suffix(Suffix) {}
 
     llvm::Optional<CaseType> Case;
     std::string Prefix;
     std::string Suffix;
   };
 
+  enum class ShouldFixStatus {
+    ShouldFix,
+    InsideMacro, /// If the identifier was used or declared within a macro we
+                 /// won't offer a fixup for safety reasons.
+    ConflictsWithKeyword /// The fixup will conflict with a language keyword, so
+                         /// we can't fix it automatically.
+  };
+
   /// Holds an identifier name check failure, tracking the kind of the
   /// identifer, its possible fixup and the starting locations of all the
   /// identifier usages.
   struct NamingCheckFailure {
     std::string KindName;
     std::string Fixup;
 
     /// Whether the failure should be fixed or not.
     ///
     /// ie: if the identifier was used or declared within a macro we won't offer
     /// a fixup for safety reasons.
-    bool ShouldFix;
+    bool ShouldFix() const { return (FixStatus == ShouldFixStatus::ShouldFix); }
+
+    bool ShouldNotify() const {
+      return FixStatus == ShouldFixStatus::ShouldFix ||
+             FixStatus == ShouldFixStatus::ConflictsWithKeyword;
+    }
+
+    ShouldFixStatus FixStatus = ShouldFixStatus::ShouldFix;
 
     /// A set of all the identifier usages starting SourceLocation, in
     /// their encoded form.
     llvm::DenseSet<unsigned> RawUsageLocs;
 
-    NamingCheckFailure() : ShouldFix(true) {}
+    NamingCheckFailure() = default;
   };
 
   typedef std::pair<SourceLocation, std::string> NamingCheckId;
 
   typedef llvm::DenseMap<NamingCheckId, NamingCheckFailure>
       NamingCheckFailureMap;
 
   /// Check Macros for style violations.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

if (FixLocation.isInvalid())
     return;
 
   // Try to insert the identifier location in the Usages map, and bail out if it
   // is already in there
   auto &Failure = Failures[Decl];
   if (!Failure.RawUsageLocs.insert(FixLocation.getRawEncoding()).second)
     return;
 
-  if (!Failure.ShouldFix)
+  if (!Failure.ShouldFix())
     return;
 
-  Failure.ShouldFix = utils::rangeCanBeFixed(Range, SourceMgr);
+  if (!utils::rangeCanBeFixed(Range, SourceMgr))
+    Failure.FixStatus = IdentifierNamingCheck::ShouldFixStatus::InsideMacro;
 }
 
 /// Convenience method when the usage to be added is a NamedDecl
 static void addUsage(IdentifierNamingCheck::NamingCheckFailureMap &Failures,
                      const NamedDecl *Decl, SourceRange Range,
                      SourceManager *SourceMgr = nullptr) {
   return addUsage(Failures,
                   IdentifierNamingCheck::NamingCheckId(Decl->getLocation(),
if (StringRef(Fixup).equals(Name)) {
       }
     } else {
       NamingCheckFailure &Failure = NamingCheckFailures[NamingCheckId(
           Decl->getLocation(), Decl->getNameAsString())];
       SourceRange Range =
           DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation())
               .getSourceRange();
 
+      const IdentifierTable &Idents = Decl->getASTContext().Idents;
+      auto CheckNewIdentifier = Idents.find(Fixup);
+      if (CheckNewIdentifier != Idents.end() &&
+          CheckNewIdentifier->second->isKeyword(getLangOpts())) {
+        Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;
+      }
+
       Failure.Fixup = std::move(Fixup);
       Failure.KindName = std::move(KindName);
       addUsage(NamingCheckFailures, Decl, Range);
     }
   }
 }
 
 void IdentifierNamingCheck::checkMacro(SourceManager &SourceMgr,
                                        const Token &MacroNameTok,
                                        const MacroInfo *MI) {

 void IdentifierNamingCheck::onEndOfTranslationUnit() {
   for (const auto &Pair : NamingCheckFailures) {
     const NamingCheckId &Decl = Pair.first;
     const NamingCheckFailure &Failure = Pair.second;
 
     if (Failure.KindName.empty())
       continue;
 
-    if (Failure.ShouldFix) {
-      auto Diag = diag(Decl.first, "invalid case style for %0 '%1'")
-                  << Failure.KindName << Decl.second;
+    if (Failure.ShouldNotify()) {
+      auto Diag =
+          diag(Decl.first,
+               "invalid case style for %0 '%1'%select{; cannot be fixed "
+               "because '%3' would conflict with a keyword|}2")
+          << Failure.KindName << Decl.second
+          << (Failure.FixStatus != ShouldFixStatus::ConflictsWithKeyword)
+          << Failure.Fixup;
 
+      if (Failure.ShouldFix()) {
         for (const auto &Loc : Failure.RawUsageLocs) {
-        // We assume that the identifier name is made of one token only. This is
-        // always the case as we ignore usages in macros that could build
+          // We assume that the identifier name is made of one token only. This
+          // is always the case as we ignore usages in macros that could build
           // identifier names by combining multiple tokens.
           //
-        // For destructors, we alread take care of it by remembering the
+          // For destructors, we already take care of it by remembering the
           // location of the start of the identifier and not the start of the
           // tilde.
           //
           // Other multi-token identifiers, such as operators are not checked at
           // all.
           Diag << FixItHint::CreateReplacement(
               SourceRange(SourceLocation::getFromRawEncoding(Loc)),
               Failure.Fixup);
         }
       }
     }
   }
+}
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

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

+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config='{CheckOptions: [ \
+// RUN:     {key: readability-identifier-naming.ParameterCase, value: lower_case} \
+// RUN:   ]}'
+
+int func(int Break) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for parameter 'Break'; cannot be fixed because 'break' would conflict with a keyword
+  // CHECK-FIXES: {{^}}int func(int Break) {{{$}}
+  if (Break == 1) {
+    // CHECK-FIXES: {{^}}  if (Break == 1) {{{$}}
+    return 2;
+  }
+
+  return 0;
+}

clang/include/clang/Basic/IdentifierTable.h

public:
 
   using iterator = HashTableTy::const_iterator;
   using const_iterator = HashTableTy::const_iterator;
 
   iterator begin() const { return HashTable.begin(); }
   iterator end() const   { return HashTable.end(); }
   unsigned size() const  { return HashTable.size(); }
 
+  iterator find(StringRef Name) const { return HashTable.find(Name); }
+
   /// Print some statistics to stderr that indicate how well the
   /// hashing is doing.
   void PrintStats() const;
 
   /// Populate the identifier table with info about the language keywords
   /// for the language specified by \p LangOpts.
   void AddKeywords(const LangOptions &LangOpts);
 };