Skip to content

Commit 91e2cc5

Browse files
yuxuanchen1997meta-codesync[bot]
authored andcommitted
Fix IFNDR circular dependency in not_null conversion operator SFINAE
Summary: The `is_not_null_castable` and `is_not_null_move_castable` traits used `std::integral_constant<bool, A && B>` where B contains `std::is_convertible<not_null<PtrT>&, ToT>`. Since template arguments to `std::integral_constant` are eagerly instantiated regardless of short-circuit semantics, checking `std::is_convertible<not_null<PtrT>, U>` would instantiate `operator U()`, whose SFINAE constraint instantiates `is_not_null_castable`, which recursively instantiates `std::is_convertible<not_null<PtrT>, U>`, causing unbounded template recursion. This manifests under libc++ when libraries like `nlohmann::json` perform exhaustive SFINAE probing of `is_convertible` for arbitrary type combinations. The fix replaces `std::integral_constant<bool, A && B>` with `std::conjunction<A, B>`. Per [meta.logical], `std::conjunction` implements short-circuit instantiation: if any argument is `false_type`, subsequent arguments are not instantiated. This breaks the cycle since when `is_convertible<PtrT&, ToT>` yields false_type, the second trait containing the recursive `is_convertible<not_null<...>, ToT>` check is never instantiated. Note: D74599633 attempted to fix this. However, the `check_constraint_if_not wrapper` in `operator U()` only prevents instantiation of `is_not_null_castable` when U is the same type as PtrT: template < typename U, typename = std::enable_if_t<detail::check_constraint_if_not< std::is_same_v<PtrT, folly::remove_cvref_t<U>>, // Only skips when U == PtrT detail::is_not_null_castable>::template apply<PtrT, U>>> operator U() const& ... The problem is the recursion happens for types other than `PtrT`. The `check_constraint_if_not mechanism` only breaks the cycle for the `U == Ptr`T case (which would cause ambiguity with `operator const PtrT&()`). It doesn't help when U is an unrelated type like `string_view`. Reviewed By: yfeldblum Differential Revision: D92435995 fbshipit-source-id: f8af523c5f323ef26e2f333ccff2d1025ffa5f2a
1 parent 7c5920a commit 91e2cc5

1 file changed

Lines changed: 23 additions & 16 deletions

File tree

third-party/folly/src/folly/memory/not_null-inl.h

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -88,26 +88,33 @@ struct secret_guaranteed_not_null : guaranteed_not_null_provider {
8888
// - It must not already be castable, otherwise the compiler will raise an
8989
// ambiguity error
9090
// - PtrT must be castable to ToT
91+
//
92+
// NOTE: We use std::conjunction for short-circuit evaluation to avoid infinite
93+
// template recursion. With std::integral_constant<bool, A && B>, both operands
94+
// are instantiated even when A is false. When external code checks
95+
// std::is_convertible<not_null<PtrT>, U>, this instantiates operator U(),
96+
// whose SFINAE constraint instantiates is_not_null_castable, which would check
97+
// std::is_convertible<not_null<PtrT>, U> again, causing unbounded recursion.
98+
// std::conjunction short-circuits: if the first trait is false_type, the second
99+
// trait is not instantiated, breaking the cycle.
91100
template <typename FromPtrT, typename ToT>
92101
struct is_not_null_castable
93-
: std::integral_constant<
94-
bool,
95-
std::is_convertible_v<const FromPtrT&, ToT> &&
96-
!std::is_convertible_v<
97-
// No need to specialize based on null handler as it doesn't
98-
// affect the result.
99-
const not_null<FromPtrT, default_null_handler>&,
100-
ToT>> {};
102+
: std::conjunction<
103+
std::is_convertible<const FromPtrT&, ToT>,
104+
std::negation<std::is_convertible<
105+
// No need to specialize based on null handler as it doesn't
106+
// affect the result.
107+
const not_null<FromPtrT, default_null_handler>&,
108+
ToT>>> {};
101109
template <typename FromPtrT, typename ToT>
102110
struct is_not_null_move_castable
103-
: std::integral_constant<
104-
bool,
105-
std::is_convertible_v<FromPtrT&&, ToT> &&
106-
!std::is_convertible_v<
107-
// No need to specialize based on null handler as it doesn't
108-
// affect the result.
109-
not_null<FromPtrT, default_null_handler>&&,
110-
ToT>> {};
111+
: std::conjunction<
112+
std::is_convertible<FromPtrT&&, ToT>,
113+
std::negation<std::is_convertible<
114+
// No need to specialize based on null handler as it doesn't
115+
// affect the result.
116+
not_null<FromPtrT, default_null_handler>&&,
117+
ToT>>> {};
111118

112119
template <typename T, typename = decltype(*std::declval<T*>() == nullptr)>
113120
inline std::true_type is_comparable_to_nullptr_fn(const T&) {

0 commit comments

Comments
 (0)