Merge pull request #1765 from JuliaOpt/bl/variablenotowned

🚸 Throw error if a variable is not owned

src/JuMP.jl

@@ -491,7 +491,7 @@ function Base.showerror(io::IO, ex::VariableNotOwnedError)
     print(io, "VariableNotOwnedError: Variable not owned by model present in $(ex.context)")
 end
 
-function verify_ownership(m::Model, vec::Vector{VariableRef})
+function check_belongs_to_model(vec::Vector{VariableRef}, m::Model)
     n = length(vec)
     @inbounds for i in 1:n
         vec[i].m !== m && return false
@@ -555,23 +555,27 @@ end
 Return the value of the attribute `attr` from model's MOI backend.
 """
 MOI.get(m::Model, attr::MOI.AbstractModelAttribute) = MOI.get(backend(m), attr)
-function MOI.get(m::Model, attr::MOI.AbstractVariableAttribute, v::VariableRef)
-    @assert m === owner_model(v) # TODO: Improve the error message.
-    return MOI.get(backend(m), attr, index(v))
+function MOI.get(model::Model, attr::MOI.AbstractVariableAttribute,
+                 v::VariableRef)
+    check_belongs_to_model(v, model)
+    return MOI.get(backend(model), attr, index(v))
 end
-function MOI.get(m::Model, attr::MOI.AbstractConstraintAttribute, cr::ConstraintRef)
-    @assert m === cr.model # TODO: Improve the error message.
-    return MOI.get(backend(m), attr, index(cr))
+function MOI.get(model::Model, attr::MOI.AbstractConstraintAttribute,
+                 cr::ConstraintRef)
+    check_belongs_to_model(cr, model)
+    return MOI.get(backend(model), attr, index(cr))
 end
 
 MOI.set(m::Model, attr::MOI.AbstractModelAttribute, value) = MOI.set(backend(m), attr, value)
-function MOI.set(m::Model, attr::MOI.AbstractVariableAttribute, v::VariableRef, value)
-    @assert m === owner_model(v) # TODO: Improve the error message.
-    MOI.set(backend(m), attr, index(v), value)
-end
-function MOI.set(m::Model, attr::MOI.AbstractConstraintAttribute, cr::ConstraintRef, value)
-    @assert m === cr.model # TODO: Improve the error message.
-    MOI.set(backend(m), attr, index(cr), value)
+function MOI.set(model::Model, attr::MOI.AbstractVariableAttribute,
+                 v::VariableRef, value)
+    check_belongs_to_model(v, model)
+    MOI.set(backend(model), attr, index(v), value)
+end
+function MOI.set(model::Model, attr::MOI.AbstractConstraintAttribute,
+                 cr::ConstraintRef, value)
+    check_belongs_to_model(cr, model)
+    MOI.set(backend(model), attr, index(cr), value)
 end
 
 ###############################################################################

src/aff_expr.jl

@@ -261,8 +261,15 @@ Replaces `getvalue` for most use cases.
 """
 value(a::GenericAffExpr) = value(a, value)
 
+function check_belongs_to_model(a::GenericAffExpr, model::AbstractModel)
+    for variable in keys(a.terms)
+        check_belongs_to_model(variable, model)
+    end
+end
+
 # Note: No validation is performed that the variables in the AffExpr belong to
-# the same model.
+# the same model. The verification is done in `check_belongs_to_model` which
+# should be called before calling `MOI.ScalarAffineFunction`.
 function MOI.ScalarAffineFunction(a::AffExpr)
     assert_isfinite(a)
     terms = MOI.ScalarAffineTerm{Float64}[MOI.ScalarAffineTerm(t[1],
@@ -275,6 +282,7 @@ function moi_function_type(::Type{<:GenericAffExpr{T}}) where T
     return MOI.ScalarAffineFunction{T}
 end
 
+
 function AffExpr(m::Model, f::MOI.ScalarAffineFunction)
     aff = AffExpr()
     for t in f.terms

src/constraints.jl

@@ -106,6 +106,36 @@ struct ConstraintRef{M <: AbstractModel, C, Shape <: AbstractShape}
     shape::Shape
 end
 
+"""
+    struct ConstraintNotOwned{C <: ConstraintRef} <: Exception
+        constraint_ref::C
+    end
+
+The constraint `constraint_ref` was used in a model different to
+`owner_model(constraint_ref)`.
+"""
+struct ConstraintNotOwned{C <: ConstraintRef} <: Exception
+    constraint_ref::C
+end
+
+"""
+    owner_model(cref::ConstraintRef)
+
+Returns the model to which `cref` belongs.
+"""
+owner_model(cref::ConstraintRef) = cref.model
+
+"""
+    check_belongs_to_model(cref::ConstraintRef, model::AbstractModel)
+
+Throw `ConstraintNotOwned` if `owner_model(cref)` is not `model`.
+"""
+function check_belongs_to_model(cref::ConstraintRef, model::AbstractModel)
+    if owner_model(cref) !== model
+        throw(ConstraintNotOwned(cref))
+    end
+end
+
 Base.broadcastable(cref::ConstraintRef) = Ref(cref)
 
 """
@@ -286,6 +316,9 @@ function constraint_object(ref::ConstraintRef{Model, MOICON{FuncType, SetType}})
     s = MOI.get(model, MOI.ConstraintSet(), ref)::SetType
     return ScalarConstraint(jump_function(model, f), s)
 end
+function check_belongs_to_model(c::ScalarConstraint, model)
+    check_belongs_to_model(c.func, model)
+end
 
 struct VectorConstraint{F <: AbstractJuMPScalar,
                         S <: MOI.AbstractVectorSet,
@@ -309,6 +342,11 @@ function constraint_object(ref::ConstraintRef{Model, MOICON{FuncType, SetType}})
     s = MOI.get(model, MOI.ConstraintSet(), ref)::SetType
     return VectorConstraint(jump_function(model, f), s, ref.shape)
 end
+function check_belongs_to_model(c::VectorConstraint, model)
+    for func in c.func
+        check_belongs_to_model(func, model)
+    end
+end
 
 function moi_add_constraint(model::MOI.ModelLike, f::MOI.AbstractFunction,
                             s::MOI.AbstractSet)
@@ -333,6 +371,7 @@ Add a constraint `c` to `Model model` and sets its name.
 function add_constraint(model::Model, c::AbstractConstraint, name::String="")
     # The type of backend(model) is unknown so we directly redirect to another
     # function.
+    check_belongs_to_model(c, model)
     cindex = moi_add_constraint(backend(model), moi_function(c), moi_set(c))
     cref = ConstraintRef(model, cindex, shape(c))
     if !isempty(name)

src/objective.jl

@@ -73,6 +73,7 @@ function set_objective_function(model::Model, func::MOI.AbstractScalarFunction)
 end
 
 function set_objective_function(model::Model, func::AbstractJuMPScalar)
+    check_belongs_to_model(func, model)
     set_objective_function(model, moi_function(func))
 end
 

src/quad_expr.jl

@@ -198,6 +198,14 @@ function Base.convert(::Type{GenericQuadExpr{C, V}}, v::Union{Real,AbstractVaria
 end
 GenericQuadExpr{C, V}() where {C, V} = zero(GenericQuadExpr{C, V})
 
+function check_belongs_to_model(q::GenericQuadExpr, model::AbstractModel)
+    check_belongs_to_model(q.aff, model)
+    for variable_pair in keys(q.terms)
+        check_belongs_to_model(variable_pair.a, model)
+        check_belongs_to_model(variable_pair.b, model)
+    end
+end
+
 """
     moi_quadratic_term(t::Tuple)
 

src/variables.jl

@@ -119,6 +119,37 @@ true
 """
 owner_model(v::AbstractVariableRef) = v.model
 
+"""
+    struct VariableNotOwned{V <: AbstractVariableRef} <: Exception
+        variable::V
+    end
+
+The variable `variable` was used in a model different to
+`owner_model(variable)`.
+"""
+struct VariableNotOwned{V <: AbstractVariableRef} <: Exception
+    variable::V
+end
+
+"""
+    check_belongs_to_model(func::AbstractJuMPScalar, model::AbstractModel)
+
+Throw `VariableNotOwned` if the `owner_model` of one of the variables of the
+function `func` is not `model`.
+
+    check_belongs_to_model(constraint::AbstractConstraint, model::AbstractModel)
+
+Throw `VariableNotOwned` if the `owner_model` of one of the variables of the
+constraint `constraint` is not `model`.
+"""
+function check_belongs_to_model end
+
+function check_belongs_to_model(v::AbstractVariableRef, model::AbstractModel)
+    if owner_model(v) !== model
+        throw(VariableNotOwned(v))
+    end
+end
+
 Base.iszero(::VariableRef) = false
 Base.copy(v::VariableRef) = VariableRef(v.model, v.index)
 Base.broadcastable(v::VariableRef) = Ref(v)

test/model.jl

@@ -42,6 +42,79 @@ end
 include("nonnegative_bridge.jl")
 
 function test_model()
+    @testset "Test variable/model 'hygiene'" begin
+        model_x = Model()
+        @variable(model_x, x)
+        model_y = Model()
+        @variable(model_y, y)
+        err = JuMP.VariableNotOwned(y)
+        @testset "Variable" begin
+            @testset "constraint" begin
+                @test_throws err @constraint(model_x, y in MOI.EqualTo(1.0))
+                @test_throws err @constraint(model_x, [x, y] in MOI.Zeros(2))
+            end
+            @testset "objective" begin
+                @test_throws err @objective(model_x, Min, y)
+            end
+        end
+        @testset "Linear" begin
+            @testset "constraint" begin
+                @test_throws err @constraint(model_x, x + y == 1)
+                @test_throws err begin
+                    @constraint(model_x, [x, x + y] in MOI.Zeros(2))
+                end
+                @test_throws err begin
+                    @constraint(model_x, [x + y, x] in MOI.Zeros(2))
+                end
+            end
+            @testset "objective" begin
+                @test_throws err @objective(model_x, Min, x + y)
+            end
+        end
+        @testset "Quadratic" begin
+            @testset "constraint" begin
+                @test_throws err @constraint(model_x, x * y >= 1)
+                @test_throws err begin
+                    @constraint(model_x, [x, x * y] in MOI.Zeros(2))
+                end
+                @test_throws err begin
+                    @constraint(model_x, [x * y, x] in MOI.Zeros(2))
+                end
+                @test_throws err @constraint(model_x, x * x + x + y <= 1)
+                @test_throws err begin
+                    @constraint(model_x, [x, x * x + x + y] in MOI.Zeros(2))
+                end
+                @test_throws err begin
+                    @constraint(model_x, [x * x + x + y, x] in MOI.Zeros(2))
+                end
+            end
+            @testset "objective" begin
+                @test_throws err @objective(model_x, Min, x * y)
+                @test_throws err @objective(model_x, Min, x * x + x + y)
+            end
+        end
+        @testset "Attribute" begin
+            cy = @constraint(model_y, y in MOI.EqualTo(1.0))
+            cerr = JuMP.ConstraintNotOwned(cy)
+            @testset "get" begin
+                @test_throws err begin
+                    MOI.get(model_x, MOI.VariablePrimalStart(), y)
+                end
+                @test_throws cerr begin
+                    MOI.get(model_x, MOI.ConstraintPrimalStart(), cy)
+                end
+            end
+            @testset "set" begin
+                @test_throws err begin
+                    MOI.set(model_x, MOI.VariablePrimalStart(), y, 1.0)
+                end
+                @test_throws cerr begin
+                    MOI.set(model_x, MOI.ConstraintPrimalStart(), cy, 1.0)
+                end
+            end
+        end
+    end
+
     @testset "optimize_hook" begin
         m = Model()
         @test m.optimize_hook === nothing