Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[libcxx] improves diagnostics for containers with bad value types #106296

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

[libcxx] improves diagnostics for containers with bad value types #106296

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
5529499
[libcxx] improves diagnostics for containers with bad value types
cjdb Aug 26, 2024
1e6b81b
rewords diagnostics per internal feedback and adds arrays
cjdb Aug 29, 2024
6c1e1e3
formats files
cjdb Aug 29, 2024
938299d
sorts includes missed by clang-format
cjdb Aug 29, 2024
65346a3
more include sorting
cjdb Aug 29, 2024
472ffcf
more formatting, with a successful git clang-format
cjdb Aug 29, 2024
7596a2e
excludes <array> test from C++03
cjdb Aug 29, 2024
25e999f
suppresses irrelevant diagnostic
cjdb Aug 29, 2024
5a1b902
s/__is_unbounded_array(T)/__libcpp_is_unbounded_array<T>::value/g
cjdb Aug 29, 2024
99a4509
applies Louis' request
cjdb Aug 29, 2024
45c85d6
finally gets git-clang-format on side
cjdb Aug 29, 2024
dbf872a
unifies the containers' diagnostics
cjdb Aug 30, 2024
e596964
Update libcxx/include/__type_traits/diagnostic_utilities.h
cjdb Sep 3, 2024
e2d65c0
removes extraneous headers, adds macro to `std::allocator`
cjdb Sep 3, 2024
0487281
applies clang-format
cjdb Sep 3, 2024
6d9ce9e
changes using `is_array` to `is_bounded_array`
cjdb Sep 3, 2024
c49b7e8
reduces the number of trait instantiations
cjdb Sep 5, 2024
eb2fd68
removes commented out code
cjdb Sep 6, 2024
55a2e7a
Merge branch 'main' into cleaner-libcxx-diagnostics
cjdb Sep 6, 2024
d9043c4
post-sync clang-format
cjdb Sep 6, 2024
ab41024
responds to red CI
cjdb Sep 6, 2024
7bd03b3
responds to red CI
cjdb Sep 6, 2024
ef9d9c3
responds to red CI
cjdb Sep 6, 2024
f0f88d7
responds to red CI
cjdb Sep 7, 2024
bb82c95
replaces TODOs
cjdb Sep 7, 2024
33a452d
Revert "replaces TODOs"
cjdb Sep 8, 2024
6981a2a
Update libcxx/include/__type_traits/diagnostic_utilities.h
cjdb Sep 9, 2024
9fe3f4a
Update libcxx/include/__type_traits/diagnostic_utilities.h
cjdb Sep 9, 2024
b97942e
separates the cv-unqualified object requirement for other diganostics
cjdb Sep 10, 2024
02ffa16
fixes asan diagnostic
cjdb Sep 11, 2024
f75d1ae
Merge branch 'main' into cleaner-libcxx-diagnostics
cjdb Sep 16, 2024
20e0996
Merge branch 'main' into cleaner-libcxx-diagnostics
cjdb Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions libcxx/include/__memory/allocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@
#include <__memory/allocator_traits.h>
#include <__type_traits/is_const.h>
#include <__type_traits/is_constant_evaluated.h>
#include <__type_traits/is_function.h>
#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_void.h>
#include <__type_traits/is_volatile.h>
#include <__type_traits/remove_reference.h>
cjdb marked this conversation as resolved.
Show resolved Hide resolved
#include <__utility/forward.h>
#include <cstddef>
#include <new>
Expand Down Expand Up @@ -76,8 +79,16 @@ struct __non_trivial_if<true, _Unique> {

template <class _Tp>
class _LIBCPP_TEMPLATE_VIS allocator : private __non_trivial_if<!is_void<_Tp>::value, allocator<_Tp> > {
static_assert(!is_const<_Tp>::value, "std::allocator does not support const types");
static_assert(!is_volatile<_Tp>::value, "std::allocator does not support volatile types");
static_assert(!is_const<_Tp>::value, "'std::allocator' can only allocate non-const object types");
static_assert(!is_volatile<_Tp>::value, "'std::allocator' can only allocate non-volatile object types");
static_assert(!is_reference<_Tp>::value || !is_function<typename remove_reference<_Tp>::type>::value,
"'std::allocator' can only allocate object types; function references are not objects (consider using "
"a function pointer)");
static_assert(!is_reference<_Tp>::value,
"'std::allocator' can only allocate object types; references are not objects");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just check is_same<remove_cvref_t<_Tp>, _Tp> instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That erases the tailored diagnostics; noting references are not objects, etc., are hints I that believe significantly improves UX.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Folks internally suggested that I change the text to the following, as they feel it'll be more helpful than what I'm currently proposing.

  • std::allocator<T const>: 'std::allocator' cannot allocate const objects (return to status quo)
  • std::allocator<T volatile>: 'std::allocator' cannot allocate volatile objects (return to status quo)
  • std::allocator<T&>: 'std::allocator' cannot allocate references
  • std::allocator<T()>: 'std::allocator' cannot allocate functions

We do lose the function reference suggestion, but I'm okay with doing that for something that's more helpful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply "cannot allocate cv or reference qualified objects"? I'm not sure how much of an improvement it is to specify the qualifier explicitly, since it's already shown in the diagnostic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kicked the internal discussion off by asking if the average C++ programmer would be able to understand 'std::vector' can only allocate non-cv-qualified object types, and the feedback I've received can be aggregated as roughly

  • most C++ programmers don't know what the term "cv-qualified" means
  • we should only mention relevant information, and mentioning "volatile" when volatile isn't used was repeatedly called out in particular
  • most C++ programmers probably aren't going to know that "object" has a very different meaning to other languages
  • most non-toolchain folks in the discussion seemed to prefer separate messages; toolchain folks were sort of ambivalent

What do you feel a single check will offer over independent checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your concern with respect to the number of instantiations?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'm concerned that we instantiate a bunch of types for the rare case they are actually required for diagnostic purposes. Every instantiation takes time and memory, and for a type that is used everywhere, like vector, we should keep unnecessary instantiations as low as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds to me like you're concerned about build times. Have you run any preliminary analysis to determine that there is a noticeable impact?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more a death by a thousand cuts situation than something significant I think. If we add this many checks everywhere people will definitely notice, but not if we just add it in a single place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point, I agree. I think I've got something that bridges the gap between what you suggested above, and what I was originally hoping to land.

static_assert(
!is_function<_Tp>::value,
"'std::allocator' can only allocate object types; functions are not objects (consider using a function pointer)");

public:
typedef size_t size_type;
Expand Down
9 changes: 9 additions & 0 deletions libcxx/include/array
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,15 @@ template <size_t I, class T, size_t N> const T&& get(const array<T, N>&&) noexce
#include <__type_traits/is_array.h>
#include <__type_traits/is_const.h>
#include <__type_traits/is_constructible.h>
#include <__type_traits/is_function.h>
#include <__type_traits/is_nothrow_constructible.h>
#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_swappable.h>
#include <__type_traits/is_trivially_relocatable.h>
#include <__type_traits/is_void.h>
#include <__type_traits/remove_cv.h>
#include <__type_traits/remove_reference.h>
cjdb marked this conversation as resolved.
Show resolved Hide resolved
#include <__utility/empty.h>
#include <__utility/integer_sequence.h>
#include <__utility/move.h>
Expand Down Expand Up @@ -167,6 +171,11 @@ _LIBCPP_BEGIN_NAMESPACE_STD

template <class _Tp, size_t _Size>
struct _LIBCPP_TEMPLATE_VIS array {
static_assert(!is_reference<_Tp>::value || !is_function<typename remove_reference<_Tp>::type>::value, "'std::array' can only hold object types; function references are not objects (consider using a function pointer)");
static_assert(!is_reference<_Tp>::value, "'std::array' can only hold object types; references are not objects");
static_assert(!is_function<_Tp>::value, "'std::array' can only hold object types; functions are not objects (consider using a function pointer)");
static_assert(!is_void<_Tp>::value, "'std::array' can only hold object types; 'void' is not an object");

using __trivially_relocatable = __conditional_t<__libcpp_is_trivially_relocatable<_Tp>::value, array, void>;

// types:
Expand Down
12 changes: 12 additions & 0 deletions libcxx/include/deque
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,9 +212,15 @@ template <class T, class Allocator, class Predicate>
#include <__ranges/size.h>
#include <__split_buffer>
#include <__type_traits/is_allocator.h>
#include <__type_traits/is_const.h>
#include <__type_traits/is_convertible.h>
#include <__type_traits/is_function.h>
#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_swappable.h>
#include <__type_traits/is_void.h>
#include <__type_traits/is_volatile.h>
#include <__type_traits/remove_reference.h>
#include <__type_traits/type_identity.h>
#include <__utility/forward.h>
#include <__utility/move.h>
Expand Down Expand Up @@ -468,6 +474,12 @@ const _DiffType __deque_iterator<_ValueType, _Pointer, _Reference, _MapPointer,

template <class _Tp, class _Allocator /*= allocator<_Tp>*/>
class _LIBCPP_TEMPLATE_VIS deque {
static_assert(!is_const<_Tp>::value, "'std::deque' can only hold non-const types");
static_assert(!is_volatile<_Tp>::value, "'std::deque' can only hold non-volatile types");
static_assert(!is_reference<_Tp>::value || !is_function<typename remove_reference<_Tp>::type>::value, "'std::deque' can only hold object types; function references are not objects (consider using a function pointer)");
static_assert(!is_reference<_Tp>::value, "'std::deque' can only hold object types; references are not objects");
static_assert(!is_function<_Tp>::value, "'std::deque' can only hold object types; functions are not objects (consider using a function pointer)");
static_assert(!is_void<_Tp>::value, "'std::deque' can only hold object types; 'void' is not an object");
public:
// types:

Expand Down
133 changes: 133 additions & 0 deletions libcxx/include/forward_list
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,11 +220,16 @@ template <class T, class Allocator, class Predicate>
#include <__type_traits/conditional.h>
#include <__type_traits/is_allocator.h>
#include <__type_traits/is_const.h>
#include <__type_traits/is_function.h>
#include <__type_traits/is_nothrow_assignable.h>
#include <__type_traits/is_nothrow_constructible.h>
#include <__type_traits/is_pointer.h>
#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_swappable.h>
#include <__type_traits/is_void.h>
#include <__type_traits/is_volatile.h>
#include <__type_traits/remove_reference.h>
#include <__type_traits/type_identity.h>
#include <__utility/forward.h>
#include <__utility/move.h>
Expand Down Expand Up @@ -635,8 +640,136 @@ void __forward_list_base<_Tp, _Alloc>::clear() _NOEXCEPT {
__before_begin()->__next_ = nullptr;
}

// begin-diagnostic-helpers
// std::forward_list can only have non-cv-qualified object types as its value type, which we diagnose.
// In order to short-cirucit redundant (and cryptic) diagnostics, std::forward_list must be the one
// to fire the static_assert. Since std::forward_list inherits from __forward_list_base, we also need
// to create specialisations for the rejected types, so that everything appears "good" to std::forward_list
// (otherwise we'll get lots of unhelpful diagnostics that suppress the one std::forward_list offers).
template <class _Tp, class _Alloc>
class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp const, _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};

template <class _Tp, class _Alloc>
class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp volatile, _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};

template <class _Tp, class _Alloc>
class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp&, _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};

template <class _Tp, class _Alloc>
class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp&&, _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};

template <class _Tp, class... _Args, class _Alloc>
class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp(_Args...), _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};

template <class _Alloc>
class _LIBCPP_TEMPLATE_VIS __forward_list_base<void, _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};
// end-diagnostic-helpers

template <class _Tp, class _Alloc /*= allocator<_Tp>*/>
class _LIBCPP_TEMPLATE_VIS forward_list : private __forward_list_base<_Tp, _Alloc> {
static_assert(!is_const<_Tp>::value, "'std::forward_list' can only hold non-const types");
cjdb marked this conversation as resolved.
Show resolved Hide resolved
static_assert(!is_volatile<_Tp>::value, "'std::forward_list' can only hold non-volatile types");
static_assert(!is_reference<_Tp>::value || !is_function<typename remove_reference<_Tp>::type>::value, "'std::forward_list' can only hold object types; function references are not objects (consider using a function pointer)");
static_assert(!is_reference<_Tp>::value, "'std::forward_list' can only hold object types; references are not objects");
static_assert(!is_function<_Tp>::value, "'std::forward_list' can only hold object types; functions are not objects (consider using a function pointer)");
static_assert(!is_void<_Tp>::value, "'std::forward_list' can only hold object types; 'void' is not an object");

typedef __forward_list_base<_Tp, _Alloc> base;
typedef typename base::__node_allocator __node_allocator;
typedef typename base::__node_type __node_type;
Expand Down
134 changes: 134 additions & 0 deletions libcxx/include/list
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,16 @@ template <class T, class Allocator, class Predicate>
#include <__ranges/from_range.h>
#include <__type_traits/conditional.h>
#include <__type_traits/is_allocator.h>
#include <__type_traits/is_const.h>
#include <__type_traits/is_function.h>
#include <__type_traits/is_nothrow_assignable.h>
#include <__type_traits/is_nothrow_constructible.h>
#include <__type_traits/is_pointer.h>
#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_void.h>
#include <__type_traits/is_volatile.h>
#include <__type_traits/remove_reference.h>
#include <__type_traits/type_identity.h>
#include <__utility/forward.h>
#include <__utility/move.h>
Expand Down Expand Up @@ -659,8 +665,136 @@ void __list_imp<_Tp, _Alloc>::swap(__list_imp& __c)
__c.__end_.__prev_->__next_ = __c.__end_.__next_->__prev_ = __c.__end_as_link();
}

// begin-diagnostic-helpers
// std::list can only have non-cv-qualified object types as its value type, which we diagnose. In order
// to short-cirucit redundant (and cryptic) diagnostics, std::list must be the one to fire the static_assert.
// Since std::list inherits from __list_imp, we also need to create specialisations for the rejected types,
// so that everything appears "good" to std::list (otherwise we'll get lots of unhelpful diagnostics that
// suppress the one std::list offers).
template <class _Tp, class _Alloc>
class _LIBCPP_TEMPLATE_VIS __list_imp<_Tp const, _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};

template <class _Tp, class _Alloc>
class _LIBCPP_TEMPLATE_VIS __list_imp<_Tp volatile, _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};

template <class _Tp, class _Alloc>
class _LIBCPP_TEMPLATE_VIS __list_imp<_Tp&, _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};

template <class _Tp, class _Alloc>
class _LIBCPP_TEMPLATE_VIS __list_imp<_Tp&&, _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};

template <class _Tp, class... _Args, class _Alloc>
class _LIBCPP_TEMPLATE_VIS __list_imp<_Tp(_Args...), _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};

template <class _Alloc>
class _LIBCPP_TEMPLATE_VIS __list_imp<void, _Alloc> {
public:
using __node_allocator = void*;
using __node_alloc_traits = void*;
using __node_pointer = void*;
using __node_type = void*;
using __node_base = void*;
using __node_base_pointer = void*;
using __link_pointer = void*;

using pointer = int*;
using const_pointer = int const*;
using size_type = unsigned int;
using difference_type = int;
using iterator = int*;
using const_iterator = int const*;
};
// end-diagnostic-helpers

template <class _Tp, class _Alloc /*= allocator<_Tp>*/>
class _LIBCPP_TEMPLATE_VIS list : private __list_imp<_Tp, _Alloc> {
static_assert(!is_const<_Tp>::value, "'std::list' can only hold non-const types");
static_assert(!is_volatile<_Tp>::value, "'std::list' can only hold non-volatile types");
static_assert(!is_reference<_Tp>::value || !is_function<typename remove_reference<_Tp>::type>::value, "'std::list' can only hold object types; function references are not objects (consider using a function pointer)");
static_assert(!is_reference<_Tp>::value, "'std::list' can only hold object types; references are not objects");
static_assert(!is_function<_Tp>::value, "'std::list' can only hold object types; functions are not objects (consider using a function pointer)");
static_assert(!is_void<_Tp>::value, "'std::list' can only hold object types; 'void' is not an object");

typedef __list_imp<_Tp, _Alloc> base;
typedef typename base::__node_type __node_type;
typedef typename base::__node_allocator __node_allocator;
Expand Down
Loading
Loading