[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?

940

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

     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.

     ///
     /// 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;

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

   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

           DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation())
               .getSourceRange();
 
+      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);

     if (Failure.KindName.empty())
       continue;
 
-    if (Failure.ShouldFix) {
-      auto Diag = diag(Decl.first, "invalid case style for %0 '%1'")
+    if (Failure.ShouldNotify()) {
+      auto Diag = diag(Decl.first, "invalid case style for %0 '%1'%2")
                   << Failure.KindName << Decl.second;
 
-      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
-        // identifier names by combining multiple tokens.
-        //
-        // For destructors, we alread 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);
+      if (Failure.FixStatus == ShouldFixStatus::ConflictsWithKeyword) {
+        std::string CannotFixReason =
+            std::string(". Cannot be fixed because '") + Failure.Fixup +
+            "' would conflict with a language keyword";
+        Diag << CannotFixReason;
+      } else {
+        Diag << "";
+      }
+
+      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
+          // identifier names by combining multiple tokens.
+          //
+          // 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);
+        }
       }
     }
   }

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 language keyword
+  // CHECK-FIXES: {{^}}int func(int Break) {{{$}}
+  if (Break == 1) {
+    // CHECK-FIXES: {{^}}  if (Break == 1) {{{$}}
+    return 2;
+  }
+
+  return 0;
+}
 No newline at end of file

clang/include/clang/Basic/IdentifierTable.h

   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;