From 60b532e78302f99506d6f3af7350e8b4b8057792 Mon Sep 17 00:00:00 2001
From: odow <o.dowson@gmail.com>
Date: Fri, 22 Dec 2023 17:26:12 +1300
Subject: [PATCH 1/7] [Bridges] fix BoundAlreadySet errors

---
 src/Bridges/bridge_optimizer.jl       | 81 +++++++++++++++++++++------
 src/Test/test_model.jl                | 12 ++--
 test/Bridges/bridge_optimizer.jl      |  2 -
 test/Bridges/lazy_bridge_optimizer.jl | 10 +---
 4 files changed, 70 insertions(+), 35 deletions(-)

diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl
index 67b5661732..8259df332e 100644
--- a/src/Bridges/bridge_optimizer.jl
+++ b/src/Bridges/bridge_optimizer.jl
@@ -697,6 +697,7 @@ function MOI.delete(b::AbstractBridgeOptimizer, ci::MOI.ConstraintIndex)
     else
         MOI.delete(b.model, ci)
     end
+    return
 end
 
 function MOI.delete(
@@ -1732,28 +1733,76 @@ function add_bridged_constraint(b, BridgeType, f, s)
     return ci
 end
 
+function _throw_if_bound_already_set(b, x, ::S2, ::Type{S1}) where {S1,S2}
+    ErrorType = _bound_error_type(S1, S2)
+    if ErrorType === nothing
+        return
+    end
+    ci = MOI.ConstraintIndex{MOI.VariableIndex,S1}(x.value)
+    if (is_bridged(b, S1) && MOI.is_valid(b, ci)) || MOI.is_valid(b.model, ci)
+        throw(ErrorType(x))
+    end
+    return
+end
+
+function _bound_error_type(::Type{S}, ::Type{S}) where {S<:MOI.LessThan}
+    return MOI.UpperBoundAlreadySet{S,S}
+end
+
+function _bounderror_typet(::Type{S1}, ::Type{S2}) where {S1<:MOI.LessThan,S2}
+    return MOI.UpperBoundAlreadySet{S1,S2}
+end
+
+function _bound_error_type(::Type{S1}, ::Type{S2}) where {S1,S2<:MOI.LessThan}
+    return MOI.UpperBoundAlreadySet{S1,S2}
+end
+
+_bound_error_type(S1, S2) = MOI.LowerBoundAlreadySet{S1,S2}
+
+_bound_error_type(::Type{<:MOI.LessThan}, ::Type{<:MOI.GreaterThan}) = nothing
+
+_bound_error_type(::Type{<:MOI.GreaterThan}, ::Type{<:MOI.LessThan}) = nothing
+
 function _check_double_single_variable(
     b::AbstractBridgeOptimizer,
-    func::MOI.VariableIndex,
-    set,
-)
-    if !is_bridged(b, typeof(set))
-        # The variable might have been constrained in `b.model` to a set of type
-        # `typeof(set)` on creation.
-        ci = MOI.ConstraintIndex{MOI.VariableIndex,typeof(set)}(func.value)
-        if MOI.is_valid(b.model, ci)
-            error(
-                "The variable `$(func)` was constrained on creation ",
-                "to a set of type `$(typeof(set))`. Therefore, a ",
-                "`VariableIndex`-in-`$(typeof(set))` cannot be added on this ",
-                "variable. Use `MOI.set` with `MOI.ConstraintSet()` instead.",
-            )
-        end
+    x::MOI.VariableIndex,
+    s::S,
+) where {
+    T,
+    S<:Union{
+        MOI.EqualTo{T},
+        MOI.GreaterThan{T},
+        MOI.LessThan{T},
+        MOI.Interval{T},
+        MOI.Semicontinuous{T},
+        MOI.Semiinteger{T},
+    },
+}
+    _throw_if_bound_already_set(b, x, s, MOI.EqualTo{T})
+    _throw_if_bound_already_set(b, x, s, MOI.GreaterThan{T})
+    _throw_if_bound_already_set(b, x, s, MOI.LessThan{T})
+    _throw_if_bound_already_set(b, x, s, MOI.Interval{T})
+    _throw_if_bound_already_set(b, x, s, MOI.Semicontinuous{T})
+    _throw_if_bound_already_set(b, x, s, MOI.Semiinteger{T})
+    return
+end
+
+function _check_double_single_variable(
+    b::AbstractBridgeOptimizer,
+    x::MOI.VariableIndex,
+    s::S,
+) where {S<:MOI.AbstractScalarSet}
+    ci = MOI.ConstraintIndex{MOI.VariableIndex,S}(x.value)
+    if (is_bridged(b, S) && MOI.is_valid(b, ci)) || MOI.is_valid(b.model, ci)
+        error(
+            "The variable `$x` was already constrained to a set of type `$S`." *
+            " Use `MOI.set` with `MOI.ConstraintSet()` instead.",
+        )
     end
     return
 end
 
-_check_double_single_variable(b::AbstractBridgeOptimizer, func, set) = nothing
+_check_double_single_variable(::AbstractBridgeOptimizer, ::Any, ::Any) = nothing
 
 function MOI.add_constraint(
     b::AbstractBridgeOptimizer,
diff --git a/src/Test/test_model.jl b/src/Test/test_model.jl
index 3778738c67..5d59cc3265 100644
--- a/src/Test/test_model.jl
+++ b/src/Test/test_model.jl
@@ -805,12 +805,10 @@ function test_model_LowerBoundAlreadySet(
             continue
         end
         ci = MOI.add_constraint(model, x, set1)
-        err = MOI.LowerBoundAlreadySet{typeof(set1),typeof(set2)}(x)
-        @test_throws err MOI.add_constraint(model, x, set2)
+        @test_throws MOI.LowerBoundAlreadySet MOI.add_constraint(model, x, set2)
         MOI.delete(model, ci)
         ci = MOI.add_constraint(model, x, set2)
-        err = MOI.LowerBoundAlreadySet{typeof(set2),typeof(set1)}(x)
-        @test_throws err MOI.add_constraint(model, x, set1)
+        @test_throws MOI.LowerBoundAlreadySet MOI.add_constraint(model, x, set1)
         MOI.delete(model, ci)
     end
     return
@@ -839,12 +837,10 @@ function test_model_UpperBoundAlreadySet(
             continue
         end
         ci = MOI.add_constraint(model, x, set1)
-        err = MOI.UpperBoundAlreadySet{typeof(set1),typeof(set2)}(x)
-        @test_throws err MOI.add_constraint(model, x, set2)
+        @test_throws MOI.UpperBoundAlreadySet MOI.add_constraint(model, x, set2)
         MOI.delete(model, ci)
         ci = MOI.add_constraint(model, x, set2)
-        err = MOI.UpperBoundAlreadySet{typeof(set2),typeof(set1)}(x)
-        @test_throws err MOI.add_constraint(model, x, set1)
+        @test_throws MOI.UpperBoundAlreadySet MOI.add_constraint(model, x, set1)
         MOI.delete(model, ci)
     end
     return
diff --git a/test/Bridges/bridge_optimizer.jl b/test/Bridges/bridge_optimizer.jl
index daf11fffe3..0a57b16507 100644
--- a/test/Bridges/bridge_optimizer.jl
+++ b/test/Bridges/bridge_optimizer.jl
@@ -524,8 +524,6 @@ function test_MOI_Test()
         MOI.Test.Config(exclude = Any[MOI.optimize!]),
         include = ["test_linear_", "test_model_"],
         exclude = [
-            "test_model_LowerBoundAlreadySet",
-            "test_model_UpperBoundAlreadySet",
             "test_model_ListOfConstraintAttributesSet",
         ],
     )
diff --git a/test/Bridges/lazy_bridge_optimizer.jl b/test/Bridges/lazy_bridge_optimizer.jl
index 91ec43a005..92a38d6082 100644
--- a/test/Bridges/lazy_bridge_optimizer.jl
+++ b/test/Bridges/lazy_bridge_optimizer.jl
@@ -311,8 +311,6 @@ function test_MOI_runtests_LPModel()
             "test_model_default_DualStatus",
             "test_model_default_PrimalStatus",
             "test_model_default_TerminationStatus",
-            "test_model_LowerBoundAlreadySet",
-            "test_model_UpperBoundAlreadySet",
         ],
     )
     return
@@ -328,8 +326,6 @@ function test_MOI_runtests_StandardSDPAModel()
         );
         exclude = String[
             "test_model_ListOfVariablesWithAttributeSet",
-            "test_model_LowerBoundAlreadySet",
-            "test_model_UpperBoundAlreadySet",
             "test_model_ScalarFunctionConstantNotZero",
             "test_model_delete",
         ],
@@ -344,11 +340,7 @@ function test_MOI_runtests_GeometricSDPAModel()
         model,
         MOI.Test.Config(
             exclude = Any[MOI.optimize!, MOI.SolverName, MOI.SolverVersion],
-        );
-        exclude = String[
-            "test_model_LowerBoundAlreadySet",
-            "test_model_UpperBoundAlreadySet",
-        ],
+        ),
     )
     return
 end

From 956a8429e5c1df0719e6c478603dbde86621e21b Mon Sep 17 00:00:00 2001
From: odow <o.dowson@gmail.com>
Date: Fri, 22 Dec 2023 17:43:50 +1300
Subject: [PATCH 2/7] Fix

---
 src/Bridges/bridge_optimizer.jl | 23 +++++++++++++++--------
 src/Test/test_model.jl          | 12 ++++++++----
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl
index 8259df332e..b6b53c7d79 100644
--- a/src/Bridges/bridge_optimizer.jl
+++ b/src/Bridges/bridge_optimizer.jl
@@ -1749,7 +1749,7 @@ function _bound_error_type(::Type{S}, ::Type{S}) where {S<:MOI.LessThan}
     return MOI.UpperBoundAlreadySet{S,S}
 end
 
-function _bounderror_typet(::Type{S1}, ::Type{S2}) where {S1<:MOI.LessThan,S2}
+function _bound_error_type(::Type{S1}, ::Type{S2}) where {S1<:MOI.LessThan,S2}
     return MOI.UpperBoundAlreadySet{S1,S2}
 end
 
@@ -1757,7 +1757,9 @@ function _bound_error_type(::Type{S1}, ::Type{S2}) where {S1,S2<:MOI.LessThan}
     return MOI.UpperBoundAlreadySet{S1,S2}
 end
 
-_bound_error_type(S1, S2) = MOI.LowerBoundAlreadySet{S1,S2}
+function _bound_error_type(::Type{S1}, ::Type{S2}) where {S1,S2}
+    return MOI.LowerBoundAlreadySet{S1,S2}
+end
 
 _bound_error_type(::Type{<:MOI.LessThan}, ::Type{<:MOI.GreaterThan}) = nothing
 
@@ -1770,20 +1772,25 @@ function _check_double_single_variable(
 ) where {
     T,
     S<:Union{
-        MOI.EqualTo{T},
-        MOI.GreaterThan{T},
-        MOI.LessThan{T},
         MOI.Interval{T},
+        MOI.EqualTo{T},
         MOI.Semicontinuous{T},
         MOI.Semiinteger{T},
+        MOI.LessThan{T},
+        MOI.GreaterThan{T},
     },
 }
-    _throw_if_bound_already_set(b, x, s, MOI.EqualTo{T})
-    _throw_if_bound_already_set(b, x, s, MOI.GreaterThan{T})
-    _throw_if_bound_already_set(b, x, s, MOI.LessThan{T})
+    # !!! warning
+    #     The order here is _very_ important because an Interval constraint
+    #     might get re-written into a LessThan and GreaterThan. To throw the
+    #     appropriate error, we _must_ check sets like `Interval` and `EqualTo`
+    #     _before_ `LessThan` and `GreaterThan`.
     _throw_if_bound_already_set(b, x, s, MOI.Interval{T})
+    _throw_if_bound_already_set(b, x, s, MOI.EqualTo{T})
     _throw_if_bound_already_set(b, x, s, MOI.Semicontinuous{T})
     _throw_if_bound_already_set(b, x, s, MOI.Semiinteger{T})
+    _throw_if_bound_already_set(b, x, s, MOI.LessThan{T})
+    _throw_if_bound_already_set(b, x, s, MOI.GreaterThan{T})
     return
 end
 
diff --git a/src/Test/test_model.jl b/src/Test/test_model.jl
index 5d59cc3265..3778738c67 100644
--- a/src/Test/test_model.jl
+++ b/src/Test/test_model.jl
@@ -805,10 +805,12 @@ function test_model_LowerBoundAlreadySet(
             continue
         end
         ci = MOI.add_constraint(model, x, set1)
-        @test_throws MOI.LowerBoundAlreadySet MOI.add_constraint(model, x, set2)
+        err = MOI.LowerBoundAlreadySet{typeof(set1),typeof(set2)}(x)
+        @test_throws err MOI.add_constraint(model, x, set2)
         MOI.delete(model, ci)
         ci = MOI.add_constraint(model, x, set2)
-        @test_throws MOI.LowerBoundAlreadySet MOI.add_constraint(model, x, set1)
+        err = MOI.LowerBoundAlreadySet{typeof(set2),typeof(set1)}(x)
+        @test_throws err MOI.add_constraint(model, x, set1)
         MOI.delete(model, ci)
     end
     return
@@ -837,10 +839,12 @@ function test_model_UpperBoundAlreadySet(
             continue
         end
         ci = MOI.add_constraint(model, x, set1)
-        @test_throws MOI.UpperBoundAlreadySet MOI.add_constraint(model, x, set2)
+        err = MOI.UpperBoundAlreadySet{typeof(set1),typeof(set2)}(x)
+        @test_throws err MOI.add_constraint(model, x, set2)
         MOI.delete(model, ci)
         ci = MOI.add_constraint(model, x, set2)
-        @test_throws MOI.UpperBoundAlreadySet MOI.add_constraint(model, x, set1)
+        err = MOI.UpperBoundAlreadySet{typeof(set2),typeof(set1)}(x)
+        @test_throws err MOI.add_constraint(model, x, set1)
         MOI.delete(model, ci)
     end
     return

From b552684fb894bec36babb1cfb886471b4bebc264 Mon Sep 17 00:00:00 2001
From: odow <o.dowson@gmail.com>
Date: Fri, 22 Dec 2023 17:50:50 +1300
Subject: [PATCH 3/7] fix ambiguity

---
 src/Bridges/bridge_optimizer.jl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl
index b6b53c7d79..3ea795a28a 100644
--- a/src/Bridges/bridge_optimizer.jl
+++ b/src/Bridges/bridge_optimizer.jl
@@ -1745,8 +1745,11 @@ function _throw_if_bound_already_set(b, x, ::S2, ::Type{S1}) where {S1,S2}
     return
 end
 
-function _bound_error_type(::Type{S}, ::Type{S}) where {S<:MOI.LessThan}
-    return MOI.UpperBoundAlreadySet{S,S}
+function _bound_error_type(
+    ::Type{S1},
+    ::Type{S2},
+) where {S1<:MOI.LessThan,S2<:MOI.LessThan}
+    return MOI.UpperBoundAlreadySet{S1,S2}
 end
 
 function _bound_error_type(::Type{S1}, ::Type{S2}) where {S1<:MOI.LessThan,S2}

From 08dad9a2ef9ff4ef4a40364980234d4e2019ea05 Mon Sep 17 00:00:00 2001
From: odow <o.dowson@gmail.com>
Date: Fri, 22 Dec 2023 18:26:33 +1300
Subject: [PATCH 4/7] Update

---
 src/Bridges/bridge_optimizer.jl       | 2 +-
 test/Bridges/bridge_optimizer.jl      | 2 ++
 test/Bridges/lazy_bridge_optimizer.jl | 2 ++
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl
index 3ea795a28a..7b3e2093fe 100644
--- a/src/Bridges/bridge_optimizer.jl
+++ b/src/Bridges/bridge_optimizer.jl
@@ -1801,7 +1801,7 @@ function _check_double_single_variable(
     b::AbstractBridgeOptimizer,
     x::MOI.VariableIndex,
     s::S,
-) where {S<:MOI.AbstractScalarSet}
+) where {S}
     ci = MOI.ConstraintIndex{MOI.VariableIndex,S}(x.value)
     if (is_bridged(b, S) && MOI.is_valid(b, ci)) || MOI.is_valid(b.model, ci)
         error(
diff --git a/test/Bridges/bridge_optimizer.jl b/test/Bridges/bridge_optimizer.jl
index 0a57b16507..daf11fffe3 100644
--- a/test/Bridges/bridge_optimizer.jl
+++ b/test/Bridges/bridge_optimizer.jl
@@ -524,6 +524,8 @@ function test_MOI_Test()
         MOI.Test.Config(exclude = Any[MOI.optimize!]),
         include = ["test_linear_", "test_model_"],
         exclude = [
+            "test_model_LowerBoundAlreadySet",
+            "test_model_UpperBoundAlreadySet",
             "test_model_ListOfConstraintAttributesSet",
         ],
     )
diff --git a/test/Bridges/lazy_bridge_optimizer.jl b/test/Bridges/lazy_bridge_optimizer.jl
index 92a38d6082..64777f256b 100644
--- a/test/Bridges/lazy_bridge_optimizer.jl
+++ b/test/Bridges/lazy_bridge_optimizer.jl
@@ -326,6 +326,8 @@ function test_MOI_runtests_StandardSDPAModel()
         );
         exclude = String[
             "test_model_ListOfVariablesWithAttributeSet",
+            "test_model_LowerBoundAlreadySet",
+            "test_model_UpperBoundAlreadySet",
             "test_model_ScalarFunctionConstantNotZero",
             "test_model_delete",
         ],

From 2d4645ece51733248b9ec85ed8f000ec6e625dd4 Mon Sep 17 00:00:00 2001
From: odow <o.dowson@gmail.com>
Date: Fri, 22 Dec 2023 19:23:03 +1300
Subject: [PATCH 5/7] Update

---
 src/Utilities/variables_container.jl  | 2 ++
 test/Bridges/lazy_bridge_optimizer.jl | 9 +--------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/src/Utilities/variables_container.jl b/src/Utilities/variables_container.jl
index 585f85492e..18c65b9226 100644
--- a/src/Utilities/variables_container.jl
+++ b/src/Utilities/variables_container.jl
@@ -308,6 +308,8 @@ function MOI.is_valid(
     return !iszero(b.set_mask[ci.value] & _single_variable_flag(S))
 end
 
+MOI.is_valid(::VariablesContainer, ::MOI.ConstraintIndex) = false
+
 function MOI.get(
     model::VariablesContainer,
     ::MOI.ConstraintFunction,
diff --git a/test/Bridges/lazy_bridge_optimizer.jl b/test/Bridges/lazy_bridge_optimizer.jl
index 64777f256b..bbc6eb193e 100644
--- a/test/Bridges/lazy_bridge_optimizer.jl
+++ b/test/Bridges/lazy_bridge_optimizer.jl
@@ -303,15 +303,8 @@ function test_MOI_runtests_LPModel()
     bridged = MOI.Bridges.full_bridge_optimizer(model, Float64)
     MOI.Test.runtests(
         bridged,
-        MOI.Test.Config(exclude = Any[MOI.optimize!]),
+        MOI.Test.Config(exclude = Any[MOI.optimize!]);
         include = ["test_model_", "test_constraint_"],
-        exclude = [
-            "test_constraint_ConstraintDualStart",
-            "test_constraint_ConstraintPrimalStart",
-            "test_model_default_DualStatus",
-            "test_model_default_PrimalStatus",
-            "test_model_default_TerminationStatus",
-        ],
     )
     return
 end

From 778e78ae4e9adf6f7c4d5227946b2d9672a40ca5 Mon Sep 17 00:00:00 2001
From: odow <o.dowson@gmail.com>
Date: Fri, 22 Dec 2023 21:39:54 +1300
Subject: [PATCH 6/7] Update

---
 src/Bridges/bridge_optimizer.jl       | 15 ---------------
 test/Bridges/lazy_bridge_optimizer.jl |  5 ++++-
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/src/Bridges/bridge_optimizer.jl b/src/Bridges/bridge_optimizer.jl
index 7b3e2093fe..1405be62ac 100644
--- a/src/Bridges/bridge_optimizer.jl
+++ b/src/Bridges/bridge_optimizer.jl
@@ -1797,21 +1797,6 @@ function _check_double_single_variable(
     return
 end
 
-function _check_double_single_variable(
-    b::AbstractBridgeOptimizer,
-    x::MOI.VariableIndex,
-    s::S,
-) where {S}
-    ci = MOI.ConstraintIndex{MOI.VariableIndex,S}(x.value)
-    if (is_bridged(b, S) && MOI.is_valid(b, ci)) || MOI.is_valid(b.model, ci)
-        error(
-            "The variable `$x` was already constrained to a set of type `$S`." *
-            " Use `MOI.set` with `MOI.ConstraintSet()` instead.",
-        )
-    end
-    return
-end
-
 _check_double_single_variable(::AbstractBridgeOptimizer, ::Any, ::Any) = nothing
 
 function MOI.add_constraint(
diff --git a/test/Bridges/lazy_bridge_optimizer.jl b/test/Bridges/lazy_bridge_optimizer.jl
index bbc6eb193e..efb63526a1 100644
--- a/test/Bridges/lazy_bridge_optimizer.jl
+++ b/test/Bridges/lazy_bridge_optimizer.jl
@@ -318,9 +318,12 @@ function test_MOI_runtests_StandardSDPAModel()
             exclude = Any[MOI.optimize!, MOI.SolverName, MOI.SolverVersion],
         );
         exclude = String[
-            "test_model_ListOfVariablesWithAttributeSet",
+            # Okay to exclude: because the bridge reformulates bound
+            # constraints, there is no conflict.
             "test_model_LowerBoundAlreadySet",
             "test_model_UpperBoundAlreadySet",
+            # TODO(odow): innvestigate
+            "test_model_ListOfVariablesWithAttributeSet",
             "test_model_ScalarFunctionConstantNotZero",
             "test_model_delete",
         ],

From 384c5831981ae870a6ced473250542d9f8cd64a8 Mon Sep 17 00:00:00 2001
From: odow <o.dowson@gmail.com>
Date: Fri, 22 Dec 2023 21:46:23 +1300
Subject: [PATCH 7/7] Add explanation

---
 test/Bridges/lazy_bridge_optimizer.jl | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/test/Bridges/lazy_bridge_optimizer.jl b/test/Bridges/lazy_bridge_optimizer.jl
index efb63526a1..43003c069c 100644
--- a/test/Bridges/lazy_bridge_optimizer.jl
+++ b/test/Bridges/lazy_bridge_optimizer.jl
@@ -318,14 +318,24 @@ function test_MOI_runtests_StandardSDPAModel()
             exclude = Any[MOI.optimize!, MOI.SolverName, MOI.SolverVersion],
         );
         exclude = String[
-            # Okay to exclude: because the bridge reformulates bound
-            # constraints, there is no conflict.
+            # TODO(odow): investigate. This seems like an actual bug.
+            "test_model_delete",
+            # Skip these tests because the bridge reformulates bound
+            # constraints, so there is no conflict. An error _is_ thrown if two
+            # sets of the same type are added.
             "test_model_LowerBoundAlreadySet",
             "test_model_UpperBoundAlreadySet",
-            # TODO(odow): innvestigate
-            "test_model_ListOfVariablesWithAttributeSet",
+            # MOI.ScalarFunctionConstantNotZero is thrown, not of the original
+            # constraint, but of the bridged constraint. This seems okay. The
+            # fix would require that a bridge optimizer has a try-catch for this
+            # error.
             "test_model_ScalarFunctionConstantNotZero",
-            "test_model_delete",
+            # The error is:
+            # Cannot substitute `MOI.VariableIndex(1)` as it is bridged into `0.0 + 1.0 MOI.VariableIndex(-1)`.
+            # This seems okay. We can't get a list of variables if they are
+            # bridged.
+            "test_model_ListOfVariablesWithAttributeSet",
+
         ],
     )
     return