Fix GCC 15 -Wnull-dereference in public headers; always build with -Werror

.github/workflows/ros2-lyrical.yaml

@@ -20,9 +20,14 @@ jobs:
           # which configures with -DCMAKE_INSTALL_LIBDIR=lib/<multiarch>.
           # AFTER_SCRIPT asserts that the library is still installed to
           # $prefix/lib, as required by ament tooling (see issue #1175).
+          # Release: downstream packages (e.g. Nav2) build our headers with
+          # -Werror on this GCC; -Wnull-dereference only fires with
+          # optimization enabled (see issue #1288).
           - ROS_DISTRO: lyrical
             ROS_REPO: main
-            TARGET_CMAKE_ARGS: -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu
+            TARGET_CMAKE_ARGS: >-
+              -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu
+              -DCMAKE_BUILD_TYPE=Release
             AFTER_SCRIPT: >-
               lib=$(find "$BASEDIR/target_ws/install" -name libbehaviortree_cpp.so);
               echo "libbehaviortree_cpp.so installed at: ${lib:-NOT FOUND}";

CMakeLists.txt

@@ -288,16 +288,27 @@ target_compile_features(${BTCPP_LIBRARY} PUBLIC cxx_std_17)
 
 if(MSVC)
     target_compile_options(${BTCPP_LIBRARY} PRIVATE "/source-charset:utf-8")
-else()
-    if(ENABLE_DEBUG)
-        target_compile_options(${BTCPP_LIBRARY} PRIVATE -Wall -Wextra -g3 -ggdb3 -O0 -fno-omit-frame-pointer)
-    else()
-        target_compile_options(${BTCPP_LIBRARY} PRIVATE -Wall -Wextra)
-    endif()
+elseif(ENABLE_DEBUG)
+    target_compile_options(${BTCPP_LIBRARY} PRIVATE -g3 -ggdb3 -O0 -fno-omit-frame-pointer)
 endif()
 
 add_library(BT::${BTCPP_LIBRARY} ALIAS ${BTCPP_LIBRARY})
 
+# Downstream projects (e.g. Nav2) compile our public headers with -Werror, so
+# the library and every target below that instantiates them must be
+# warning-free (issue #1288). -Wnull-dereference only fires with optimization.
+# Set after the 3rdparty subdirectories, so vendored code is not affected.
+if(MSVC)
+    # C4702 (unreachable code) is a code-generation warning that fires on
+    # instantiation-dependent `if constexpr` tails; GCC/Clang have no
+    # equivalent in -Wall -Wextra.
+    set(BTCPP_WARNING_FLAGS /W4 /WX /permissive- /wd4702)
+else()
+    set(BTCPP_WARNING_FLAGS -Wall -Wextra -Wpedantic -Wnull-dereference -Werror)
+endif()
+target_compile_options(${BTCPP_LIBRARY} PRIVATE ${BTCPP_WARNING_FLAGS})
+add_compile_options(${BTCPP_WARNING_FLAGS})
+
 # Add fuzzing targets
 if(ENABLE_FUZZING)
     add_fuzzing_targets()

examples/ex03_sqlite_log.cpp

@@ -113,7 +113,7 @@ int main()
   // Write some data (from the blackboard) and write into the
   // extra column called "extra_data". We will use JSON serialization
 
-  auto meta_callback = [&](BT::Duration timestamp, const BT::TreeNode& node,
+  auto meta_callback = [&](BT::Duration /*timestamp*/, const BT::TreeNode& node,
                            BT::NodeStatus prev_status,
                            BT::NodeStatus status) -> std::string {
     if(prev_status == BT::NodeStatus::RUNNING && BT::isStatusCompleted(status))

examples/t10_observer.cpp

@@ -52,7 +52,7 @@ int main()
   // a certain set of transitions happened as expected
   BT::TreeObserver observer(tree);
 
-  std::map<int, std::string> UID_to_path;
+  std::map<uint16_t, std::string> UID_to_path;
 
   // Print the unique ID and the corresponding human readable path
   // Path is also expected to be unique.
@@ -64,7 +64,7 @@ int main()
   tree.tickWhileRunning();
 
   // You can access a specific statistic, using is full path or the UID
-  const auto& last_action_stats = observer.getStatistics("last_action");
+  [[maybe_unused]] const auto& last_action_stats = observer.getStatistics("last_action");
   assert(last_action_stats.transitions_count > 0);
 
   std::cout << "----------------" << std::endl;

fuzzing/bb_fuzzer.cpp

@@ -4,6 +4,7 @@
 #include <iostream>
 #include <memory>
 #include <thread>
+#include <tuple>
 #include <vector>
 
 #include <fuzzer/FuzzedDataProvider.h>
@@ -94,7 +95,7 @@ class BlackboardFuzzer
           break;
         case 6: {
           // Try to get non-existent key
-          bb->get<int>(generateKey());
+          std::ignore = bb->get<int>(generateKey());
           break;
         }
       }
@@ -112,16 +113,16 @@ class BlackboardFuzzer
             bb->unset(existing_key);
             break;
           case 1:
-            bb->getEntry(existing_key);
+            std::ignore = bb->getEntry(existing_key);
             break;
           case 2:
-            bb->get<int>(existing_key);
+            std::ignore = bb->get<int>(existing_key);
             break;
           case 3:
-            bb->get<double>(existing_key);
+            std::ignore = bb->get<double>(existing_key);
             break;
           case 4:
-            bb->get<std::string>(existing_key);
+            std::ignore = bb->get<std::string>(existing_key);
             break;
         }
       }

fuzzing/bt_fuzzer.cpp

@@ -2,6 +2,7 @@
 #include "behaviortree_cpp/xml_parsing.h"
 
 #include <string>
+#include <tuple>
 
 #include <fuzzer/FuzzedDataProvider.h>
 
@@ -78,7 +79,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)
       std::string random_xml = fdp.ConsumeRandomLengthString(size - 1);
       try
       {
-        factory.createTreeFromText(random_xml);
+        std::ignore = factory.createTreeFromText(random_xml);
       }
       catch(const std::exception&)
       {}
@@ -105,7 +106,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)
       switch(fdp.ConsumeIntegralInRange<int>(0, 2))
       {
         case 0:
-          factory.createTreeFromText(xml, blackboard);
+          std::ignore = factory.createTreeFromText(xml, blackboard);
           break;
         case 1:
           BT::VerifyXML(xml, {});
@@ -114,7 +115,8 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)
           factory.registerBehaviorTreeFromText(xml);
           if(!factory.registeredBehaviorTrees().empty())
           {
-            factory.createTree(factory.registeredBehaviorTrees().front(), blackboard);
+            std::ignore =
+                factory.createTree(factory.registeredBehaviorTrees().front(), blackboard);
           }
           break;
       }

fuzzing/script_fuzzer.cpp

@@ -50,7 +50,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size)
 
           if(result.isNumber())
           {
-            volatile auto num = result.cast<double>();
+            [[maybe_unused]] volatile auto num = result.cast<double>();
           }
 
           env.vars->set("result", result);

include/behaviortree_cpp/bt_factory.h

@@ -65,7 +65,7 @@ inline TreeNodeManifest CreateManifest(const std::string& ID,
 #endif
 
 #else
-#define BTCPP_EXPORT static
+#define BTCPP_EXPORT [[maybe_unused]] static
 #endif
 /* Use this macro to automatically register one or more custom Nodes
 * into a factory. For instance:
@@ -88,7 +88,7 @@ inline TreeNodeManifest CreateManifest(const std::string& ID,
   BTCPP_EXPORT void BT_RegisterNodesFromPlugin(BT::BehaviorTreeFactory& factory)
 // NOLINTEND(cppcoreguidelines-macro-usage,bugprone-macro-parentheses)
 
-constexpr const char* PLUGIN_SYMBOL = "BT_RegisterNodesFromPlugin";
+inline constexpr const char* PLUGIN_SYMBOL = "BT_RegisterNodesFromPlugin";
 
 bool WildcardMatch(const std::string& str, StringView filter);
 

include/behaviortree_cpp/contrib/expected.hpp

@@ -2225,7 +2225,6 @@ class expected
             ? ( contained.value() )
             : ( error_traits<error_type>::rethrow( contained.error() ), contained.value() );
     }
-    nsel_RESTORE_MSVC_WARNINGS()
 
 #if !nsel_COMPILER_GNUC_VERSION || nsel_COMPILER_GNUC_VERSION >= 490
 
@@ -2244,6 +2243,8 @@ class expected
     }
 
 #endif
+    // BT.CPP: restore moved below the && overloads, which hit C4702 too.
+    nsel_RESTORE_MSVC_WARNINGS()
 
     constexpr error_type const & error() const &
     {

include/behaviortree_cpp/json_export.h

@@ -17,6 +17,22 @@
 namespace BT
 {
 
+namespace details
+{
+// Reference to the value of type T stored in entry; throws instead of
+// dereferencing nullptr when entry is empty or holds another type.
+template <typename T>
+const T& anyRef(const BT::Any& entry)
+{
+  const T* ptr = const_cast<BT::Any&>(entry).castPtr<T>();
+  if(ptr == nullptr)
+  {
+    throw LogicError("JsonExporter: BT::Any does not contain a ", demangle(typeid(T)));
+  }
+  return *ptr;
+}
+}  // namespace details
+
 /**
 *  To add new type to the JSON library, you should follow these instructions:
 *    https://json.nlohmann.me/features/arbitrary_types/
@@ -181,7 +197,7 @@
   type_names_.insert({ BT::demangle(typeid(T)), BT::TypeInfo::Create<T>() });
 
   ToJonConverter to_converter = [](const BT::Any& entry, nlohmann::json& dst) {
-    dst = *const_cast<BT::Any&>(entry).castPtr<T>();
+    dst = details::anyRef<T>(entry);
   };
   to_json_converters_.insert({ typeid(T), to_converter });
 
@@ -194,7 +210,7 @@
 
   //---- include vectors of T
   ToJonConverter to_array_converter = [](const BT::Any& entry, nlohmann::json& dst) {
-    dst = *const_cast<BT::Any&>(entry).castPtr<std::vector<T>>();
+    dst = details::anyRef<std::vector<T>>(entry);
   };
   to_json_converters_.insert({ typeid(std::vector<T>), to_array_converter });
 
@@ -225,7 +241,7 @@
   //---------------------------------------------
   // add the vector<T> converter (must be created before moving converter)
   auto vector_converter = [converter](const BT::Any& entry, nlohmann::json& json) {
-    auto& vec = *const_cast<BT::Any&>(entry).castPtr<std::vector<T>>();
+    const auto& vec = details::anyRef<std::vector<T>>(entry);
     for(const auto& item : vec)
     {
       nlohmann::json item_json;

include/behaviortree_cpp/utils/safe_any.hpp

@@ -265,8 +265,7 @@ inline bool ValidCast(const SRC& val)
         {
           return false;
         }
-        SRC back_conv = static_cast<SRC>(as_float);
-        return back_conv == val;
+        // the round-trip check below completes the test
       }
     }
     // Handle conversion to integral

include/behaviortree_cpp/utils/thread_safe_signal.h

@@ -45,12 +45,13 @@ class ThreadSafeSignal
 
   void notify(CallableArgs... args)
   {
-    std::vector<std::weak_ptr<Subscription>> subscribers;
-    {
+    // Copy-construct the snapshot: copy-assigning the vector triggers a
+    // -Wnull-dereference false positive in GCC 15 at -O2 (issue #1288).
+    const auto subscribers = [this] {
       const std::scoped_lock lock(mutex_);
       removeExpired();
-      subscribers = subscribers_;
-    }
+      return subscribers_;
+    }();
     for(const auto& weak_subscriber : subscribers)
     {
       if(auto subscriber = weak_subscriber.lock())

src/basic_types.cpp

@@ -166,7 +166,7 @@ T ConvertWithBoundCheck(StringView str)
     throw RuntimeError(
         StrCat("Value out of bound when converting [", str, "] to integer"));
   }
-  return res;
+  return static_cast<T>(res);
 }
 }  // namespace
 

src/loggers/abstract_logger.cpp

@@ -90,7 +90,7 @@ void StatusChangeLogger::handleStatusChange(TimePoint timestamp, const TreeNode&
   // Copy state under lock, then release before calling user code
   // This prevents recursive mutex locking when multiple nodes change status
   bool should_callback = false;
-  Duration adjusted_timestamp;
+  Duration adjusted_timestamp{};
   {
     std::unique_lock lk(_p->callback_mutex);
     if(_p->enabled && (status != NodeStatus::IDLE || _p->show_transition_to_idle))

src/loggers/bt_file_logger_v2.cpp

@@ -113,7 +113,7 @@ void FileLogger2::callback(Duration timestamp, const TreeNode& node,
   Transition trans{};
   trans.timestamp_usec = uint64_t(ToUsec(timestamp - _p->first_timestamp));
   trans.node_uid = node.UID();
-  trans.status = static_cast<uint64_t>(status);
+  trans.status = static_cast<uint8_t>(status);
   {
     const std::scoped_lock lock(_p->queue_mutex);
     _p->transitions_queue.push_back(trans);

src/loggers/bt_sqlite_logger.cpp

@@ -185,7 +185,7 @@ void SqliteLogger::callback(Duration timestamp, const TreeNode& node,
     const std::scoped_lock lk(queue_mutex_);
     monotonic_timestamp_ = std::max(monotonic_timestamp_ + 1, tm_usec);
 
-    long elapsed_time = 0;
+    int64_t elapsed_time = 0;
 
     if(prev_status == NodeStatus::IDLE && status == NodeStatus::RUNNING)
     {

src/xml_parsing.cpp

@@ -297,10 +297,6 @@ struct XMLParser::PImpl
   void loadSubtreeModel(const XMLElement* xml_root);
 };
 
-#if defined(__linux) || defined(__linux__)
-#pragma GCC diagnostic pop
-#endif
-
 XMLParser::XMLParser(const BehaviorTreeFactory& factory) : _p(new PImpl(factory))
 {}
 
@@ -904,7 +900,7 @@ TreeNode::Ptr XMLParser::PImpl::createNodeFromXML(const XMLElement* element,
             {
               port_model.converter()(port_value);
             }
-            catch(std::exception& ex)
+            catch(const std::exception&)
             {
               auto msg = StrCat("The port with name \"", port_name, "\" and value \"",
                                 port_value, "\" can not be converted to ",