[clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

[salman-javed-nz]

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=48613.

llvm-header-guard is suggesting header guards with leading underscores
if the header file path begins with a '/' or similar special character.
Only reserved identifiers should begin with an underscore.

[aaron.ballman]

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

28

This fixes some issues but I think it introduces new issues too. The old code would accept Unicode characters and pass them along as (valid) identifier characters. The new code will replace all the Unicode characters with _ which means some people's header guards may go from useful to _______________. We should definitely add test cases for Unicode file names.

Ultimately, I think this functionality will want to use UnicodeCharSets.h to implement something along the lines of what's done in isAllowedInitiallyIDChar() and isAllowedIDChar() from Lexer.cpp.

[whisperity]

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

42–46

Is this true? At least in C++ (and perhaps in C) I believe _foo is a non-reserved identifier, only __foo or _Foo would be reserved.

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

41

valid non-reserved

[aaron.ballman]

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

42–46

_foo is reserved if its an external identifier in both C and C++: https://godbolt.org/z/GnG4v33vK

However, the issue here is that header guards are capitalized, so a file name like _foo still should drop the leading underscore so it doesn't get turned into _FOO which is always reserved.

[salman-javed-nz]

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

28

isAllowed*IDChar()does exactly what I want, but is a static function in Lexer.cpp.
Should I be changing its declaration so that it's exposed via Lexer.h?
I don't know what the bar is to warrant changing core public Clang API.

If I can just call the function directly from llvm-header-guard, I can avoid code duplication.

[aaron.ballman]

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

28

We could expose the interface from Lexer if we feel that's the right approach. I was going back and forth on whether it was the right approach and eventually thought that we don't want to expose it directly because I don't think we want clang-tidy to care about things like LangOpts.AsmPreprocessor or even LangOpts.DollarIdents.

So my recommendation is to put this into LexerUtils.h/.cpp in clang-tidy rather than changing the Clang Lexer interface. WDYT?

[salman-javed-nz]

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

28

(For brevity, when I say isAllowedIDChar, I mean both isAllowedIDChar and isAllowedInitiallyIDChar)

I see two possible directions:

If I expose the interface from Lexer.h, then in Clang-Tidy, it's only a matter of calling
isAllowedIDChar(C, Context->getLangOpts()). Clang-Tidy doesn't need to care about the contents of LangOpts - it just needs to pass them on as-is to the isAllowedIDChar() function.
getLangOpts() is already used all over the Clang-Tidy code base.

The other way is to create a copy of isAllowedIDChar() in the Clang-Tidy project and take out the parts to do with LangOpts.AsmPreprocessor and LangOpts.DollarIdents. Now we have an implementation tailored to just the things Clang-Tidy cares about. I will need to add clangLex to CMakeLists.txt to gain visibility of clang/lib/Lex/UnicodeCharSets.h's character tables.

[salman-javed-nz]

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

28

I need to make a correction to my previous comment (I swear Phabricator had an "edit comment" button but I just don't see it at the moment).

The other way is to create a copy of isAllowedIDChar() in the Clang-Tidy project and take out the parts to do with LangOpts.AsmPreprocessor and LangOpts.DollarIdents. Now we have an implementation tailored to just the things Clang-Tidy cares about. I will need to add clangLex to CMakeLists.txt to gain visibility of clang/lib/Lex/UnicodeCharSets.h's character tables.

Of course clangTidyUtils already depends on clangLex, so we don't need another add_clang_library line in CMakeLists.txt, contrary to what I said earlier. However, UnicodeCharSets.h is still doesn't come up in Clang-Tidy's header search path because it doesn't sit in the include/clang folder. The file will need moving.

[salman-javed-nz]

Back out the changes to the "replace invalid characters in an identifier with underscores" feature.
Making this feature work with Unicode characters on different operating systems significantly increases the amount of code change required.
This can always be revisited in another patch.

In this patch, let's focus on fixing the most pressing problem at hand first: header guards starting with underscores.

[salman-javed-nz]

I reverted my changes to do with the invalid character substitution. Doing something akin to isAllowedInitiallyIDChar() and isAllowedIDChar() in Lexer.cpp will require converting from char* to UTF32*. Windows complicates things by using its own code page (typically Windows-1252). Will require a lot of careful consideration to implement correctly.

I have reverted this part of the patch for now, as it is not mentioned by the PR.

[aaron.ballman]

In D114149#3144614, @salman-javed-nz wrote:

I reverted my changes to do with the invalid character substitution. Doing something akin to isAllowedInitiallyIDChar() and isAllowedIDChar() in Lexer.cpp will require converting from char* to UTF32*. Windows complicates things by using its own code page (typically Windows-1252). Will require a lot of careful consideration to implement correctly.

I have reverted this part of the patch for now, as it is not mentioned by the PR.

I think reverting the changes was a good idea -- we can handle that separately as it is more complex and this solves a real problem. However, I'm not certain that Windows complicates anything here as Clang accepts UTF-8 input. We have -finput-charset but that only supports UTF-8 currently.

LGTM, modulo a commenting nit.

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

169

This wasn't actually a typo -- I think it meant "if and only if" (https://en.wikipedia.org/wiki/If_and_only_if). Feel free to expand the comment instead of using iff.

[salman-javed-nz]

Update "iff" comment based on review.

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

HeaderGuardCheck(StringRef Name, ClangTidyContext *Context)
     utils::parseFileExtensions(RawStringHeaderFileExtensions,
                                HeaderFileExtensions,
                                utils::defaultFileExtensionDelimiters());
   }
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;
 
+  /// Ensure that the provided header guard is a non-reserved identifier.
+  std::string sanitizeHeaderGuard(StringRef Guard);
+
   /// Returns ``true`` if the check should suggest inserting a trailing comment
   /// on the ``#endif`` of the header guard. It will use the same name as
   /// returned by ``HeaderGuardCheck::getHeaderGuard``.
   virtual bool shouldSuggestEndifComment(StringRef Filename);
   /// Returns ``true`` if the check should suggest changing an existing header
   /// guard to the string returned by ``HeaderGuardCheck::getHeaderGuard``.
   virtual bool shouldFixHeaderGuard(StringRef Filename);
   /// Returns ``true`` if the check should add a header guard to the file

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

 /// canonicalize a path by removing ./ and ../ components.
 static std::string cleanPath(StringRef Path) {
   SmallString<256> Result = Path;
   llvm::sys::path::remove_dots(Result, true);
   return std::string(Result.str());
 }
 
 namespace {
 class HeaderGuardPPCallbacks : public PPCallbacks {
 public:
   HeaderGuardPPCallbacks(Preprocessor *PP, HeaderGuardCheck *Check)
       : PP(PP), Check(Check) {}
 
   void FileChanged(SourceLocation Loc, FileChangeReason Reason,
                    SrcMgr::CharacteristicKind FileType,
                    FileID PrevFID) override {
     // Record all files we enter. We'll need them to diagnose headers without
     // guards.
     SourceManager &SM = PP->getSourceManager();
     if (Reason == EnterFile && FileType == SrcMgr::C_User) {
       if (const FileEntry *FE = SM.getFileEntryForID(SM.getFileID(Loc))) {
         std::string FileName = cleanPath(FE->getName());
         Files[FileName] = FE;
       }
     }
   }
 
   void Ifndef(SourceLocation Loc, const Token &MacroNameTok,
               const MacroDefinition &MD) override {
     if (MD)
       return;
 
     // Record #ifndefs that succeeded. We also need the Location of the Name.
     Ifndefs[MacroNameTok.getIdentifierInfo()] =
         std::make_pair(Loc, MacroNameTok.getLocation());
public:
   /// change was made.
   std::string checkHeaderGuardDefinition(SourceLocation Ifndef,
                                          SourceLocation Define,
                                          SourceLocation EndIf,
                                          StringRef FileName,
                                          StringRef CurHeaderGuard,
                                          std::vector<FixItHint> &FixIts) {
     std::string CPPVar = Check->getHeaderGuard(FileName, CurHeaderGuard);
+    CPPVar = Check->sanitizeHeaderGuard(CPPVar);
     std::string CPPVarUnder = CPPVar + '_';
 
-    // Allow a trailing underscore iff we don't have to change the endif comment
-    // too.
+    // Allow a trailing underscore if and only if we don't have to change the
+    // endif comment too.
     if (Ifndef.isValid() && CurHeaderGuard != CPPVar &&
         (CurHeaderGuard != CPPVarUnder ||
          wouldFixEndifComment(FileName, EndIf, CurHeaderGuard))) {
       FixIts.push_back(FixItHint::CreateReplacement(
           CharSourceRange::getTokenRange(
               Ifndef, Ifndef.getLocWithOffset(CurHeaderGuard.size())),
           CPPVar));
       FixIts.push_back(FixItHint::CreateReplacement(
for (const auto &FE : Files) {
 
       SourceManager &SM = PP->getSourceManager();
       FileID FID = SM.translateFile(FE.getValue());
       SourceLocation StartLoc = SM.getLocForStartOfFile(FID);
       if (StartLoc.isInvalid())
         continue;
 
       std::string CPPVar = Check->getHeaderGuard(FileName);
+      CPPVar = Check->sanitizeHeaderGuard(CPPVar);
       std::string CPPVarUnder = CPPVar + '_'; // Allow a trailing underscore.
       // If there's a macro with a name that follows the header guard convention
       // but was not recognized by the preprocessor as a header guard there must
       // be code outside of the guarded area. Emit a plain warning without
       // fix-its.
       // FIXME: Can we move it into the right spot?
       bool SeenMacro = false;
       for (const auto &MacroEntry : Macros) {

 }
 
 void HeaderGuardCheck::registerPPCallbacks(const SourceManager &SM,
                                            Preprocessor *PP,
                                            Preprocessor *ModuleExpanderPP) {
   PP->addPPCallbacks(std::make_unique<HeaderGuardPPCallbacks>(PP, this));
 }
 
+std::string HeaderGuardCheck::sanitizeHeaderGuard(StringRef Guard) {
+  // Only reserved identifiers are allowed to start with an '_'.
+  return Guard.drop_while([](char C) { return C == '_'; }).str();
+}
+
 bool HeaderGuardCheck::shouldSuggestEndifComment(StringRef FileName) {
   return utils::isFileExtension(FileName, HeaderFileExtensions);
 }
 
 bool HeaderGuardCheck::shouldFixHeaderGuard(StringRef FileName) { return true; }
 
 bool HeaderGuardCheck::shouldSuggestToAddHeaderGuard(StringRef FileName) {
   return utils::isFileExtension(FileName, HeaderFileExtensions);
 }
 
 std::string HeaderGuardCheck::formatEndIf(StringRef HeaderGuard) {
   return "endif // " + HeaderGuard.str();
 }
 } // namespace utils
 } // namespace tidy
 } // namespace clang

clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp

EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
             "#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
             "\n"
             "\n"
             "#endif\n",
             runHeaderGuardCheck(
                 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
                 StringRef("header is missing header guard")));
 
+  // Substitution of characters should not result in a header guard starting
+  // with "_".
+  EXPECT_EQ("#ifndef BAR_H\n"
+            "#define BAR_H\n"
+            "\n"
+            "\n"
+            "#endif\n",
+            runHeaderGuardCheck("", "include/--bar.h",
+                                StringRef("header is missing header guard")));
+
 #ifdef WIN32
   // Check interaction with Windows-style path separators (\).
   EXPECT_EQ(
       "#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
       "#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
       "\n"
       "\n"
       "#endif\n",