[libc++] Add clang-tidy check for version checks

[philnik]

Summary

This check flags code which uses _LIBCPP_STD_VER the wrong way, or tries to use __cplusplus. It flags cases where we use _LIBCPP_STD_VER > instead of _LIBCPP_STD_VER >= and where wrong values are used (e.g. _LIBCPP_STD_VER >= 24).

[Mordante]

Are we running clang-tidy only on our headers or also on our test suite?

libcxx/test/tools/clang_tidy_checks/CMakeLists.txt

6–15

This should be 16 for the CI. IIRC 17 is not installed by the CI.
I ran issues with this while testing locally with Clang-tidy, I am looking at the issue where cmake fails to run a second time and I have a solution, but I want to test a bit further before making a patch.

[philnik]

In D144261#4142106, @Mordante wrote:

Are we running clang-tidy only on our headers or also on our test suite?

Only in our headers currently, not even in src/.

libcxx/test/tools/clang_tidy_checks/CMakeLists.txt

6–15

This diff shouldn't be here. The problem is that find_package(Clang 16...17) isn't "use Clang version 16 or 17", like one would expect. I'm not sure what it actually is, but AFAICT we have to use

find_package(Clang 16)
if (NOT Clang_FOUND)
  find_package(Clang 17)
endif()

to fix this.

[Mordante]

LGTM, but I like to see the CI green before approving.

libcxx/test/tools/clang_tidy_checks/CMakeLists.txt

6–15

I have a fix for running CMake twice and can address this issue in the same patch, so let's revert this hunk in this patch.

[ldionne]

LGTM w/ green CI. Please also provide a short description of the check being added in the commit message. Add clang-tidy check for version checks is not self-explanatory.

[philnik]

Rebased

libcxx/include/__chrono/concepts.h

 #include <__type_traits/is_specialization.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-#if _LIBCPP_STD_VER > 17
+#if _LIBCPP_STD_VER >= 20
 
 template <class _Tp>
 concept __is_hh_mm_ss = __is_specialization_v<_Tp, chrono::hh_mm_ss>;
 
-#endif // _LIBCPP_STD_VER > 17
+#endif // _LIBCPP_STD_VER >= 20
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___CHRONO_CONCEPTS_H

libcxx/include/__config

 // Valid C++ identifier that revs with every libc++ version. This can be used to
 // generate identifiers that must be unique for every released libc++ version.
 #  define _LIBCPP_VERSIONED_IDENTIFIER _LIBCPP_CONCAT(v, _LIBCPP_VERSION)
 
 #  if __STDC_HOSTED__ == 0
 #    define _LIBCPP_FREESTANDING
 #  endif
 
+// NOLINTBEGIN(libcpp-cpp-version-check)
 #  ifndef _LIBCPP_STD_VER
 #    if __cplusplus <= 201103L
 #      define _LIBCPP_STD_VER 11
 #    elif __cplusplus <= 201402L
 #      define _LIBCPP_STD_VER 14
 #    elif __cplusplus <= 201703L
 #      define _LIBCPP_STD_VER 17
 #    elif __cplusplus <= 202002L
 #      define _LIBCPP_STD_VER 20
 #    else
 // Expected release year of the next C++ standard
 #      define _LIBCPP_STD_VER 23
 #    endif
 #  endif // _LIBCPP_STD_VER
+// NOLINTEND(libcpp-cpp-version-check)
 
 #  if defined(__ELF__)
 #    define _LIBCPP_OBJECT_FORMAT_ELF 1
 #  elif defined(__MACH__)
 #    define _LIBCPP_OBJECT_FORMAT_MACHO 1
 #  elif defined(_WIN32)
 #    define _LIBCPP_OBJECT_FORMAT_COFF 1
 #  elif defined(__wasm__)

 // its vtable and typeinfo to libc++ rather than having all other libraries
 // using that class define their own copies.
 #    define _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION
 #  endif
 
 #  define _LIBCPP_TOSTRING2(x) #x
 #  define _LIBCPP_TOSTRING(x) _LIBCPP_TOSTRING2(x)
 
+// NOLINTNEXTLINE(libcpp-cpp-version-check)
 #  if __cplusplus < 201103L
 #    define _LIBCPP_CXX03_LANG
 #  endif
 
 #  ifndef __has_attribute
 #    define __has_attribute(__x) 0
 #  endif
 

libcxx/include/iomanip

 template <class _CharT>
 inline _LIBCPP_INLINE_VISIBILITY
 __iom_t10<_CharT>
 put_time(const tm* __tm, const _CharT* __fmt)
 {
     return __iom_t10<_CharT>(__tm, __fmt);
 }
 
-#if _LIBCPP_STD_VER >= 11
-
 template <class _CharT, class _Traits>
 _LIBCPP_HIDE_FROM_ABI basic_ostream<_CharT, _Traits>&
 __quoted_output(basic_ostream<_CharT, _Traits>& __os,
                 const _CharT *__first, const _CharT *__last, _CharT __delim, _CharT __escape)
 {
     basic_string<_CharT, _Traits> __str;
     __str.push_back(__delim);
     for (; __first != __last; ++__first) {

 template <class _CharT, class _Traits, class _Allocator>
 _LIBCPP_HIDE_FROM_ABI
 __quoted_proxy<_CharT, _Traits, _Allocator>
 __quoted(basic_string<_CharT, _Traits, _Allocator>& __s, _CharT __delim = _CharT('"'), _CharT __escape = _CharT('\\'))
 {
     return __quoted_proxy<_CharT, _Traits, _Allocator>(__s, __delim, __escape);
 }
 
-#endif // _LIBCPP_STD_VER >= 11
-
 #if _LIBCPP_STD_VER >= 14
 
 template <class _CharT>
 _LIBCPP_HIDE_FROM_ABI
 auto quoted(const _CharT *__s, _CharT __delim = _CharT('"'), _CharT __escape = _CharT('\\'))
 {
     const _CharT *__end = __s;
     while (*__end) ++__end;

libcxx/test/tools/clang_tidy_checks/CMakeLists.txt

 
 # Look for the latest package
 set(CMAKE_FIND_PACKAGE_SORT_ORDER NATURAL)
 set(CMAKE_FIND_PACKAGE_SORT_DIRECTION DEC)
 
 # The find_package changes these variables. This leaves the build in an odd
 # state. Calling cmake a second time tries to write site config information in
 # the system's libc++. Restoring these setting after testing fixes this issue.
 set(LLVM_DIR_SAVE ${LLVM_DIR})
 set(Clang_DIR_SAVE ${Clang_DIR})
 
 find_package(Clang 16)
 if (NOT Clang_FOUND)
   find_package(Clang 17)
 endif()
 
 set(SOURCES
     abi_tag_on_virtual.cpp
     hide_from_abi.cpp
+    proper_version_checks.cpp
     qualify_declval.cpp
     robust_against_adl.cpp
     uglify_attributes.cpp
 
     libcpp_module.cpp
    )
 
 if(NOT Clang_FOUND)

libcxx/test/tools/clang_tidy_checks/libcpp_module.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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 "clang-tidy/ClangTidyModule.h"
 #include "clang-tidy/ClangTidyModuleRegistry.h"
 
 #include "abi_tag_on_virtual.hpp"
 #include "hide_from_abi.hpp"
-#include "robust_against_adl.hpp"
+#include "proper_version_checks.hpp"
 #include "qualify_declval.hpp"
+#include "robust_against_adl.hpp"
 #include "uglify_attributes.hpp"
 
 namespace {
 class LibcxxTestModule : public clang::tidy::ClangTidyModule {
 public:
   void addCheckFactories(clang::tidy::ClangTidyCheckFactories& check_factories) override {
     check_factories.registerCheck<libcpp::abi_tag_on_virtual>("libcpp-avoid-abi-tag-on-virtual");
     check_factories.registerCheck<libcpp::hide_from_abi>("libcpp-hide-from-abi");
+    check_factories.registerCheck<libcpp::proper_version_checks>("libcpp-cpp-version-check");
     check_factories.registerCheck<libcpp::robust_against_adl_check>("libcpp-robust-against-adl");
     check_factories.registerCheck<libcpp::uglify_attributes>("libcpp-uglify-attributes");
     check_factories.registerCheck<libcpp::qualify_declval>("libcpp-qualify-declval");
   }
 };
 } // namespace
 
 clang::tidy::ClangTidyModuleRegistry::Add<LibcxxTestModule> libcpp_module{
     "libcpp-module", "Adds libc++-specific checks."};

libcxx/test/tools/clang_tidy_checks/proper_version_checks.hpp

+//===----------------------------------------------------------------------===//
+//
+// 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 "clang-tidy/ClangTidyCheck.h"
+
+namespace libcpp {
+class proper_version_checks : public clang::tidy::ClangTidyCheck {
+public:
+  proper_version_checks(llvm::StringRef, clang::tidy::ClangTidyContext*);
+  void registerPPCallbacks(const clang::SourceManager& source_manager,
+                           clang::Preprocessor* preprocessor,
+                           clang::Preprocessor* module_expander) override;
+};
+} // namespace libcpp

libcxx/test/tools/clang_tidy_checks/proper_version_checks.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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 "proper_version_checks.hpp"
+
+#include <clang/Lex/Lexer.h>
+#include <clang/Lex/PPCallbacks.h>
+#include <clang/Lex/Preprocessor.h>
+
+namespace libcpp {
+namespace {
+class proper_version_checks_callbacks : public clang::PPCallbacks {
+public:
+  proper_version_checks_callbacks(clang::Preprocessor& preprocessor, clang::tidy::ClangTidyCheck& check)
+      : preprocessor_(preprocessor), check_(check) {}
+
+  void If(clang::SourceLocation location, clang::SourceRange condition_range, ConditionValueKind) override {
+    check_condition(location, condition_range);
+  }
+
+  void Elif(clang::SourceLocation location,
+            clang::SourceRange condition_range,
+            ConditionValueKind,
+            clang::SourceLocation if_location) override {
+    check_condition(location, condition_range);
+  }
+
+private:
+  void check_condition(clang::SourceLocation location, clang::SourceRange condition_range) {
+    std::string_view condition = clang::Lexer::getSourceText(
+        clang::CharSourceRange::getTokenRange(condition_range),
+        preprocessor_.getSourceManager(),
+        preprocessor_.getLangOpts());
+    if (preprocessor_.getSourceManager().isInMainFile(location))
+      return;
+
+    if (condition.starts_with("_LIBCPP_STD_VER") && condition.find(">") != std::string_view::npos &&
+        condition.find(">=") == std::string_view::npos)
+      check_.diag(location, "_LIBCPP_STD_VER >= version should be used instead of _LIBCPP_STD_VER > prev_version");
+
+    else if (condition.starts_with("__cplusplus"))
+      check_.diag(location, "Use _LIBCPP_STD_VER instead of __cplusplus to constrain based on the C++ version");
+
+    else if (condition == "_LIBCPP_STD_VER >= 11")
+      check_.diag(location, "_LIBCPP_STD_VER >= 11 is always true. Did you mean '#ifndef _LIBCPP_CXX03_LANG'?");
+
+    else if (condition.starts_with("_LIBCPP_STD_VER >= ") &&
+             std::ranges::none_of(std::array{"14", "17", "20", "23"}, [&](auto val) {
+               return condition.find(val) != std::string_view::npos;
+             }))
+      check_.diag(location, "Not a valid value for _LIBCPP_STD_VER. Use 14, 17, 20 or 23");
+  }
+
+  clang::Preprocessor& preprocessor_;
+  clang::tidy::ClangTidyCheck& check_;
+};
+} // namespace
+
+proper_version_checks::proper_version_checks(llvm::StringRef name, clang::tidy::ClangTidyContext* context)
+    : clang::tidy::ClangTidyCheck(name, context) {}
+
+void proper_version_checks::registerPPCallbacks(
+    const clang::SourceManager& source_manager,
+    clang::Preprocessor* preprocessor,
+    clang::Preprocessor* module_expander) {
+  preprocessor->addPPCallbacks(std::make_unique<proper_version_checks_callbacks>(*preprocessor, *this));
+}
+
+} // namespace libcpp