[libc++] Reject standard attributes which are extensions in libcpp-uglify-attributes

[philnik]

Summary

This adds a list of attributes which can be pretty to be able to reject attributes which were introduced in a later C++ standard.
Fixes #61196

[philnik]

libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp

72

The old way is fundamentally broken. IMO it's better to produce slightly wrong hints in an edge case instead of never producing hints.

[Mordante]

Since this fixes a bug, should we add regression tests?

libcxx/include/barrier

133

This looks odd, either we are Standard compliant and we can use [[nodiscard]] or this is an extension in some language modi, but we're not using _LIBCPP_NODISCARD_EXT. Do you know why this is needed here?

libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp

50

This count feels extremely error prone, maybe an array per language version. This then has some duplicates but feels easier to maintain. It also allows for the removal of an attribute in a later version of the standard.

72

The TODO sound do. Don't try to do something seems like maintaining the status-quo, while a todo implies a change.

Can you add in the comment what is "fundamentally" broken?

I don't mind not fixing, but then let's state that and not put a TODO. That way the comment doesn't show when grepping for TODOs.

[philnik]

libcxx/include/barrier

133

<barrier> itself is an extension in C++14 and C++17. This is how I noticed that we have them as extensions.

libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp

50

I don't disagree, but one array per version seems also error-prone.

[ldionne]

libcxx/include/barrier

133

Yeah, this is a weird case but I think using __nodiscard__ is right here. We support barrier in C++14 so we can't use nodiscard, but barrier is specified to have nodiscard unconditionally so using _LIBCPP_NODISCARD_EXT also doesn't make sense.

libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp

49

FWIW I would do this:

std::vector<std::string> get_standard_attributes(const clang::LangOptions& lang_opts) {
  std::vector<std::string> attributes = {
    // C++11 attributes
    "noreturn",
    "carries_dependency"
  };

  if (lang_opts.CPlusPlus14)
    attributes.push_back("deprecated");

  ...

  return attributes;
}

Not quite as efficient, but much more straightforward and TBH efficiency here probably doesn't matter.

72

I am not sure I understand what's your preferred resolution here. Do you want a TODO like @philnik did here, or do you want him to instead explain why this is fundamentally broken (i.e. we don't have the right Clang API) and make it a normal comment, but not a TODO?

Personally, I would go with (2) unless there is a proper solution to fix the TODO, in which case I would do it in this patch.

[philnik]

Address comments

[philnik]

libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp

72

I've reworded the TODO for now. I'd really like to fix the problem, so I'd rather keep the TODO.

[Mordante]

LGTM! I added some comments, but only feedback and no changes are needed.

libcxx/include/barrier

133

Thanks for the comments. I still don't like it, but no objection either.

libcxx/test/tools/clang_tidy_checks/uglify_attributes.cpp

29

Interesting I recalled more attributes in C++11, but according to cppreference this is correct.

49

+1 I really like this a lot better.

[rsmith]

There still appear to be problems after this patch. It looks like a test that defines these tokens to "weird" things and then tries including the libc++ headers would have caught these issues.

libcxx/include/__config

1180

This is still broken. We'll fail to compile this header if msvc or no_unique_address is defined as a macro that expands to something other than a single identifier. My understanding is that the direction on the bug was to remove the msvc::no_unique_address handling entirely.

1190

This is also still broken, if no_unique_address is defined as a macro.

[philnik]

In D145508#4252137, @rsmith wrote:

There still appear to be problems after this patch. It looks like a test that defines these tokens to "weird" things and then tries including the libc++ headers would have caught these issues.

That would have caught this specific case, but wouldn't catch using other attributes in their pretty variants. I rather go conservative here, i.e. allow specific attributes instead of trying to ban some. I'll try to add something to also check the preprocessor checks in another patch.

libcxx/include/__config

 
 #  if _LIBCPP_STD_VER >= 23
 #    define _LIBCPP_CONSTEXPR_SINCE_CXX23 constexpr
 #  else
 #    define _LIBCPP_CONSTEXPR_SINCE_CXX23
 #  endif
 
 #  if __has_cpp_attribute(nodiscard)
-#    define _LIBCPP_NODISCARD [[nodiscard]]
+#    define _LIBCPP_NODISCARD [[__nodiscard__]]
 #  else
 // We can't use GCC's [[gnu::warn_unused_result]] and
 // __attribute__((warn_unused_result)), because GCC does not silence them via
 // (void) cast.
 #    define _LIBCPP_NODISCARD
 #  endif
 
 // _LIBCPP_NODISCARD_EXT may be used to apply [[nodiscard]] to entities not

 // The attribute uses 1-based indices for ordinary and static member functions.
 // The attribute uses 2-based indices for non-static member functions.
 #    define _LIBCPP_ATTRIBUTE_FORMAT(archetype, format_string_index, first_format_arg_index)                           \
       __attribute__((__format__(archetype, format_string_index, first_format_arg_index)))
 #  else
 #    define _LIBCPP_ATTRIBUTE_FORMAT(archetype, format_string_index, first_format_arg_index) /* nothing */
 #  endif
 
 #  if __has_cpp_attribute(msvc::no_unique_address)
 // MSVC implements [[no_unique_address]] as a silent no-op currently.
 // (If/when MSVC breaks its C++ ABI, it will be changed to work as intended.)
 // However, MSVC implements [[msvc::no_unique_address]] which does what
 // [[no_unique_address]] is supposed to do, in general.
 
 // Clang-cl does not yet (14.0) implement either [[no_unique_address]] or
 // [[msvc::no_unique_address]] though. If/when it does implement
 // [[msvc::no_unique_address]], this should be preferred though.
 #    define _LIBCPP_NO_UNIQUE_ADDRESS [[msvc::no_unique_address]]
 #  elif __has_cpp_attribute(no_unique_address)
-#    define _LIBCPP_NO_UNIQUE_ADDRESS [[no_unique_address]]
+#    define _LIBCPP_NO_UNIQUE_ADDRESS [[__no_unique_address__]]
 #  else
 #    define _LIBCPP_NO_UNIQUE_ADDRESS /* nothing */
 // Note that this can be replaced by #error as soon as clang-cl
 // implements msvc::no_unique_address, since there should be no C++20
 // compiler that doesn't support one of the two attributes at that point.
 // We generally don't want to use this macro outside of C++20-only code,
 // because using it conditionally in one language version only would make
 // the ABI inconsistent.

libcxx/include/barrier

public:
 
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     __barrier_base(ptrdiff_t __expected, _CompletionF __completion = _CompletionF())
             : __expected_(__expected), __base_(std::__construct_barrier_algorithm_base(this->__expected_),
                                                &__destroy_barrier_algorithm_base),
               __expected_adjustment_(0), __completion_(std::move(__completion)), __phase_(0)
     {
     }
-    [[nodiscard]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
+    [[__nodiscard__]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     arrival_token arrive(ptrdiff_t __update)
     {
         auto const __old_phase = __phase_.load(memory_order_relaxed);
         for(; __update; --__update)
             if(__arrive_barrier_algorithm_base(__base_.get(), __old_phase)) {
                 __completion_();
                 __expected_ += __expected_adjustment_.load(memory_order_relaxed);
                 __expected_adjustment_.store(0, memory_order_relaxed);
public:
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     barrier(ptrdiff_t __count, _CompletionF __completion = _CompletionF())
         : __b_(__count, _VSTD::move(__completion)) {
     }
 
     barrier(barrier const&) = delete;
     barrier& operator=(barrier const&) = delete;
 
-    [[nodiscard]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
+    [[__nodiscard__]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     arrival_token arrive(ptrdiff_t __update = 1)
     {
         return __b_.arrive(__update);
     }
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     void wait(arrival_token&& __phase) const
     {
         __b_.wait(_VSTD::move(__phase));

libcxx/test/tools/clang_tidy_checks/uglify_attributes.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/ClangTidyCheck.h"
 #include "clang-tidy/ClangTidyModuleRegistry.h"
 
 #include "uglify_attributes.hpp"
 
 #include <algorithm>
+#include <array>
+#include <span>
 #include <string_view>
 
 namespace {
 bool isUgly(std::string_view str) {
   if (str.size() < 2)
     return false;
   if (str[0] == '_' && str[1] >= 'A' && str[1] <= 'Z')
     return true;
   return str.find("__") != std::string_view::npos;
 }
 
+std::vector<const char*> get_standard_attributes(const clang::LangOptions& lang_opts) {
+  std::vector<const char*> attributes = {"noreturn", "carries_dependency"};
+
+  if (lang_opts.CPlusPlus14)
+    attributes.emplace_back("deprecated");
+
+  if (lang_opts.CPlusPlus17) {
+    attributes.emplace_back("fallthrough");
+    attributes.emplace_back("nodiscard");
+    attributes.emplace_back("maybe_unused");
+  }
+
+  if (lang_opts.CPlusPlus20) {
+    attributes.emplace_back("likely");
+    attributes.emplace_back("unlikely");
+    attributes.emplace_back("no_unique_address");
+  }
+
+  if (lang_opts.CPlusPlus2b) {
+    attributes.emplace_back("assume");
+  }
+
+  return attributes;
+}
+
 AST_MATCHER(clang::Attr, isPretty) {
   if (Node.isKeywordAttribute())
     return false;
-  if (Node.isCXX11Attribute() && !Node.hasScope()) // TODO: reject standard attributes that are version extensions
+  if (Node.isCXX11Attribute() && !Node.hasScope()) {
+    if (isUgly(Node.getAttrName()->getName()))
       return false;
+    return !llvm::is_contained(
+        get_standard_attributes(Finder->getASTContext().getLangOpts()), Node.getAttrName()->getName());
+  }
   if (Node.hasScope())
     if (!isUgly(Node.getScopeName()->getName()))
       return true;
-
   if (Node.getAttrName())
     return !isUgly(Node.getAttrName()->getName());
 
   return false;
 }
 
 std::optional<std::string> getUglyfiedCXX11Attr(const clang::Attr& attr) {
-  // Don't try to fix attributes with `using` in them.
-  if (std::ranges::search(std::string_view(attr.getSpelling()), std::string_view("::")).empty())
-    return std::nullopt;
+  // TODO: Don't emit FixItHints for attributes with `using` in them or emit correct fixes.
 
   std::string attr_string;
   if (attr.isClangScope())
     attr_string += "_Clang::";
   else if (attr.isGNUScope())
     attr_string += "__gnu__::";
 
   if (!attr.getAttrName()->getName().starts_with("__")) {
uglify_attributes::uglify_attributes(llvm::StringRef name, clang::tidy::ClangTidyContext* context)
     : clang::tidy::ClangTidyCheck(name, context) {}
 
 void uglify_attributes::registerMatchers(clang::ast_matchers::MatchFinder* finder) {
   using namespace clang::ast_matchers;
   finder->addMatcher(attr(isPretty()).bind("normal_attribute"), this);
 }
 
 void uglify_attributes::check(const clang::ast_matchers::MatchFinder::MatchResult& result) {
-  if (const auto* call = result.Nodes.getNodeAs<clang::Attr>("normal_attribute"); call != nullptr) {
-    auto diagnostic = diag(call->getLoc(), "Non-standard attributes should use the _Ugly spelling");
-    auto uglified   = getUglified(*call);
+  if (const auto* attr = result.Nodes.getNodeAs<clang::Attr>("normal_attribute"); attr != nullptr) {
+    auto diagnostic = diag(attr->getLoc(), "Non-standard attributes should use the _Ugly spelling");
+    auto uglified   = getUglified(*attr);
     if (uglified.has_value()) {
-      diagnostic << clang::FixItHint::CreateReplacement(call->getRange(), *uglified);
+      diagnostic << clang::FixItHint::CreateReplacement(attr->getRange(), *uglified);
     }
   }
 }
 } // namespace libcpp