[clang] Add test run of 'dump_ast_matchers.py'

[vbvictor]

We use dump_ast_matchers.py script to update this auto-generated file in case someone make changes to ASTMatchers.
This PR ensures that dump_ast_matchers.py was run after change in ASTMatchers.

Closes #165418.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Baranov Victor (vbvictor)

Changes

WIP

Closes #165418.

---

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

2 Files Affected:

• (modified) clang/include/clang/ASTMatchers/ASTMatchers.h (+1-1)
• (modified) llvm/utils/git/code-format-helper.py (+64-1)

diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index 98e62de2a9bfb..cc72613db4d15 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -1390,7 +1390,7 @@ extern const internal::VariadicDynCastAllOfMatcher<Expr, RequiresExpr>
 
 /// Matches concept requirement body declaration.
 ///
-/// Example matches '{ *p; }'
+/// Example matches '{ * p; }'
 /// \code
 ///   template<typename T>
 ///   concept dereferencable = requires(T p) { *p; }
diff --git a/llvm/utils/git/code-format-helper.py b/llvm/utils/git/code-format-helper.py
index 406a72817acb8..6de7d21ab68a6 100755
--- a/llvm/utils/git/code-format-helper.py
+++ b/llvm/utils/git/code-format-helper.py
@@ -466,7 +466,70 @@ def format_run(self, changed_files: List[str], args: FormatArgs) -> Optional[str
         return report
 
 
-ALL_FORMATTERS = (DarkerFormatHelper(), ClangFormatHelper(), UndefGetFormatHelper())
+class DumpASTMatchersHelper(FormatHelper):
+    name = "dump_ast_matchers"
+    friendly_name = "AST matchers documentation"
+
+    output_html = "clang/docs/LibASTMatchersReference.html"
+    script_dir = "clang/docs/tools"
+    script_name = "dump_ast_matchers.py"
+
+    @property
+    def instructions(self) -> str:
+        return f"cd {self.script_dir} && python {self.script_name}"
+
+    def should_run(self, changed_files: List[str]) -> List[str]:
+        for file in changed_files:
+            if file == "clang/include/clang/ASTMatchers/ASTMatchers.h":
+                return True
+        return False
+
+    def has_tool(self) -> bool:
+        if not os.path.exists(os.path.join(self.script_dir, self.script_name)):
+            return False
+        return True
+
+    def format_run(self, changed_files: List[str], args: FormatArgs) -> Optional[str]:
+        if not self.should_run(changed_files):
+            return None
+
+        if args.verbose:
+            print(f"Running: {self.instructions}")
+
+        # Run the 'dump_ast_matchers.py' from its directory as specified in the script doc
+        proc = subprocess.run(
+            ["python", self.script_name],
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            encoding="utf-8",
+            cwd=self.script_dir,
+        )
+
+        if proc.returncode != 0:
+            return f"Execution of 'dump_ast_matchers.py' failed:\n{proc.stderr}\n{proc.stdout}"
+
+        # Check if 'LibASTMatchersReference.html' file was modified
+        cmd = ["git", "diff", "--exit-code", self.output_html]
+        proc = subprocess.run(
+            cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8"
+        )
+
+        # 'LibASTMatchersReference.html' was modified - count as failure
+        if proc.returncode != 0:
+            if args.verbose:
+                print(f"error: {self.name} exited with code {proc.returncode}")
+                print(proc.stdout.decode("utf-8"))
+            return proc.stdout.decode("utf-8")
+        else:
+            return None
+
+
+ALL_FORMATTERS = (
+    DarkerFormatHelper(),
+    ClangFormatHelper(),
+    UndefGetFormatHelper(),
+    DumpASTMatchersHelper(),
+)
 
 
 def hook_main():

[github-actions]

✅ With the latest revision this PR passed the AST matchers documentation.

[github-actions]

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

[vbvictor]

Adding AST Matchers/clang-tidy maintainers to gather opinion from dump_ast_matchers.py standpoint.

[boomanaiden154]

Why do we need to do something in the code formatting action that's completely custom instead of something like #118154?

Also, this should probably just be autogenerated inside the build system.

I tried this for the example above in #113739, but maintainers were not happy.

[vbvictor]

Why do we need to do something in the code formatting action that's completely custom instead of something like #118154?

Thank you for reference, I implemented it the easy way. Though formatting message looked cool

The job will fail till we land, #165448.

[boomanaiden154]

The PR title and description need to be updated. It would be good if the branch gets updated to kick off CI again too.

[boomanaiden154]

LGTM. Probably would be good to get approval from someone on the clang side as well.

[vbvictor]

Disabled test on Windows because of error which makes re-generated file different from expected:

<urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1129)>

This should be either fixed by tinkering with certs in win-builder or by adjusting dump_ast_matchers.py.
But I believe this should be out of current scope because Linux builder still run the test which is platform-independent.

[boomanaiden154]

The test shouldn't be accessing anything over the internet. That should be fixed before landing.

[AaronBallman]

Hmmm, instead of getting a precommit CI failure when forgetting to run the AST documentation generator, we should be running the documentation generator on-demand in the backend like we do for generating docs from attributes or diagnostics and then nobody has to remember to do that step ever.

This is still a step in the right direction, so I'm not opposed.

[AaronBallman]

Why is the test unsupported on Windows?

[vbvictor]

There was error with ssl certificates of urlopen #165472 (comment), so I disabled the test for now.

[vbvictor]

Thought I'm trying to make this script totally offline, so we should not have this problem in the long run.

[vbvictor]

The test shouldn't be accessing anything over the internet. That should be fixed before landing.

I tried to make it full offline, but stopped on this issue #163548.

Basically, in dump_ast_matchers.py we fetch https://clang.llvm.org/doxygen/classes.html from the web and try to generate links base on classes that can be found in Doxygen docs.
To make script full offline, we better make all links by hand with expected class-names. But it turns out that some classes are simply not rendered in https://clang.llvm.org/doxygen/classes.html (thought can be found in release build of doxygen)