Extend support for specifying languages and version in add_new_check.py

[njames93]

• Allow specifying a language standard when adding a new check
• Simplify the language standards(and or-later) handlnig in check_clang_tidy

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Nathan James (njames93)

Changes

• Allow specifying a language standard when adding a new check
• Simplify the language standards(and or-later) handlnig in check_clang_tidy

---

Full diff: https://github.com/llvm/llvm-project/pull/100129.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/add_new_check.py (+94-16)
• (modified) clang-tools-extra/test/clang-tidy/check_clang_tidy.py (+35-15)

diff --git a/clang-tools-extra/clang-tidy/add_new_check.py b/clang-tools-extra/clang-tidy/add_new_check.py
index 3a62df1f510ba..adc99566b35c9 100755
--- a/clang-tools-extra/clang-tidy/add_new_check.py
+++ b/clang-tools-extra/clang-tidy/add_new_check.py
@@ -13,10 +13,12 @@
 
 import argparse
 import io
+import itertools
 import os
 import re
 import sys
 
+
 # Adapts the module's CMakelist file. Returns 'True' if it could add a new
 # entry and 'False' if the entry already existed.
 def adapt_cmake(module_path, check_name_camel):
@@ -53,7 +55,18 @@ def adapt_cmake(module_path, check_name_camel):
 
 
 # Adds a header for the new check.
-def write_header(module_path, module, namespace, check_name, check_name_camel):
+def write_header(
+    module_path, module, namespace, check_name, check_name_camel, lang_restrict
+):
+    if lang_restrict:
+        override_supported = """
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return %s;
+  }""" % (
+            lang_restrict % {"lang": "LangOpts"}
+        )
+    else:
+        override_supported = ""
     filename = os.path.join(module_path, check_name_camel) + ".h"
     print("Creating %s..." % filename)
     with io.open(filename, "w", encoding="utf8", newline="\n") as f:
@@ -94,7 +107,7 @@ class %(check_name_camel)s : public ClangTidyCheck {
   %(check_name_camel)s(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
-  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;%(override_supported)s
 };
 
 } // namespace clang::tidy::%(namespace)s
@@ -107,6 +120,7 @@ class %(check_name_camel)s : public ClangTidyCheck {
                 "check_name": check_name,
                 "module": module,
                 "namespace": namespace,
+                "override_supported": override_supported,
             }
         )
 
@@ -292,7 +306,9 @@ def add_release_notes(module_path, module, check_name):
 
 
 # Adds a test for the check.
-def write_test(module_path, module, check_name, test_extension):
+def write_test(module_path, module, check_name, test_extension, test_standard):
+    if test_standard:
+        test_standard = f"-std={test_standard}-or-later "
     check_name_dashes = module + "-" + check_name
     filename = os.path.normpath(
         os.path.join(
@@ -309,7 +325,7 @@ def write_test(module_path, module, check_name, test_extension):
     print("Creating %s..." % filename)
     with io.open(filename, "w", encoding="utf8", newline="\n") as f:
         f.write(
-            """// RUN: %%check_clang_tidy %%s %(check_name_dashes)s %%t
+            """// RUN: %%check_clang_tidy %(standard)s%%s %(check_name_dashes)s %%t
 
 // FIXME: Add something that triggers the check here.
 void f();
@@ -324,7 +340,7 @@ def write_test(module_path, module, check_name, test_extension):
 // FIXME: Add something that doesn't trigger the check here.
 void awesome_f2();
 """
-            % {"check_name_dashes": check_name_dashes}
+            % {"check_name_dashes": check_name_dashes, "standard": test_standard}
         )
 
 
@@ -497,7 +513,10 @@ def format_link_alias(doc_file):
         if (match or (check_name.startswith("clang-analyzer-"))) and check_name:
             module = doc_file[0]
             check_file = doc_file[1].replace(".rst", "")
-            if not match or match.group(1) == "https://clang.llvm.org/docs/analyzer/checkers":
+            if (
+                not match
+                or match.group(1) == "https://clang.llvm.org/docs/analyzer/checkers"
+            ):
                 title = "Clang Static Analyzer " + check_file
                 # Preserve the anchor in checkers.html from group 2.
                 target = "" if not match else match.group(1) + ".html" + match.group(2)
@@ -515,7 +534,7 @@ def format_link_alias(doc_file):
             if target:
                 # The checker is just a redirect.
                 return (
-                        "   :doc:`%(check_name)s <%(module)s/%(check_file)s>`, %(ref_begin)s`%(title)s <%(target)s>`%(ref_end)s,%(autofix)s\n"
+                    "   :doc:`%(check_name)s <%(module)s/%(check_file)s>`, %(ref_begin)s`%(title)s <%(target)s>`%(ref_end)s,%(autofix)s\n"
                     % {
                         "check_name": check_name,
                         "module": module,
@@ -523,13 +542,14 @@ def format_link_alias(doc_file):
                         "target": target,
                         "title": title,
                         "autofix": autofix,
-                        "ref_begin" : ref_begin,
-                        "ref_end" : ref_end
-                    })
+                        "ref_begin": ref_begin,
+                        "ref_end": ref_end,
+                    }
+                )
             else:
                 # The checker is just a alias without redirect.
                 return (
-                        "   :doc:`%(check_name)s <%(module)s/%(check_file)s>`, %(title)s,%(autofix)s\n"
+                    "   :doc:`%(check_name)s <%(module)s/%(check_file)s>`, %(title)s,%(autofix)s\n"
                     % {
                         "check_name": check_name,
                         "module": module,
@@ -537,7 +557,8 @@ def format_link_alias(doc_file):
                         "target": target,
                         "title": title,
                         "autofix": autofix,
-                    })
+                    }
+                )
         return ""
 
     checks = map(format_link, doc_files)
@@ -599,6 +620,22 @@ def main():
         "objc": "m",
         "objc++": "mm",
     }
+    cpp_language_to_requirements = {
+        "c++98": "CPlusPlus",
+        "c++11": "CPlusPlus11",
+        "c++14": "CPlusPlus14",
+        "c++17": "CPlusPlus17",
+        "c++20": "CPlusPlus20",
+        "c++23": "CPlusPlus23",
+        "c++26": "CPlusPlus26",
+    }
+    c_language_to_requirements = {
+        "c99": None,
+        "c11": "C11",
+        "c17": "C17",
+        "c23": "C23",
+        "c27": "C2Y",
+    }
     parser = argparse.ArgumentParser()
     parser.add_argument(
         "--update-docs",
@@ -609,9 +646,19 @@ def main():
         "--language",
         help="language to use for new check (defaults to c++)",
         choices=language_to_extension.keys(),
-        default="c++",
+        default=None,
         metavar="LANG",
     )
+    parser.add_argument(
+        "--standard",
+        help="Specify a specific version of the language",
+        choices=list(
+            itertools.chain(
+                cpp_language_to_requirements.keys(), c_language_to_requirements.keys()
+            )
+        ),
+        default=None,
+    )
     parser.add_argument(
         "module",
         nargs="?",
@@ -652,12 +699,43 @@ def main():
     else:
         namespace = module
 
-    write_header(module_path, module, namespace, check_name, check_name_camel)
+    language = args.language
+
+    if args.standard:
+        if args.standard in cpp_language_to_requirements:
+            if language and language != "c++":
+                raise ValueError("C++ standard chosen when language is not C++")
+            language = "c++"
+        elif args.standard in c_language_to_requirements:
+            if language and language != "c":
+                raise ValueError("C standard chosen when language is not C")
+            language = "c"
+
+    if not language:
+        language = "c++"
+
+    language_restrict = None
+
+    if language == "c":
+        language_restrict = "!%(lang)s.CPlusPlus"
+        extra = c_language_to_requirements.get(args.standard, None)
+        if extra:
+            language_restrict += f" && %(lang)s.{extra}"
+    elif language == "c++":
+        language_restrict = (
+            f"%(lang)s.{cpp_language_to_requirements.get(args.standard, 'CPlusPlus')}"
+        )
+    elif language in ["objc", "objc++"]:
+        language_restrict = "%(lang)s.ObjC"
+
+    write_header(
+        module_path, module, namespace, check_name, check_name_camel, language_restrict
+    )
     write_implementation(module_path, module, namespace, check_name_camel)
     adapt_module(module_path, module, check_name, check_name_camel)
     add_release_notes(module_path, module, check_name)
-    test_extension = language_to_extension.get(args.language)
-    write_test(module_path, module, check_name, test_extension)
+    test_extension = language_to_extension.get(language)
+    write_test(module_path, module, check_name, test_extension, args.standard)
     write_docs(module_path, module, check_name)
     update_checks_list(clang_tidy_path)
     print("Done. Now it's your turn!")
diff --git a/clang-tools-extra/test/clang-tidy/check_clang_tidy.py b/clang-tools-extra/test/clang-tidy/check_clang_tidy.py
index e92179ac82c6a..5e39c05f76d86 100755
--- a/clang-tools-extra/test/clang-tidy/check_clang_tidy.py
+++ b/clang-tools-extra/test/clang-tidy/check_clang_tidy.py
@@ -205,9 +205,11 @@ def run_clang_tidy(self):
                 self.temp_file_name,
             ]
             + [
-                "-fix"
-                if self.export_fixes is None
-                else "--export-fixes=" + self.export_fixes
+                (
+                    "-fix"
+                    if self.export_fixes is None
+                    else "--export-fixes=" + self.export_fixes
+                )
             ]
             + [
                 "--checks=-*," + self.check_name,
@@ -299,19 +301,37 @@ def run(self):
             self.check_notes(clang_tidy_output)
 
 
+CPP_STANDARDS = [
+    "c++98",
+    "c++11",
+    ("c++14", "c++1y"),
+    ("c++17", "c++1z"),
+    ("c++20", "c++2a"),
+    ("c++23", "c++2b"),
+    ("c++26", "c++2c"),
+]
+C_STANDARDS = ["c99", ("c11", "c1x"), "c17", ("c23", "c2x"), "c2y"]
+
+
 def expand_std(std):
-    if std == "c++98-or-later":
-        return ["c++98", "c++11", "c++14", "c++17", "c++20", "c++23", "c++2c"]
-    if std == "c++11-or-later":
-        return ["c++11", "c++14", "c++17", "c++20", "c++23", "c++2c"]
-    if std == "c++14-or-later":
-        return ["c++14", "c++17", "c++20", "c++23", "c++2c"]
-    if std == "c++17-or-later":
-        return ["c++17", "c++20", "c++23", "c++2c"]
-    if std == "c++20-or-later":
-        return ["c++20", "c++23", "c++2c"]
-    if std == "c++23-or-later":
-        return ["c++23", "c++2c"]
+    split_std, or_later, _ = std.partition("-or-later")
+
+    if not or_later:
+        return [split_std]
+
+    for standard_list in (CPP_STANDARDS, C_STANDARDS):
+        item = next(
+            (
+                i
+                for i, v in enumerate(standard_list)
+                if (split_std in v if isinstance(v, (list, tuple)) else split_std == v)
+            ),
+            None,
+        )
+        if item is not None:
+            return [split_std] + [
+                x if isinstance(x, str) else x[0] for x in standard_list[item + 1 :]
+            ]
     return [std]
 
 

[PiotrZSL]

would be nice to add also getCheckTraversalKind with a TK_IgnoreUnlessSpelledInSource used by default

[njames93]

That's a good idea for a follow up :)

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[Marcondiro]

Hello @njames93,
After this PR, the class derived from ClangTidyCheck created by the script add_new_check.py overrides isLanguageVersionSupported by default, restricting the check's scope to LangOpts.CPlusPlus code.

Running clang-tidy/add_new_check.py readability awesome-function-names generates:

// clang-tools-extra/clang-tidy/readability/AwesomeFunctionNamesCheck.h
class AwesomeFunctionNamesCheck : public ClangTidyCheck {
public:
  [...]
  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
    return LangOpts.CPlusPlus;
  }
};

Is this behavior expected?
If so, clang-tools-extra/docs/clang-tidy/Contributing.rst could be updated accordingly. (It definitely didn't take me a while to figure out why my check wasn't working on C code 😄 )

Thank you!

[njames93]

Having a default in there is important to help people understand how to use it. But, you're right, maybe we should document this so that people won't fall into this trap.
FWIW the script does have an argument to specify the language the check should run on.