[libc++][modules] Increase clang-tidy version used.

[mordante]

As suggested in #71438 we should use
export import std;
in the std.compat module.

Testing this locally failed when building with the clang-tidy-17 plugin. The std module was considered corrupt in the test
libcxx/test/libcxx/module_std_compat.gen.py
however the test
libcxx/test/libcxx/module_std.gen.py
passed. Both test generated identical std.pcm files. Using the clang-tidy-18 plugin solves the issue.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

As suggested in #71438 we should use
export import std;
in the std.compat module.

Testing this locally failed when building with the clang-tidy-17 plugin. The std module was considered corrupt in the test
libcxx/test/libcxx/module_std_compat.gen.py
however the test
libcxx/test/libcxx/module_std.gen.py
passed. Both test generated identical std.pcm files. Using the clang-tidy-18 plugin solves the issue.

---

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

2 Files Affected:

• (modified) libcxx/test/tools/clang_tidy_checks/CMakeLists.txt (+2-2)
• (modified) libcxx/utils/libcxx/test/features.py (+2-2)

diff --git a/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt b/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
index 260e90f45f577c..978e7095216522 100644
--- a/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
+++ b/libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
@@ -5,9 +5,9 @@
 set(LLVM_DIR_SAVE ${LLVM_DIR})
 set(Clang_DIR_SAVE ${Clang_DIR})
 
-find_package(Clang 17)
+find_package(Clang 18)
 if (NOT Clang_FOUND)
-  find_package(Clang 18)
+  find_package(Clang 17)
 endif()
 
 set(SOURCES
diff --git a/libcxx/utils/libcxx/test/features.py b/libcxx/utils/libcxx/test/features.py
index 58c33e5bb37475..1ada090a9abff7 100644
--- a/libcxx/utils/libcxx/test/features.py
+++ b/libcxx/utils/libcxx/test/features.py
@@ -31,8 +31,8 @@ def _getSuitableClangTidy(cfg):
             return None
 
         # TODO MODULES require ToT due module specific fixes.
-        if runScriptExitCode(cfg, ['clang-tidy-17 --version']) == 0:
-          return 'clang-tidy-17'
+        if runScriptExitCode(cfg, ['clang-tidy-18 --version']) == 0:
+          return 'clang-tidy-18'
 
         # TODO This should be the last stable release.
         # LLVM RELEASE bump to latest stable version

[github-actions]

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:

darker --check --diff -r d06ae33ec32122bb526fb35025c1f0cf979f1090...84c74e07fb1505926bbe4a8a0823eff4baf302d9 libcxx/utils/libcxx/test/features.py

View the diff from darker here.

--- features.py	2024-01-17 07:17:04.000000 +0000
+++ features.py	2024-01-17 07:20:16.412052 +0000
@@ -29,12 +29,12 @@
         # If we didn't build the libcxx-tidy plugin via CMake, we can't run the clang-tidy tests.
         if runScriptExitCode(cfg, ["stat %{test-tools}/clang_tidy_checks/libcxx-tidy.plugin"]) != 0:
             return None
 
         # TODO MODULES require ToT due module specific fixes.
-        if runScriptExitCode(cfg, ['clang-tidy-18 --version']) == 0:
-          return 'clang-tidy-18'
+        if runScriptExitCode(cfg, ["clang-tidy-18 --version"]) == 0:
+            return "clang-tidy-18"
 
         # TODO This should be the last stable release.
         # LLVM RELEASE bump to latest stable version
         if runScriptExitCode(cfg, ["clang-tidy-16 --version"]) == 0:
             return "clang-tidy-16"

[philnik777]

I'm really not happy with bumping the clang-tidy version we use all the time to the trunk version. We agreed to using the latest stable version, which we've not done way too many times now. I'd really like to first understand what exactly the issue is that is solved by bumping the version again.

[mordante]

I'm really not happy with bumping the clang-tidy version we use all the time to the trunk version. We agreed to using the latest stable version, which we've not done way too many times now. I'd really like to first understand what exactly the issue is that is solved by bumping the version again.

I'm also not too happy that we need to bump it, but the newest version has fixes I need. That patch will be 2 patches from now in this stacked review. I think the description of the review is clear what the issue is. Can you tell what's not clear?

[philnik777]

I'd like to understand what the fixed issues are you're relying on. From what I can tell, this could just as much be a bug in our setup for the clang-tidy plugin as an actual fix in trunk.

[mordante]

It's the line export import std; in
https://github.com/llvm/llvm-project/pull/76330/files#diff-e881fdd0e6e66610142a28228b2bbf0e38520ee7186946bca06cb8d195dcd2b4

This works with Clang-17, Clang-18, and clang-tidy-18. It fails with clang-tidy-17. Clang-tidy tests that directly use the std module work. When importing std clang-tidy complains about a corrupt AST in the std module. So this sounds like a clang-tidy-17 but that has been fixed in clang-tidy-18.

[philnik777]

Oh shit. I just realized that this is most likely a latent bug no matter what. We build the module with Clang 18, and then essentially try to load it with Clang 17 (aka Clang Tidy 17). AFAIK that's not guaranteed to work, and probably just happens to work currently with Clang 17 building and Clang 18 loading the module (assuming we even test that right now?). I think we may have to always match the Clang and Clang Tidy versions we use.

[mordante]

Good point, I actually think that's true. I think we should do that in a separate PR. Maybe discuss it on Discord after the holidays.

[H-G-Hristov]

Oh shit. I just realized that this is most likely a latent bug no matter what. We build the module with Clang 18, and then essentially try to load it with Clang 17 (aka Clang Tidy 17). AFAIK that's not guaranteed to work, and probably just happens to work currently with Clang 17 building and Clang 18 loading the module (assuming we even test that right now?). I think we may have to always match the Clang and Clang Tidy versions we use.

I should probably keep out of these discussions but here I am: Matching Clang with Clang-Tidy versions feels only natural. For instance "Member visit" requires new syntax (deducing this) and fixes available in the latest Clang 18 nightly, so it was surprising to find out the test failing due to Clang-Tidy being used in the CI. I guess this case happens rarely but this means working on library features dependant on newly implemented language features might have to be postponed to the release after.

[philnik777]

Oh shit. I just realized that this is most likely a latent bug no matter what. We build the module with Clang 18, and then essentially try to load it with Clang 17 (aka Clang Tidy 17). AFAIK that's not guaranteed to work, and probably just happens to work currently with Clang 17 building and Clang 18 loading the module (assuming we even test that right now?). I think we may have to always match the Clang and Clang Tidy versions we use.

I should probably keep out of these discussions but here I am: Matching Clang with Clang-Tidy versions feels only natural. For instance "Member visit" requires new syntax (deducing this) and fixes available in the latest Clang 18 nightly, so it was surprising to find out the test failing due to Clang-Tidy being used in the CI. I guess this case happens rarely but this means working on library features dependant on newly implemented language features might have to be postponed to the release after.

This change doesn't actually help you in this regard. We still support Clang 16 and 17, so the CI would have simply failed at a later stage, but for the same reason.

@mordante I'd much rather see this fixed properly than tape over it with this patch.

[mordante]

Oh shit. I just realized that this is most likely a latent bug no matter what. We build the module with Clang 18, and then essentially try to load it with Clang 17 (aka Clang Tidy 17). AFAIK that's not guaranteed to work, and probably just happens to work currently with Clang 17 building and Clang 18 loading the module (assuming we even test that right now?). I think we may have to always match the Clang and Clang Tidy versions we use.

I should probably keep out of these discussions but here I am:

You're welcome participate in the discussion. This is a public forum.

Matching Clang with Clang-Tidy versions feels only natural. For instance "Member visit" requires new syntax (deducing this) and fixes available in the latest Clang 18 nightly, so it was surprising to find out the test failing due to Clang-Tidy being used in the CI. I guess this case happens rarely but this means working on library features dependant on newly implemented language features might have to be postponed to the release after.

+1

This change doesn't actually help you in this regard. We still support Clang 16 and 17, so the CI would have simply failed at a later stage, but for the same reason.

@mordante I'd much rather see this fixed properly than tape over it with this patch.

Then I can prohibit clang-16 and clang-17. IMO The biggest issue is that the clang-tidy version is hard-coded in CMake. This is something I mentioned in the past too. If it was a CMake option it would be trivial to change the value without changing CMakeList.txt.

[philnik777]

Oh shit. I just realized that this is most likely a latent bug no matter what. We build the module with Clang 18, and then essentially try to load it with Clang 17 (aka Clang Tidy 17). AFAIK that's not guaranteed to work, and probably just happens to work currently with Clang 17 building and Clang 18 loading the module (assuming we even test that right now?). I think we may have to always match the Clang and Clang Tidy versions we use.

I should probably keep out of these discussions but here I am:

You're welcome participate in the discussion. This is a public forum.

Definitely. @H-G-Hristov feel free to comment on things you have an opinion/question about. Having an outside view is often quite helpful.

Then I can prohibit clang-16 and clang-17.

Yeah, that's the solution. clang-tidy defines the same version macros as clang does, so prohibiting it from clang-16 is the same as prohibiting it for clang-tidy 16.

IMO The biggest issue is that the clang-tidy version is hard-coded in CMake. This is something I mentioned in the past too. If it was a CMake option it would be trivial to change the value without changing CMakeList.txt.

We could also extract the clang version used and search for a clang-tidy that fits this version. That would also avoid the matching problems for others. We would probably have to get a path to the corresponding clang-tidy too. I don't know whether that's trivially possible right now.

[mordante]

Then I can prohibit clang-16 and clang-17.

Yeah, that's the solution. clang-tidy defines the same version macros as clang does, so prohibiting it from clang-16 is the same as prohibiting it for clang-tidy 16.

What makes it currently hard for modules is that in the current situation it would mean modules would have about 0% coverage.

IMO The biggest issue is that the clang-tidy version is hard-coded in CMake. This is something I mentioned in the past too. If it was a CMake option it would be trivial to change the value without changing CMakeList.txt.

We could also extract the clang version used and search for a clang-tidy that fits this version. That would also avoid the matching problems for others. We would probably have to get a path to the corresponding clang-tidy too. I don't know whether that's trivially possible right now.

I think matching the version would be trivial for the additional libraries. For the executable it might be harder, but there we can let the user specify an executable and if specified test that version. Then we can issue a diagnostic when the versions don't match.

[philnik777]

I'm OK with this as a quick fix, since Mark promised to fix it properly in the long term.