From f138b9d6facb783e7cc7d035e3e14f0d2a92ed5a Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Tue, 23 Sep 2025 09:08:14 -0700 Subject: [PATCH] Annotate ABSL_DIE_IF_NULL's return type with `absl_nonnull` This helps inform Nullability inference without needing any special casing. Since ABSL_DIE_IF_NULL is allowed for pointers, smart pointers, or classes marked with ABSL_NULLABILITY_COMPATIBLE we introduce a trait to help add the annotation only when compatible. This trait is kept internal for now out of caution for what we are supporting, though people have asked about it before (see b/394789178). Simple cases are tested by `-Wnonnull` in absl/base/nullability_nc_test.cc. However, it's unclear how to test the complex cases like templates with universal references with the simple compiler `-Wnonnull`. There is a followup nullability inference test (cl/808830606). Since there is Wnullability-completeness, needed to annotate the rest of die_if_null.h PiperOrigin-RevId: 810463021 Change-Id: Id5156996bf3a29a99e689974ac2af7b94b21c460 --- CMake/AbseilDll.cmake | 1 + absl/base/BUILD.bazel | 26 +++++ absl/base/CMakeLists.txt | 28 ++++++ absl/base/internal/nullability_traits.h | 71 ++++++++++++++ absl/base/internal/nullability_traits_test.cc | 98 +++++++++++++++++++ absl/log/BUILD.bazel | 2 + absl/log/CMakeLists.txt | 2 + absl/log/die_if_null.cc | 4 +- absl/log/die_if_null.h | 27 ++++- 9 files changed, 255 insertions(+), 4 deletions(-) create mode 100644 absl/base/internal/nullability_traits.h create mode 100644 absl/base/internal/nullability_traits_test.cc diff --git a/CMake/AbseilDll.cmake b/CMake/AbseilDll.cmake index a0036492..55bd0542 100644 --- a/CMake/AbseilDll.cmake +++ b/CMake/AbseilDll.cmake @@ -25,6 +25,7 @@ set(ABSL_INTERNAL_DLL_FILES "base/internal/low_level_alloc.cc" "base/internal/low_level_alloc.h" "base/internal/low_level_scheduling.h" + "base/internal/nullability_traits.h" "base/internal/per_thread_tls.h" "base/internal/poison.cc" "base/internal/poison.h" diff --git a/absl/base/BUILD.bazel b/absl/base/BUILD.bazel index 2b380e61..c68883ea 100644 --- a/absl/base/BUILD.bazel +++ b/absl/base/BUILD.bazel @@ -94,6 +94,20 @@ cc_library( ], ) +cc_library( + name = "nullability_traits_internal", + hdrs = ["internal/nullability_traits.h"], + copts = ABSL_DEFAULT_COPTS, + linkopts = ABSL_DEFAULT_LINKOPTS, + visibility = [ + "//absl:__subpackages__", + ], + deps = [ + ":config", + ":nullability", + ], +) + cc_library( name = "raw_logging_internal", srcs = ["internal/raw_logging.cc"], @@ -642,6 +656,18 @@ cc_test( ], ) +cc_test( + name = "nullability_traits_test", + srcs = ["internal/nullability_traits_test.cc"], + deps = [ + ":config", + ":nullability", + ":nullability_traits_internal", + "@googletest//:gtest", + "@googletest//:gtest_main", + ], +) + cc_test( name = "raw_logging_test", srcs = ["raw_logging_test.cc"], diff --git a/absl/base/CMakeLists.txt b/absl/base/CMakeLists.txt index 4eb1390b..9dca9896 100644 --- a/absl/base/CMakeLists.txt +++ b/absl/base/CMakeLists.txt @@ -104,6 +104,34 @@ absl_cc_test( GTest::gtest_main ) +# Internal-only target, do not depend on directly. +absl_cc_library( + NAME + nullability_traits_internal + HDRS + "internal/nullability_traits.h" + COPTS + ${ABSL_DEFAULT_COPTS} + DEPS + absl::config + absl::nullability + PUBLIC +) + +absl_cc_test( + NAME + nullability_traits_test + SRCS + "internal/nullability_traits_test.cc" + COPTS + ${ABSL_TEST_COPTS} + DEPS + absl::config + absl::nullability + absl::nullability_traits_internal + GTest::gtest_main +) + # Internal-only target, do not depend on directly. absl_cc_library( NAME diff --git a/absl/base/internal/nullability_traits.h b/absl/base/internal/nullability_traits.h new file mode 100644 index 00000000..790ec909 --- /dev/null +++ b/absl/base/internal/nullability_traits.h @@ -0,0 +1,71 @@ +// Copyright 2025 The Abseil Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef ABSL_BASE_INTERNAL_NULLABILITY_TRAITS_H_ +#define ABSL_BASE_INTERNAL_NULLABILITY_TRAITS_H_ + +#include + +#include "absl/base/config.h" +#include "absl/base/nullability.h" + +namespace absl { +ABSL_NAMESPACE_BEGIN +namespace base_internal { + +// `value` is true if the type `T` is compatible with nullability annotations +// (is a raw pointer, a smart pointer, or marked with +// ABSL_NULLABILITY_COMPATIBLE). Prefer to use the higher-level +// `AddNonnullIfCompatible` if that is sufficient. +// +// NOTE: This should not be used to detect if the compiler is Clang (since +// Clang is the only compiler that supports nullability annotations). +#if defined(__clang__) && !defined(__OBJC__) && \ + ABSL_HAVE_FEATURE(nullability_on_classes) +template +struct IsNullabilityCompatibleType { + constexpr static bool value = false; +}; + +template +struct IsNullabilityCompatibleType> { + constexpr static bool value = true; +}; +#else +// False when absl_nullable is a no-op (for non-Clang compilers or Objective-C.) +template +struct IsNullabilityCompatibleType { + constexpr static bool value = false; +}; +#endif + +// A trait to add `absl_nonnull` to a type if it is compatible with nullability +// annotations. +template ::value> +struct AddNonnullIfCompatible; + +template +struct AddNonnullIfCompatible { + using type = T; +}; +template +struct AddNonnullIfCompatible { + using type = absl_nonnull T; +}; + +} // namespace base_internal +ABSL_NAMESPACE_END +} // namespace absl + +#endif // ABSL_BASE_INTERNAL_NULLABILITY_TRAITS_H_ diff --git a/absl/base/internal/nullability_traits_test.cc b/absl/base/internal/nullability_traits_test.cc new file mode 100644 index 00000000..2451239c --- /dev/null +++ b/absl/base/internal/nullability_traits_test.cc @@ -0,0 +1,98 @@ +// Copyright 2025 The Abseil Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "absl/base/internal/nullability_traits.h" + +#include +#include + +#include "gtest/gtest.h" +#include "absl/base/config.h" +#include "absl/base/nullability.h" + +namespace absl { +ABSL_NAMESPACE_BEGIN +namespace base_internal { +namespace { + +struct NotSmartPtr { + int* x; +}; + +class ABSL_NULLABILITY_COMPATIBLE MySmartPtr : std::unique_ptr {}; + +// The IsNullabilityCompatibleType trait value can only be true when we define +// `absl_nullable` (isn't a no-op). +#if defined(__clang__) && !defined(__OBJC__) && \ + ABSL_HAVE_FEATURE(nullability_on_classes) +#define EXPECT_TRUE_IF_SUPPORTED EXPECT_TRUE +#else +#define EXPECT_TRUE_IF_SUPPORTED EXPECT_FALSE +#endif + +TEST(NullabilityTraitsTest, IsNullabilityEligibleTypePrimitives) { + EXPECT_FALSE(IsNullabilityCompatibleType::value); + EXPECT_FALSE(IsNullabilityCompatibleType::value); + + EXPECT_TRUE_IF_SUPPORTED(IsNullabilityCompatibleType::value); + EXPECT_TRUE_IF_SUPPORTED(IsNullabilityCompatibleType::value); + EXPECT_TRUE_IF_SUPPORTED(IsNullabilityCompatibleType::value); + EXPECT_TRUE_IF_SUPPORTED(IsNullabilityCompatibleType::value); + EXPECT_TRUE_IF_SUPPORTED(IsNullabilityCompatibleType::value); +} + +TEST(NullabilityTraitsTest, IsNullabilityCompatibleTypeAliases) { + using MyInt = int; + using MyIntPtr = int*; + EXPECT_FALSE(IsNullabilityCompatibleType::value); + EXPECT_TRUE_IF_SUPPORTED(IsNullabilityCompatibleType::value); +} + +TEST(NullabilityTraitsTest, IsNullabilityCompatibleTypeSmartPointers) { + EXPECT_TRUE_IF_SUPPORTED( + IsNullabilityCompatibleType>::value); + EXPECT_TRUE_IF_SUPPORTED( + IsNullabilityCompatibleType>::value); + + EXPECT_FALSE(IsNullabilityCompatibleType::value); + EXPECT_TRUE_IF_SUPPORTED(IsNullabilityCompatibleType::value); + EXPECT_TRUE_IF_SUPPORTED(IsNullabilityCompatibleType::value); +} + +#undef EXPECT_TRUE_IF_SUPPORTED + +TEST(NullabilityTraitsTest, AddNonnullIfCompatiblePassThroughPrimitives) { + EXPECT_TRUE((std::is_same_v::type, int>)); + EXPECT_TRUE((std::is_same_v::type, int*>)); + EXPECT_TRUE( + (std::is_same_v::type, int* const>)); +} + +TEST(NullabilityTraitsTest, AddNonnullIfCompatiblePassThroughSmartPointers) { + EXPECT_TRUE( + (std::is_same_v>::type, + std::unique_ptr>)); + EXPECT_TRUE( + (std::is_same_v>::type, + std::shared_ptr>)); + EXPECT_TRUE( + (std::is_same_v::type, NotSmartPtr>)); + EXPECT_TRUE( + (std::is_same_v::type, MySmartPtr>)); +} + +} // namespace +} // namespace base_internal +ABSL_NAMESPACE_END +} // namespace absl diff --git a/absl/log/BUILD.bazel b/absl/log/BUILD.bazel index 3e965abd..5bc71502 100644 --- a/absl/log/BUILD.bazel +++ b/absl/log/BUILD.bazel @@ -80,6 +80,8 @@ cc_library( ":log", "//absl/base:config", "//absl/base:core_headers", + "//absl/base:nullability", + "//absl/base:nullability_traits_internal", "//absl/strings", ], ) diff --git a/absl/log/CMakeLists.txt b/absl/log/CMakeLists.txt index c972c173..51f399e8 100644 --- a/absl/log/CMakeLists.txt +++ b/absl/log/CMakeLists.txt @@ -466,6 +466,8 @@ absl_cc_library( absl::config absl::core_headers absl::log + absl::nullability + absl::nullability_traits_internal absl::strings PUBLIC ) diff --git a/absl/log/die_if_null.cc b/absl/log/die_if_null.cc index 19c6a28e..0d0b78ed 100644 --- a/absl/log/die_if_null.cc +++ b/absl/log/die_if_null.cc @@ -15,6 +15,7 @@ #include "absl/log/die_if_null.h" #include "absl/base/config.h" +#include "absl/base/nullability.h" #include "absl/log/log.h" #include "absl/strings/str_cat.h" @@ -22,7 +23,8 @@ namespace absl { ABSL_NAMESPACE_BEGIN namespace log_internal { -void DieBecauseNull(const char* file, int line, const char* exprtext) { +void DieBecauseNull(const char* absl_nonnull file, int line, + const char* absl_nonnull exprtext) { LOG(FATAL).AtLocation(file, line) << absl::StrCat("Check failed: '", exprtext, "' Must be non-null"); } diff --git a/absl/log/die_if_null.h b/absl/log/die_if_null.h index 8597976f..ac7dbe68 100644 --- a/absl/log/die_if_null.h +++ b/absl/log/die_if_null.h @@ -23,10 +23,13 @@ #include +#include #include #include "absl/base/attributes.h" #include "absl/base/config.h" +#include "absl/base/internal/nullability_traits.h" +#include "absl/base/nullability.h" #include "absl/base/optimization.h" // ABSL_DIE_IF_NULL() @@ -56,12 +59,30 @@ namespace log_internal { // generates less code than its implementation would if inlined, for a slight // code size reduction each time `ABSL_DIE_IF_NULL` is called. [[noreturn]] ABSL_ATTRIBUTE_NOINLINE void DieBecauseNull( - const char* file, int line, const char* exprtext); + const char* absl_nonnull file, int line, const char* absl_nonnull exprtext); // Helper for `ABSL_DIE_IF_NULL`. + +// Since we use `remove_reference_t` before `AddNonnullIfCompatible`, we need +// to explicitly have overloads for both lvalue reference and rvalue reference +// arguments and returns. template -[[nodiscard]] T DieIfNull(const char* file, int line, const char* exprtext, - T&& t) { +[[nodiscard]] typename absl::base_internal::AddNonnullIfCompatible< + std::remove_reference_t>::type& +DieIfNull(const char* absl_nonnull file, int line, + const char* absl_nonnull exprtext, T& t) { + if (ABSL_PREDICT_FALSE(t == nullptr)) { + // Call a non-inline helper function for a small code size improvement. + DieBecauseNull(file, line, exprtext); + } + return t; +} + +template +[[nodiscard]] typename absl::base_internal::AddNonnullIfCompatible< + std::remove_reference_t>::type&& +DieIfNull(const char* absl_nonnull file, int line, + const char* absl_nonnull exprtext, T&& t) { if (ABSL_PREDICT_FALSE(t == nullptr)) { // Call a non-inline helper function for a small code size improvement. DieBecauseNull(file, line, exprtext);