[clang-tidy] Add support for new TODO style to google-readability-todo

[ymandel]

Summary

Google style is changing to encourage a different (and more common) form of
TODO, specifically: TODO: link - description. This patch adds support for the
new style, while maintaining backwards compatability with the old style. The
patch adds a configuration option to the check to control which style (V1 or V2)
is checked.

[njames93]

clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp

97

Given this check is part of the google module, doesn't it make sense to have the default as what google now recommends.

clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst

6

Maybe update this line to something along with lines of
Finds TODO comments not conforming to googles style guidelines

16–18

This shouldn't appear in the options, as it isn't read from the CheckOptions

22

It would be good to specify the default value of this option(or just the default behaviour of the check)

[ymandel]

Address reviewer comments.

[ymandel]

Thank you for the very fast review!

clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp

97

The style guide hasn't been updated yet. Once that happens, I think we should switch the default. I can add a FIXME -- what do you think?

[sammccall]

I'd be more comfortable reviewing this after it's actually been added to the style guide, it's hard to refer to non-public docs and discussions here.

I don't really understand the rationale for the hard split into two modes, vs always accepting both styles and maybe just making the fixit style configurable.
(OTOH If we *really* want to support exclusive use of the old style, why are we not supporting exclusive use of the new style?)

clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp

18

I'd really prefer if we could give these names rather than numbers. Best I can come up with is paren-style/hyphen style though.

Either way, we can drop "version" as it's redundant with "style".
(Unless you're using it to refer to the version of the style *guide*/check logic in which case it's confusing to also use it to refer to the style of the comment, as these aren't 1:1)

79

I find this part very confusing. We have two separate implementations that check whether this is a valid TODO(context): description comment, even though that's acceptable in all modes. Why?

It seems like the logic should look either like:

if (!isAnyTODOComment) // regex #1
  return;
if (isValidParenStyleTODO) // regex #2
  return;
if (allowNewStyle && isValidHyphenStyleTODO) // regex #3
  return;
diag(...) << Fix; // suggestion based on preferred style and parse from isAnyTODOComment

or

if (!matchesComplicatedAllEncompassingRegex)
  return;
if (submatchesIndicateValidParenStyleTODO)
  return;
if (submatchesIndicateValidHyphenStyleTODO)
  return;
// diagnose and suggest fix

85

this isn't accurate: TODO(username): description is still valid.
Maybe "should have"?

clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst

16

This is a confusing option name.

It's not clear what "V2Style" is, it's non-descriptive and the style guide won't use that term.
It's not clear what "Use" means - in fixes? Only accept? My code uses V2 style?

Based on the current behavior, I'd suggest "PreferHyphenStyle"

32

I think trying to enforce a new style via a check that doesn't suggest or even describe the style is a mistake.

Given TODO: text, text is almost certainly a description - creating an appropriate link is a chore, but IME ~nobody forgets to actually write what they want to do.
If the suggestion is wrong, people can fix it. The most important thing is we tell them what the expected format is.

clang-tools-extra/test/clang-tidy/checkers/google/readability-todo-v2.cpp

21

(surely this example shows how absurd this style is...)

24

"supported for backwards compatibility" is stronger than the text I've seen, which simply says they're discouraged in new code. But for example if you don't have a bug link, AFAICS TODO(username): is the only possible form.

clang-tools-extra/clang-tidy/google/TodoCommentCheck.h

 class TodoCommentCheck : public ClangTidyCheck {
 public:
   TodoCommentCheck(StringRef Name, ClangTidyContext *Context);
   ~TodoCommentCheck();
 
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;
 
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override {
+    Options.store(Opts, "UseV2Style", UseV2Style);
+  }
+
 private:
   class TodoCommentHandler;
+  bool UseV2Style;
   std::unique_ptr<TodoCommentHandler> Handler;
 };
 
 } // namespace clang::tidy::google::readability
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_TODOCOMMENTCHECK_H

clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp

 //===--- TodoCommentCheck.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 "TodoCommentCheck.h"
+#include "clang/Basic/Diagnostic.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Preprocessor.h"
+#include "llvm/Support/ErrorHandling.h"
 #include <optional>
 
 namespace clang::tidy::google::readability {
 
+enum class TodoStyleVersion {
+  V1,
+  V2,
+};
+
+std::string_view getVersionRegex(TodoStyleVersion V) {
+  switch (V) {
+  case TodoStyleVersion::V1:
+    return "^// *TODO *(\\(.*\\))?:?( )?(.*)$";
+  case TodoStyleVersion::V2:
+    return "^// *TODO *((: *[^ ]+( - .)?)|([(][^)]*[)])?).*$";
+  }
+  llvm_unreachable("bad enum value");
+}
+
 class TodoCommentCheck::TodoCommentHandler : public CommentHandler {
 public:
-  TodoCommentHandler(TodoCommentCheck &Check, std::optional<std::string> User)
-      : Check(Check), User(User ? *User : "unknown"),
-        TodoMatch("^// *TODO *(\\(.*\\))?:?( )?(.*)$") {}
+  TodoCommentHandler(TodoCommentCheck &Check, std::optional<std::string> User,
+                     TodoStyleVersion V)
+      : Check(Check), User(User ? *User : "unknown"), Version(V),
+        TodoMatch(getVersionRegex(V)) {}
 
   bool HandleComment(Preprocessor &PP, SourceRange Range) override {
     StringRef Text =
         Lexer::getSourceText(CharSourceRange::getCharRange(Range),
                              PP.getSourceManager(), PP.getLangOpts());
 
+    switch (Version) {
+    case TodoStyleVersion::V1:
+      handleV1Comment(Text, Range);
+      return false;
+    case TodoStyleVersion::V2:
+      handleV2Comment(Text, Range);
+      return false;
+    }
+  }
+
+private:
+  void handleV1Comment(StringRef Text, SourceRange Range) {
     SmallVector<StringRef, 4> Matches;
     if (!TodoMatch.match(Text, &Matches))
-      return false;
+      return;
 
     StringRef Username = Matches[1];
     StringRef Comment = Matches[3];
 
     if (!Username.empty())
-      return false;
+      return;
 
     std::string NewText = ("// TODO(" + Twine(User) + "): " + Comment).str();
 
     Check.diag(Range.getBegin(), "missing username/bug in TODO")
         << FixItHint::CreateReplacement(CharSourceRange::getCharRange(Range),
                                         NewText);
-    return false;
+  }
+
+  void handleV2Comment(StringRef Text, SourceRange Range) {
+    SmallVector<StringRef, 5> Matches;
+    if (!TodoMatch.match(Text, &Matches))
+      return;
+
+    if (/* New style*/ (!Matches[2].empty() && !Matches[3].empty()) ||
+        /* Old style */ !Matches[4].empty())
+      return;
+
+    // Either the component after " - " is missing (and it's something like new
+    // style) or it doesn't match any style, beyond starting with TODO.
+    Check.diag(Range.getBegin(), "TODO requires link and description");
   }
 
 private:
   TodoCommentCheck &Check;
   std::string User;
+  TodoStyleVersion Version;
   llvm::Regex TodoMatch;
 };
 
 TodoCommentCheck::TodoCommentCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
+      UseV2Style(Options.getLocalOrGlobal("UseV2Style", false)),
       Handler(std::make_unique<TodoCommentHandler>(
-          *this, Context->getOptions().User)) {}
+          *this, Context->getOptions().User,
+          UseV2Style ? TodoStyleVersion::V2 : TodoStyleVersion::V1)) {}
 
 TodoCommentCheck::~TodoCommentCheck() = default;
 
 void TodoCommentCheck::registerPPCallbacks(const SourceManager &SM,
                                            Preprocessor *PP,
                                            Preprocessor *ModuleExpanderPP) {
   PP->addCommentHandler(Handler.get());
 }
 
 } // namespace clang::tidy::google::readability

clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst

 .. title:: clang-tidy - google-readability-todo
 
 google-readability-todo
 =======================
 
-Finds TODO comments without a username or bug number.
+Finds TODO comments not conforming to Google's style guidelines.
 
 The relevant style guide section is
 https://google.github.io/styleguide/cppguide.html#TODO_Comments.
 
 Corresponding cpplint.py check: `readability/todo`
+
+Options
+-------
+
+.. option:: UseV2Style
+
+   Disabled by default.
+
+   When disabled, the check enforces the style
+   ```
+   // TODO(username/bug): description
+   ```
+   and will suggest fixes in this style, where possible (for example, a TODO
+   missing the username).
+   
+   When enabled, the check also accepts TODO comments in the following style:
+   ```
+   // TODO: link - description
+   ```   
+   It will still admit the previous style, for backwards compatability. But, it
+   won't suggest fixes, since mistakes are now ambiguous: given `TODO: text`, "text"
+   could be the description or it could be a link of some form.
+

clang-tools-extra/test/clang-tidy/checkers/google/readability-todo-v2.cpp

+// RUN: %check_clang_tidy %s google-readability-todo %t -- \
+// RUN: -config="{User: 'some user', CheckOptions: \
+// RUN:   [{key: google-readability-todo.UseV2Style, \
+// RUN:     value: 'true'}]}" \
+// RUN: --
+
+//   TODOfix this1
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+//   TODO fix this2
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+// TODO fix this3
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+// TODO: fix this4
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+// V2-style TODO comments:
+
+// TODO: https://github.com/llvm/llvm-project/issues/12345 - Some description
+// TODO: link - description
+
+// V1-style TODO comments, supported for backwards compatability:
+
+//   TODO(clang)fix this5
+
+// TODO(foo):shave yaks
+// TODO(bar):
+// TODO(foo): paint bikeshed
+// TODO(b/12345): find the holy grail
+// TODO (b/12345): allow spaces before parentheses
+// TODO(asdf) allow missing semicolon