[clang-tidy] Header guard check can skip past license comment

[njames93]

Summary

Add an option for header guard derived checks to control whether to skip past a license comment when adding a guard to file currently missing one.
Whats identified as a license comment is a block of comments right at the start of a file that is followed by an empty line.

All these examples are being interpreted as the first tokens in a file.

// This is identified as a license comment.
// It can span multiple lines too.

// This is not part of the license comment as its detached.

// This is not identified as a license comment as the
// block is followed by code.
void foo();

// This is identified as a license comment as its followed by
// an empty line

void foo();

[njames93]

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

313–316

clang-format(trunk) says what I submitted was correct, clang-format-10 is suggesting the change. Which do I trust??

[njames93]

Added /**/ style license comment test cases

[aaron.ballman]

 // This is not identified as a license comment as the
// block is followed by code.
void foo();

FWIW: https://github.com/GrammaTech/gtirb-pprinter/blob/master/include/gtirb_pprinter/AttPrettyPrinter.hpp or https://github.com/GrammaTech/gtirb/blob/master/include/gtirb/AuxData.hpp (so there are projects which do not put a newline between the license and code).

clang-tools-extra/clang-tidy/ClangTidyCheck.h

48

It makes me sad how much we're having to sprinkle std::move() calls around (I find the calls to move() to be more distracting that declaring the parameter types to be const std::string&.) Were these found mechanically?

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

313–316

Either is defensible, I'm fine with using what trunk says.

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

12

Unrelated to this patch, but neat to know that there is zero testing of this on Windows.

224

You seem to be missing tests that show the license and header guards are both correct and found no issues.

[njames93]

In D83223#2147072, @aaron.ballman wrote:

 // This is not identified as a license comment as the
// block is followed by code.
void foo();

FWIW: https://github.com/GrammaTech/gtirb-pprinter/blob/master/include/gtirb_pprinter/AttPrettyPrinter.hpp or https://github.com/GrammaTech/gtirb/blob/master/include/gtirb/AuxData.hpp (so there are projects which do not put a newline between the license and code).

Short of creating an AI that understands context it won't be possible to determine the difference between license and general documentation, in any case I feel this heuristic is the safest way to ensure good coverage with minimised risk of inserting the guard in the middle of documentation,

clang-tools-extra/clang-tidy/ClangTidyCheck.h

48

These are me screwing around with my branches and will be removed

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

224

That wouldn't be testing new behaviour added here. The check can already find header guards when there is a license, The code I have added only affects when no guard is found and it needs to add one. I can still add those cases for piece of mind.

[njames93]

• Added expect no changes test

[aaron.ballman]

In D83223#2147247, @njames93 wrote:

In D83223#2147072, @aaron.ballman wrote:

 // This is not identified as a license comment as the
// block is followed by code.
void foo();

FWIW: https://github.com/GrammaTech/gtirb-pprinter/blob/master/include/gtirb_pprinter/AttPrettyPrinter.hpp or https://github.com/GrammaTech/gtirb/blob/master/include/gtirb/AuxData.hpp (so there are projects which do not put a newline between the license and code).

Short of creating an AI that understands context it won't be possible to determine the difference between license and general documentation, in any case I feel this heuristic is the safest way to ensure good coverage with minimised risk of inserting the guard in the middle of documentation,

My instinct is that we shouldn't be trying to play those games in the first place and should consider *all* leading comments and empty (whitespace-only) lines as part of the "license" and expect the first significant token to be the header guard. e.g., this isn't about the license at all, it's about whether you can have prose before the header guard or not. It's not uncommon for projects to put prose before header guards, nor is it uncommon for it to go after the header guards. tbh, that feels a bit like an option for the feature rather than an automatic behavior because I could also see a project wanting to enforce a consistent style.

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

224

I'd appreciate the test coverage because it's not always immediately clear why some of the new tests are expected to fail.

[alexfh]

In D83223#2147306, @aaron.ballman wrote:

In D83223#2147247, @njames93 wrote:

In D83223#2147072, @aaron.ballman wrote:

 // This is not identified as a license comment as the
// block is followed by code.
void foo();

FWIW: https://github.com/GrammaTech/gtirb-pprinter/blob/master/include/gtirb_pprinter/AttPrettyPrinter.hpp or https://github.com/GrammaTech/gtirb/blob/master/include/gtirb/AuxData.hpp (so there are projects which do not put a newline between the license and code).

Short of creating an AI that understands context it won't be possible to determine the difference between license and general documentation, in any case I feel this heuristic is the safest way to ensure good coverage with minimised risk of inserting the guard in the middle of documentation,

My instinct is that we shouldn't be trying to play those games in the first place and should consider *all* leading comments and empty (whitespace-only) lines as part of the "license" and expect the first significant token to be the header guard. e.g., this isn't about the license at all, it's about whether you can have prose before the header guard or not. It's not uncommon for projects to put prose before header guards, nor is it uncommon for it to go after the header guards. tbh, that feels a bit like an option for the feature rather than an automatic behavior because I could also see a project wanting to enforce a consistent style.

A drive-by note: I'm also for treating all leading comments equally (and referring to them as just "comments", not "license comments").

clang-tools-extra/clang-tidy/ClangTidyCheck.h

 
 public:
   void log(raw_ostream &OS) const override { OS << this->message(); }
 };
 
 class MissingOptionError : public OptionError<MissingOptionError> {
 public:
   explicit MissingOptionError(std::string OptionName)
-      : OptionName(OptionName) {}
+      : OptionName(std::move(OptionName)) {}
 
   std::string message() const override;
   static char ID;
 private:
   const std::string OptionName;
 };
 
 class UnparseableEnumOptionError
     : public OptionError<UnparseableEnumOptionError> {
 public:
   explicit UnparseableEnumOptionError(std::string LookupName,
                                       std::string LookupValue)
-      : LookupName(LookupName), LookupValue(LookupValue) {}
+      : LookupName(std::move(LookupName)), LookupValue(std::move(LookupValue)) {
+  }
   explicit UnparseableEnumOptionError(std::string LookupName,
                                       std::string LookupValue,
                                       std::string SuggestedValue)
-      : LookupName(LookupName), LookupValue(LookupValue),
-        SuggestedValue(SuggestedValue) {}
+      : LookupName(std::move(LookupName)), LookupValue(std::move(LookupValue)),
+        SuggestedValue(std::move(SuggestedValue)) {}
 
   std::string message() const override;
   static char ID;
 
 private:
   const std::string LookupName;
   const std::string LookupValue;
   const llvm::Optional<std::string> SuggestedValue;
 };
 
 class UnparseableIntegerOptionError
     : public OptionError<UnparseableIntegerOptionError> {
 public:
   explicit UnparseableIntegerOptionError(std::string LookupName,
                                          std::string LookupValue,
                                          bool IsBoolean = false)
-      : LookupName(LookupName), LookupValue(LookupValue), IsBoolean(IsBoolean) {
-  }
+      : LookupName(std::move(LookupName)), LookupValue(std::move(LookupValue)),
+        IsBoolean(IsBoolean) {}
 
   std::string message() const override;
   static char ID;
 
 private:
   const std::string LookupName;
   const std::string LookupValue;
   const bool IsBoolean;

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

public:
   /// 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
   /// if it has none.
   virtual bool shouldSuggestToAddHeaderGuard(StringRef Filename);
+  /// Returns ``true`` if the check should skip past a block comment at the
+  /// start of a file when trying to add a header guard.
+  virtual bool shouldSkipLicenseComments(StringRef Filename);
   /// Returns a replacement for the ``#endif`` line with a comment mentioning
   /// \p HeaderGuard. The replacement should start with ``endif``.
   virtual std::string formatEndIf(StringRef HeaderGuard);
   /// Gets the canonical header guard for a file.
   virtual std::string getHeaderGuard(StringRef Filename,
                                      StringRef OldGuard = StringRef()) = 0;
 
 private:

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

void checkEndifComment(StringRef FileName, SourceLocation EndIf,
     if (wouldFixEndifComment(FileName, EndIf, HeaderGuard, &EndIfLen)) {
       FixIts.push_back(FixItHint::CreateReplacement(
           CharSourceRange::getCharRange(EndIf,
                                         EndIf.getLocWithOffset(EndIfLen)),
           Check->formatEndIf(HeaderGuard)));
     }
   }
 
+  enum class NewLineInsertions { None, One, Two };
+
+  /// Gets the first SourceLocation in a file after stripping the first block of
+  /// comments if its not attached to code, as this is likely a license. The
+  /// second item returned is the number of new lines that will need to be
+  /// inserted when creating the header guard.
+  std::pair<SourceLocation, NewLineInsertions>
+  getLocAfterLicense(FileID File, const Preprocessor *PP) {
+    const SourceManager &SM = PP->getSourceManager();
+    SourceLocation StartLoc = SM.getLocForStartOfFile(File);
+    bool Invalid = false;
+    StringRef Buffer = SM.getBufferData(File, &Invalid);
+    if (Invalid || Buffer.empty() || isWhitespace(Buffer.front()))
+      return std::make_pair(StartLoc, NewLineInsertions::None);
+
+    // Create a lexer starting at the beginning of this token.
+    Lexer TheLexer(StartLoc, PP->getLangOpts(), Buffer.begin(), Buffer.data(),
+                   Buffer.end());
+    TheLexer.SetCommentRetentionState(true);
+    Token Cur;
+    if (TheLexer.LexFromRawLexer(Cur)) {
+      // Only one token in stream. If its a comment, presume its a license and
+      // return the location following the comment, otherwise return start of
+      // file.
+      if (Cur.is(tok::comment))
+        return std::make_pair(TheLexer.getSourceLocation(),
+                              NewLineInsertions::Two);
+      return std::make_pair(StartLoc, NewLineInsertions::None);
+    }
+    auto HasTwoTrailingNewLines = [&] {
+      StringRef Next = Buffer.drop_front(TheLexer.getCurrentBufferOffset());
+      StringRef WS = Next.take_while(isWhitespace);
+      return WS.count('\n') > 1;
+    };
+    if (HasTwoTrailingNewLines()) {
+      // Return the start of the next token.
+      TheLexer.LexFromRawLexer(Cur);
+      return std::make_pair(Cur.getLocation(), NewLineInsertions::None);
+    }
+    while (true) {
+      if (TheLexer.LexFromRawLexer(Cur)) {
+        if (Cur.is(tok::comment))
+          return std::make_pair(TheLexer.getSourceLocation(),
+                                NewLineInsertions::Two);
+        if (Cur.is(tok::eof))
+          return std::make_pair(TheLexer.getSourceLocation(),
+                                NewLineInsertions::One);
+        return std::make_pair(StartLoc, NewLineInsertions::None);
+      }
+      if (HasTwoTrailingNewLines()) {
+        TheLexer.LexFromRawLexer(Cur);
+        return std::make_pair(Cur.getLocation(), NewLineInsertions::None);
+      }
+      if (Cur.is(tok::eof))
+        return std::make_pair(TheLexer.getSourceLocation(),
+                              NewLineInsertions::One);
+      if (Cur.isNot(tok::comment))
+        return std::make_pair(StartLoc, NewLineInsertions::None);
+    }
+  }
+
   /// Looks for files that were visited but didn't have a header guard.
   /// Emits a warning with fixits suggesting adding one.
   void checkGuardlessHeaders() {
     // Look for header files that didn't have a header guard. Emit a warning and
     // fix-its to add the guard.
-    // TODO: Insert the guard after top comments.
     for (const auto &FE : Files) {
       StringRef FileName = FE.getKey();
       if (!Check->shouldSuggestToAddHeaderGuard(FileName))
         continue;
 
       SourceManager &SM = PP->getSourceManager();
       FileID FID = SM.translateFile(FE.getValue());
-      SourceLocation StartLoc = SM.getLocForStartOfFile(FID);
+      SourceLocation StartLoc;
+      NewLineInsertions AddNewLine;
+      if (Check->shouldSkipLicenseComments(FileName)) {
+        std::tie(StartLoc, AddNewLine) = getLocAfterLicense(FID, PP);
+      } else {
+        StartLoc = SM.getLocForStartOfFile(FID);
+        AddNewLine = NewLineInsertions::None;
+      }
+
       if (StartLoc.isInvalid())
         continue;
 
       std::string CPPVar = Check->getHeaderGuard(FileName);
       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
for (const auto &FE : Files) {
                                  "header guard; consider moving it");
           SeenMacro = true;
           break;
         }
       }
 
       if (SeenMacro)
         continue;
-
+      StringRef Opening = AddNewLine == NewLineInsertions::None ? "#ifndef "
+                          : AddNewLine == NewLineInsertions::One
+                              ? "\n#ifndef "
+                              : "\n\n#ifndef ";
       Check->diag(StartLoc, "header is missing header guard")
           << FixItHint::CreateInsertion(
-                 StartLoc, "#ifndef " + CPPVar + "\n#define " + CPPVar + "\n\n")
+                 StartLoc,
+                 (Opening + CPPVar + "\n#define " + CPPVar + "\n\n").str())
           << FixItHint::CreateInsertion(
                  SM.getLocForEndOfFile(FID),
                  Check->shouldSuggestEndifComment(FileName)
                      ? "\n#" + Check->formatEndIf(CPPVar) + "\n"
                      : "\n#endif\n");
     }
   }
 

 }
 
 bool HeaderGuardCheck::shouldFixHeaderGuard(StringRef FileName) { return true; }
 
 bool HeaderGuardCheck::shouldSuggestToAddHeaderGuard(StringRef FileName) {
   return utils::isFileExtension(FileName, HeaderFileExtensions);
 }
 
+bool HeaderGuardCheck::shouldSkipLicenseComments(StringRef FileName) {
+  return true;
+}
+
 std::string HeaderGuardCheck::formatEndIf(StringRef HeaderGuard) {
   return "endif // " + HeaderGuard.str();
 }
 } // namespace utils
 } // namespace tidy
 } // namespace clang

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

 #include "ClangTidyTest.h"
 #include "llvm/HeaderGuardCheck.h"
 #include "llvm/IncludeOrderCheck.h"
 #include "gtest/gtest.h"
 
 using namespace clang::tidy::llvm_check;
 
 namespace clang {
 namespace tidy {
 namespace test {
 
 // FIXME: It seems this might be incompatible to dos path. Investigating.
 #if !defined(_WIN32)
 static std::string runHeaderGuardCheck(StringRef Code, const Twine &Filename,
                                        Optional<StringRef> ExpectedWarning) {
   std::vector<ClangTidyError> Errors;
   std::string Result = test::runCheckOnCode<LLVMHeaderGuardCheck>(
       Code, &Errors, Filename, std::string("-xc++-header"));
   if (Errors.size() != (size_t)ExpectedWarning.hasValue())
     return "invalid error count";
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")));
 }
+
+TEST(LLVMHeaderGuardCheckTest, FixHeaderGuardsWithComments) {
+  EXPECT_EQ("// LicenseText //\n"
+            "\n"
+            "#ifndef LLVM_CLANG_BAR_H\n"
+            "#define LLVM_CLANG_BAR_H\n"
+            "\n"
+            "\n"
+            "#endif\n",
+            runHeaderGuardCheck("// LicenseText //", "include/clang/bar.h",
+                                StringRef("header is missing header guard")));
+
+  EXPECT_EQ("// LicenseText //\n"
+            "\n"
+            "#ifndef LLVM_CLANG_BAR_H\n"
+            "#define LLVM_CLANG_BAR_H\n"
+            "\n"
+            "\n"
+            "#endif\n",
+            runHeaderGuardCheck("// LicenseText //\n", "include/clang/bar.h",
+                                StringRef("header is missing header guard")));
+  EXPECT_EQ("// LicenseText //\n"
+            "// SecondLine  //\n"
+            "// ThirdLine   //\n"
+            "\n"
+            "#ifndef LLVM_CLANG_BAR_H\n"
+            "#define LLVM_CLANG_BAR_H\n"
+            "\n"
+            "\n"
+            "#endif\n",
+            runHeaderGuardCheck("// LicenseText //\n"
+                                "// SecondLine  //\n"
+                                "// ThirdLine   //\n",
+                                "include/clang/bar.h",
+                                StringRef("header is missing header guard")));
+  EXPECT_EQ("// LicenseText //\n"
+            "// SecondLine  //\n"
+            "// ThirdLine   //\n"
+            "\n"
+            "#ifndef LLVM_CLANG_BAR_H\n"
+            "#define LLVM_CLANG_BAR_H\n"
+            "\n"
+            "\n"
+            "#endif\n",
+            runHeaderGuardCheck("// LicenseText //\n"
+                                "// SecondLine  //\n"
+                                "// ThirdLine   //",
+                                "include/clang/bar.h",
+                                StringRef("header is missing header guard")));
+
+  EXPECT_EQ("// LicenseText //\n"
+            "// SecondLine  //\n"
+            "// ThirdLine   //\n"
+            "\n"
+            "#ifndef LLVM_CLANG_BAR_H\n"
+            "#define LLVM_CLANG_BAR_H\n"
+            "\n"
+            "// FunctionDoc\n"
+            "void Foo();\n"
+            "\n"
+            "#endif\n",
+            runHeaderGuardCheck("// LicenseText //\n"
+                                "// SecondLine  //\n"
+                                "// ThirdLine   //\n"
+                                "\n"
+                                "// FunctionDoc\n"
+                                "void Foo();\n",
+                                "include/clang/bar.h",
+                                StringRef("header is missing header guard")));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_BAR_H\n"
+            "#define LLVM_CLANG_BAR_H\n"
+            "\n"
+            "// Function Doc\n"
+            "void foo();\n"
+            "\n"
+            "#endif\n",
+            runHeaderGuardCheck("// Function Doc\n"
+                                "void foo();\n",
+                                "include/clang/bar.h",
+                                StringRef("header is missing header guard")));
+
+  EXPECT_EQ("/* LicenseText \n"
+            "   SecondLine  \n"
+            "   ThirdLine */\n"
+            "\n"
+            "#ifndef LLVM_CLANG_BAR_H\n"
+            "#define LLVM_CLANG_BAR_H\n"
+            "\n"
+            "\n"
+            "#endif\n",
+            runHeaderGuardCheck("/* LicenseText \n"
+                                "   SecondLine  \n"
+                                "   ThirdLine */\n",
+                                "include/clang/bar.h",
+                                StringRef("header is missing header guard")));
+
+  EXPECT_EQ("/* LicenseText \n"
+            "   SecondLine  \n"
+            "   ThirdLine */\n"
+            "\n"
+            "#ifndef LLVM_CLANG_BAR_H\n"
+            "#define LLVM_CLANG_BAR_H\n"
+            "\n"
+            "// FunctionDoc\n"
+            "void Foo();\n"
+            "\n"
+            "#endif\n",
+            runHeaderGuardCheck("/* LicenseText \n"
+                                "   SecondLine  \n"
+                                "   ThirdLine */\n"
+                                "\n"
+                                "// FunctionDoc\n"
+                                "void Foo();\n",
+                                "include/clang/bar.h",
+                                StringRef("header is missing header guard")));
+}
 #endif
 
 } // namespace test
 } // namespace tidy
 } // namespace clang