Fix double deletion of nested bridged SingleVariable constraint (#1232)

* Fix double deletion of nested bridged SingleVariable constraint

* Add tests and fix for vector of variables

Author: blegat

src/Bridges/bridge_optimizer.jl

@@ -386,13 +386,23 @@ function _delete_variables_in_variables_constraints(
     vis::Vector{MOI.VariableIndex},
 )
     # Delete all `MOI.VectorOfVariables` constraints of these variables.
-    for ci in Constraint.vector_of_variables_constraints(Constraint.bridges(b))
+    # We reverse for the same reason as for `SingleVariable` below.
+    # As the iterators are lazy, when the inner bridge constraint is deleted,
+    # it won't be part of the iteration.
+    for ci in Iterators.reverse(Constraint.vector_of_variables_constraints(Constraint.bridges(b)))
         _delete_variables_in_vector_of_variables_constraint(b, vis, ci)
     end
     # Delete all `MOI.SingleVariable` constraints of these variables.
     for vi in vis
-        for ci in Constraint.variable_constraints(Constraint.bridges(b), vi)
-            MOI.delete(b, ci)
+        # If a bridged `SingleVariable` constraints creates a second one,
+        # then we will delete the second one when deleting the first one hence we
+        # should not delete it again in this loop.
+        # For this, we reverse the order so that we encounter the first one first
+        # and we won't delete the second one since `MOI.is_valid(b, ci)` will be `false`.
+        for ci in Iterators.reverse(Constraint.variable_constraints(Constraint.bridges(b), vi))
+            if MOI.is_valid(b, ci)
+                MOI.delete(b, ci)
+            end
         end
     end
 end

src/Utilities/lazy_iterators.jl

@@ -33,3 +33,4 @@ function Base.iterate(it::LazyMap, args...)
 end
 Base.IteratorSize(it::LazyMap) = Base.IteratorSize(it.data)
 Base.eltype(::LazyMap{T}) where {T} = T
+Iterators.reverse(it::LazyMap{T}) where T = LazyMap{T}(it.f, Iterators.reverse(it.data))

test/Bridges/bridge_optimizer.jl

@@ -276,3 +276,38 @@ end
       with 0 constraint bridges
       with inner model $(MOIU.MockOptimizer{NoIntervalModel{Float64}})""")
 end
+
+MOI.Utilities.@model(AffineOnlyModel, (), (MOI.Interval,), (MOI.PositiveSemidefiniteConeTriangle,), (), (), (MOI.ScalarAffineFunction,), (), (MOI.VectorAffineFunction,), true)
+MOI.supports_constraint(::AffineOnlyModel{T}, ::Type{MOI.SingleVariable}, ::Type{MOI.LessThan{T}}) where T = false
+MOI.supports_constraint(::AffineOnlyModel{T}, ::Type{MOI.SingleVariable}, ::Type{MOI.Interval{T}}) where T = false
+@testset "Double deletion of nested bridged SingleVariable constraint" begin
+    @testset "Scalar" begin
+        # The variable is bridged to `SingleVariable`-in-`Interval` and then `ScalarAffineFunction`-in-`Interval`.
+        # Hence there is two bridged `SingleVariable` constraints on the same variables and we need to be
+        # careful not to delete the second one twice, see https://github.com/jump-dev/MathOptInterface.jl/issues/1231
+        model = MOI.instantiate(AffineOnlyModel{Float64}, with_bridge_type=Float64)
+        x = MOI.add_variable(model)
+        c = MOI.add_constraint(model, MOI.SingleVariable(x), MOI.LessThan(1.0))
+        # Need to test the bridging to make sure it's not functionized first as otherwise,
+        # this test would not cover the case we want to test
+        b1 = MOI.Bridges.bridge(model, c)
+        @test b1 isa MOI.Bridges.Constraint.LessToIntervalBridge
+        b2 = MOI.Bridges.bridge(model, b1.constraint)
+        @test b2 isa MOI.Bridges.Constraint.ScalarFunctionizeBridge
+        @test !MOI.Bridges.is_bridged(model, b2.constraint)
+        MOI.delete(model, x)
+        @test !MOI.is_valid(model, x)
+    end
+    @testset "Vector" begin
+        model = MOI.instantiate(AffineOnlyModel{Float64}, with_bridge_type=Float64)
+        x = MOI.add_variables(model, 4)
+        c = MOI.add_constraint(model, MOI.VectorOfVariables(x), MOI.PositiveSemidefiniteConeSquare(2))
+        b1 = MOI.Bridges.bridge(model, c)
+        @test b1 isa MOI.Bridges.Constraint.SquareBridge
+        b2 = MOI.Bridges.bridge(model, b1.triangle)
+        @test b2 isa MOI.Bridges.Constraint.VectorFunctionizeBridge
+        @test !MOI.Bridges.is_bridged(model, b2.constraint)
+        MOI.delete(model, x)
+        @test all(vi -> !MOI.is_valid(model, vi), x)
+    end
+end