Update

Author: odow

src/Bridges/bridge_optimizer.jl

@@ -476,7 +476,32 @@ function MOI.is_valid(
             v_map = Variable.bridges(b)::Variable.Map
             return MOI.is_valid(v_map, ci)
         else
-            # This has the potential to be a false positive. see #2817
+            # This return value has the potential to be a false positive: it
+            # doesn't discriminate between constraints that the user added, and
+            # constraints that a bridge added that were themselves bridged.
+            #
+            # "Fixing" this particular call has a number of wide reaching
+            # effects because bridges need this to be "true" so that they can
+            # query attributes of the constraint from `b`.
+            #
+            # In most cases a false positive doesn't matter, because we really
+            # do support querying stuff about it. And also the user needs some
+            # way of obtaining the correct index, which they won't have except
+            # by luck/enumeration.
+            #
+            # The main place that this is problematic is when we come to delete
+            # constraints, and particular VariableIndex constraints, because we
+            # triviallly have their `.value` field from the `.value` of the
+            # VariableIndex.
+            #
+            # Instead of fixing everything though, we implement some extra
+            # checks when deleting, and we leave the false-positive as-is for
+            # now. If you, future reader, hit this comment while debugging, we
+            # might need to revisit this decision.
+            #
+            # x-ref https://github.com/jump-dev/MathOptInterface.jl/issues/2696
+            # x-ref https://github.com/jump-dev/MathOptInterface.jl/issues/2817
+            # x-ref https://github.com/jump-dev/MathOptInterface.jl/pull/2818
             return haskey(Constraint.bridges(b), ci)
         end
     else
@@ -490,32 +515,49 @@ function _delete_variables_in_vector_of_variables_constraint(
     ci::MOI.ConstraintIndex{MOI.VectorOfVariables,S},
 ) where {S}
     func = MOI.get(b, MOI.ConstraintFunction(), ci)
-    variables = copy(func.variables)
-    if vis == variables
+    if vis == func.variables
         MOI.delete(b, ci)
-    else
-        for vi in vis
-            i = findfirst(isequal(vi), variables)
-            if i !== nothing
-                if MOI.supports_dimension_update(S)
-                    call_in_context(MOI.delete, b, ci, IndexInVector(i))
-                else
-                    MOI.Utilities.throw_delete_variable_in_vov(vi)
-                end
-            end
+        return
+    end
+    variables = copy(func.variables)
+    for vi in vis
+        i = findfirst(isequal(vi), variables)
+        if i === nothing
+            continue
+        elseif MOI.supports_dimension_update(S)
+            call_in_context(MOI.delete, b, ci, IndexInVector(i))
+        else
+            MOI.Utilities.throw_delete_variable_in_vov(vi)
         end
     end
+    return
 end
 
-function _is_added_by_bridge(c_map, ci::MOI.ConstraintIndex{F,S}) where {F,S}
-    for (i, bridge) in c_map
-        if i == ci
-            continue
-        elseif ci in MOI.get(bridge, MOI.ListOfConstraintIndices{F,S}())
-            return true
+"""
+    _is_added_by_bridge(
+        c_map,
+        cache::Dict{Any,Any},
+        ci::MOI.ConstraintIndex{F,S},
+    ) where {F,S}
+
+Return `true` if `ci` was added by one of the bridges in `c_map`.
+
+For performance reasons, we store the `ListOfConstraintIndices{F,S}` in `cache`,
+so that we don't have to keep lopping through the bridges.
+"""
+function _is_added_by_bridge(
+    c_map,
+    cache::Dict{Any,Any},
+    ci::MOI.ConstraintIndex{F,S},
+) where {F,S}
+    ret = get!(cache, (F, S)) do
+        list = MOI.ConstraintIndex{F,S}[]
+        for bridge in values(c_map)
+            append!(list, MOI.get(bridge, MOI.ListOfConstraintIndices{F,S}()))
         end
+        return list
     end
-    return false
+    return ci in ret
 end
 
 """
@@ -524,45 +566,37 @@ end
         vis::Vector{MOI.VariableIndex},
     )
 
-!!! warning
-    There's a lot of subtle logic in this function.
+The point of this function is to delete constraints associated with the
+variables in `vis`.
+
+## Warning
+
+Because of the false positive potential in
+`is_valid(::AbstractBridgeOptimizer, MOI.ConstraintIndex)`, we need to ensure
+that we delete constraints only if they were not added by a different constraint
+bridge, otherwise when we come to delete the parent constraint we'll hit a
+runtime error where we have already deleted part of the bridged constraint.
+
+x-ref https://github.com/jump-dev/MathOptInterface.jl/issues/2817
+x-ref https://github.com/jump-dev/MathOptInterface.jl/pull/2818
 """
 function _delete_variables_in_variables_constraints(
     b::AbstractBridgeOptimizer,
     vis::Vector{MOI.VariableIndex},
 )
     c_map = Constraint.bridges(b)::Constraint.Map
-    # The point of this function is to delete constraints associated with the
-    # variables in `vis`.
-    #
-    # There are two problematic cases:
-    #
-    # 1. A VariableIndex bridged to a VectorOfVariables equivalent. For example,
-    #    x in Interval --> [x] in HyperRectangle
-    # 2. A VectorOfVariables bridged to a VariableIndex equivalent. For example,
-    #    [x] in Nonnegatives --> x in GreaterThan
-    #
-    # First, we need to delete scalar constraints, UNLESS they were added by a
-    # different constraint bridge.
+    cache = Dict{Any,Any}()
     for vi in vis
-        # If a bridged `VariableIndex` 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(c_map, vi))
-            if MOI.is_valid(b, ci) && !_is_added_by_bridge(c_map, ci)
+        for ci in Constraint.variable_constraints(c_map, vi)
+            if !_is_added_by_bridge(c_map, cache, ci)
                 MOI.delete(b, ci)
             end
         end
     end
-    # Delete all `MOI.VectorOfVariables` constraints of these variables.
-    # We reverse for the same reason as for `VariableIndex` 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(c_map))
-        _delete_variables_in_vector_of_variables_constraint(b, vis, ci)
+    for ci in Constraint.vector_of_variables_constraints(c_map)
+        if !_is_added_by_bridge(c_map, cache, ci)
+            _delete_variables_in_vector_of_variables_constraint(b, vis, ci)
+        end
     end
     return
 end