Propagate nullability (#4959)

Author: gchatelet

ortools/math_opt/core/BUILD.bazel

@@ -210,6 +210,7 @@ cc_library(
         "//ortools/math_opt:result_cc_proto",
         "//ortools/util:solve_interrupter",
         "@abseil-cpp//absl/base:core_headers",
+        "@abseil-cpp//absl/base:nullability",
         "@abseil-cpp//absl/log",
         "@abseil-cpp//absl/log:check",
         "@abseil-cpp//absl/log:die_if_null",
@@ -455,6 +456,7 @@ cc_library(
         "//ortools/math_opt:parameters_cc_proto",
         "//ortools/math_opt:result_cc_proto",
         "//ortools/util:solve_interrupter",
+        "@abseil-cpp//absl/base:nullability",
         "@abseil-cpp//absl/status:statusor",
         "@abseil-cpp//absl/strings",
         "@abseil-cpp//absl/strings:string_view",

ortools/math_opt/core/base_solver.h

@@ -19,6 +19,7 @@
 #include <string>
 #include <vector>
 
+#include "absl/base/nullability.h"
 #include "absl/status/statusor.h"
 #include "ortools/math_opt/callback.pb.h"
 #include "ortools/math_opt/infeasible_subsystem.pb.h"
@@ -78,7 +79,7 @@ class BaseSolver {
 
     // An optional interrupter that the solver can use to interrupt the solve
     // early.
-    const SolveInterrupter* interrupter = nullptr;
+    const SolveInterrupter* absl_nullable interrupter = nullptr;
 
     friend std::ostream& operator<<(std::ostream& out, const SolveArgs& args);
   };
@@ -96,7 +97,7 @@ class BaseSolver {
 
     // An optional interrupter that the solver can use to interrupt the solve
     // early.
-    const SolveInterrupter* interrupter = nullptr;
+    const SolveInterrupter* absl_nullable interrupter = nullptr;
 
     friend std::ostream& operator<<(std::ostream& out,
                                     const ComputeInfeasibleSubsystemArgs& args);

ortools/math_opt/core/solver_interface.h

@@ -146,7 +146,7 @@ class SolverInterface {
       const ModelSolveParametersProto& model_parameters,
       MessageCallback message_cb,
       const CallbackRegistrationProto& callback_registration, Callback cb,
-      const SolveInterrupter* interrupter) = 0;
+      const SolveInterrupter* absl_nullable interrupter) = 0;
 
   // Updates the model to solve and returns true, or returns false if this
   // update is not supported.
@@ -173,9 +173,9 @@ class SolverInterface {
   // When parameter `message_cb` is not null and the underlying solver does not
   // supports message callbacks, it should ignore it.
   virtual absl::StatusOr<ComputeInfeasibleSubsystemResultProto>
-  ComputeInfeasibleSubsystem(const SolveParametersProto& parameters,
-                             MessageCallback message_cb,
-                             const SolveInterrupter* interrupter) = 0;
+  ComputeInfeasibleSubsystem(
+      const SolveParametersProto& parameters, MessageCallback message_cb,
+      const SolveInterrupter* absl_nullable interrupter) = 0;
 };
 
 class AllSolversRegistry {

ortools/math_opt/core/solver_interface_mock.h

@@ -71,6 +71,8 @@
 #include <memory>
 #include <utility>
 
+#include "absl/base/attributes.h"
+#include "absl/base/nullability.h"
 #include "absl/base/thread_annotations.h"
 #include "absl/log/die_if_null.h"
 #include "absl/status/statusor.h"
@@ -98,7 +100,7 @@ class SolverInterfaceMock : public SolverInterface {
                const ModelSolveParametersProto& model_parameters,
                MessageCallback message_cb,
                const CallbackRegistrationProto& callback_registration,
-               Callback cb, const SolveInterrupter* interrupter),
+               Callback cb, const SolveInterrupter* absl_nullable interrupter),
               (override));
 
   MOCK_METHOD(absl::StatusOr<bool>, Update,
@@ -107,7 +109,8 @@ class SolverInterfaceMock : public SolverInterface {
   MOCK_METHOD(absl::StatusOr<ComputeInfeasibleSubsystemResultProto>,
               ComputeInfeasibleSubsystem,
               (const SolveParametersProto& parameters,
-               MessageCallback message_cb, const SolveInterrupter* interrupter),
+               MessageCallback message_cb,
+               const SolveInterrupter* absl_nullable interrupter),
               (override));
 };
 
@@ -136,7 +139,7 @@ class DelegatingSolver : public SolverInterface {
       const ModelSolveParametersProto& model_parameters,
       MessageCallback message_cb,
       const CallbackRegistrationProto& callback_registration, Callback cb,
-      const SolveInterrupter* const interrupter) override {
+      const SolveInterrupter* absl_nullable const interrupter) override {
     return solver_->Solve(parameters, model_parameters, std::move(message_cb),
                           callback_registration, std::move(cb), interrupter);
   };
@@ -148,7 +151,7 @@ class DelegatingSolver : public SolverInterface {
   absl::StatusOr<ComputeInfeasibleSubsystemResultProto>
   ComputeInfeasibleSubsystem(
       const SolveParametersProto& parameters, MessageCallback message_cb,
-      const SolveInterrupter* const interrupter) override {
+      const SolveInterrupter* absl_nullable const interrupter) override {
     return solver_->ComputeInfeasibleSubsystem(
         parameters, std::move(message_cb), interrupter);
   }

ortools/math_opt/cpp/BUILD.bazel

@@ -517,6 +517,7 @@ cc_library(
         "//ortools/math_opt:model_cc_proto",
         "//ortools/math_opt:model_update_cc_proto",
         "//ortools/math_opt/storage:model_storage",
+        "@abseil-cpp//absl/log:die_if_null",
         "@abseil-cpp//absl/status",
         "@abseil-cpp//absl/status:statusor",
         "@abseil-cpp//absl/strings",
@@ -531,6 +532,7 @@ cc_library(
         "//ortools/base:logging",
         "//ortools/base:source_location",
         "@abseil-cpp//absl/base:core_headers",
+        "@abseil-cpp//absl/log:die_if_null",
         "@abseil-cpp//absl/strings",
         "@abseil-cpp//absl/synchronization",
         "@abseil-cpp//absl/types:span",
@@ -559,6 +561,7 @@ cc_library(
         "//ortools/base:status_macros",
         "//ortools/math_opt/storage:model_storage",
         "//ortools/util:solve_interrupter",
+        "@abseil-cpp//absl/base:nullability",
         "@abseil-cpp//absl/container:flat_hash_set",
         "@abseil-cpp//absl/status",
     ],

ortools/math_opt/cpp/compute_infeasible_subsystem_arguments.h

@@ -14,6 +14,7 @@
 #ifndef ORTOOLS_MATH_OPT_CPP_COMPUTE_INFEASIBLE_SUBSYSTEM_ARGUMENTS_H_
 #define ORTOOLS_MATH_OPT_CPP_COMPUTE_INFEASIBLE_SUBSYSTEM_ARGUMENTS_H_
 
+#include "absl/base/nullability.h"
 #include "ortools/math_opt/cpp/message_callback.h"  // IWYU pragma: export
 #include "ortools/math_opt/cpp/parameters.h"        // IWYU pragma: export
 #include "ortools/util/solve_interrupter.h"         // IWYU pragma: export
@@ -59,7 +60,7 @@ struct ComputeInfeasibleSubsystemArguments {
   //                    ComputeInfeasibleSubsystem(model, SolverType::kGurobi,
   //                          { .interrupter = interrupter.get() });
   //
-  const SolveInterrupter* interrupter = nullptr;
+  const SolveInterrupter* absl_nullable interrupter = nullptr;
 };
 
 }  // namespace operations_research::math_opt

ortools/math_opt/cpp/message_callback.cc

@@ -20,6 +20,7 @@
 #include <vector>
 
 #include "absl/base/thread_annotations.h"
+#include "absl/log/die_if_null.h"
 #include "absl/strings/string_view.h"
 #include "absl/synchronization/mutex.h"
 #include "absl/types/span.h"

ortools/math_opt/cpp/solve_arguments.cc

@@ -24,17 +24,12 @@
 
 namespace operations_research::math_opt {
 
-absl::Status SolveArguments::CheckModelStorageAndCallback(
+absl::Status SolveArguments::CheckModelStorage(
     const ModelStorageCPtr expected_storage) const {
   RETURN_IF_ERROR(model_parameters.CheckModelStorage(expected_storage))
       << "invalid model_parameters";
   RETURN_IF_ERROR(callback_registration.CheckModelStorage(expected_storage))
       << "invalid callback_registration";
-
-  if (callback == nullptr && !callback_registration.events.empty()) {
-    return absl::InvalidArgumentError(
-        "no callback was provided to run, but callback events were registered");
-  }
   return absl::OkStatus();
 }
 

ortools/math_opt/cpp/solve_arguments.h

@@ -17,6 +17,7 @@
 #ifndef ORTOOLS_MATH_OPT_CPP_SOLVE_ARGUMENTS_H_
 #define ORTOOLS_MATH_OPT_CPP_SOLVE_ARGUMENTS_H_
 
+#include "absl/base/nullability.h"
 #include "absl/status/status.h"
 #include "ortools/math_opt/cpp/callback.h"                // IWYU pragma: export
 #include "ortools/math_opt/cpp/message_callback.h"        // IWYU pragma: export
@@ -82,13 +83,11 @@ struct SolveArguments {
   //                    Solve(model, SolverType::kGlop,
   //                          { .interrupter = interrupter.get() });
   //
-  const SolveInterrupter* interrupter = nullptr;
+  const SolveInterrupter* absl_nullable interrupter = nullptr;
 
   // Returns a failure if the referenced variables and constraints don't belong
-  // to the input expected_storage (which must not be nullptr). Also returns a
-  // failure if callback events are registered but no callback is provided.
-  absl::Status CheckModelStorageAndCallback(
-      ModelStorageCPtr expected_storage) const;
+  // to the input expected_storage (which must not be nullptr).
+  absl::Status CheckModelStorage(ModelStorageCPtr expected_storage) const;
 };
 
 }  // namespace operations_research::math_opt

ortools/math_opt/cpp/solve_arguments_test.cc

@@ -27,7 +27,7 @@ namespace {
 using ::testing::HasSubstr;
 using ::testing::status::StatusIs;
 
-TEST(CheckModelStorageAndCallbackTest, CorrectModelAndCallback) {
+TEST(CheckModelStorageTest, CorrectModelAndCallback) {
   Model model;
   const Variable x = model.AddVariable("x");
 
@@ -41,10 +41,10 @@ TEST(CheckModelStorageAndCallbackTest, CorrectModelAndCallback) {
       .callback = [](const CallbackData&) { return CallbackResult{}; },
   };
 
-  EXPECT_OK(args.CheckModelStorageAndCallback(model.storage()));
+  EXPECT_OK(args.CheckModelStorage(model.storage()));
 }
 
-TEST(CheckModelStorageAndCallbackTest, WrongModelInModelParameters) {
+TEST(CheckModelStorageTest, WrongModelInModelParameters) {
   Model model;
   const Variable x = model.AddVariable("x");
 
@@ -53,12 +53,12 @@ TEST(CheckModelStorageAndCallbackTest, WrongModelInModelParameters) {
   };
 
   Model other_model;
-  EXPECT_THAT(args.CheckModelStorageAndCallback(other_model.storage()),
+  EXPECT_THAT(args.CheckModelStorage(other_model.storage()),
               StatusIs(absl::StatusCode::kInvalidArgument,
                        HasSubstr("model_parameters")));
 }
 
-TEST(CheckModelStorageAndCallbackTest, WrongModelInCallbackRegistration) {
+TEST(CheckModelStorageTest, WrongModelInCallbackRegistration) {
   Model model;
   const Variable x = model.AddVariable("x");
 
@@ -72,26 +72,11 @@ TEST(CheckModelStorageAndCallbackTest, WrongModelInCallbackRegistration) {
   };
 
   Model other_model;
-  EXPECT_THAT(args.CheckModelStorageAndCallback(other_model.storage()),
+  EXPECT_THAT(args.CheckModelStorage(other_model.storage()),
               StatusIs(absl::StatusCode::kInvalidArgument,
                        AllOf(HasSubstr("callback_registration"),
                              HasSubstr("mip_solution_filter"))));
 }
 
-TEST(CheckModelStorageAndCallbackTest, NoCallbackWithRegisteredEvents) {
-  Model model;
-
-  const SolveArguments args = {
-      .callback_registration =
-          {
-              .events = {CallbackEvent::kMipSolution},
-          },
-  };
-
-  EXPECT_THAT(
-      args.CheckModelStorageAndCallback(model.storage()),
-      StatusIs(absl::StatusCode::kInvalidArgument, HasSubstr("no callback")));
-}
-
 }  // namespace
 }  // namespace operations_research::math_opt