From 417c58d5fed95dc1e1075e92ac3125c0c8a71e0c Mon Sep 17 00:00:00 2001
From: Oscar Dowson <odow@users.noreply.github.com>
Date: Mon, 27 Nov 2023 15:06:13 +1300
Subject: [PATCH] Fix error message for invalid indicator constraints (#3584)

---
 src/indicator.jl        | 39 ++++++++++++++++++++-------------------
 test/test_constraint.jl | 10 ++++++++++
 2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/src/indicator.jl b/src/indicator.jl
index a179fa69aae..fc54d96252e 100644
--- a/src/indicator.jl
+++ b/src/indicator.jl
@@ -16,6 +16,18 @@ function _build_indicator_constraint(
     return VectorConstraint([variable, jump_function(constraint)], set)
 end
 
+function _build_indicator_constraint(
+    _error::Function,
+    lhs::F,
+    ::ScalarConstraint,
+    ::Type{<:MOI.Indicator},
+) where {F}
+    return _error(
+        "unable to build indicator constraint with the left-hand side term " *
+        "`($lhs)::$F`. The left-hand side must be a binary decision variable.",
+    )
+end
+
 function _indicator_variable_set(::Function, variable::Symbol)
     return variable, MOI.Indicator{MOI.ACTIVATE_ON_ONE}
 end
@@ -51,30 +63,19 @@ function parse_constraint_call(
             "Invalid right-hand side `$(rhs)` of indicator constraint. Expected constraint surrounded by `{` and `}`.",
         )
     end
-    rhs_con = rhs.args[1]
-    rhs_vectorized, rhs_parsecode, rhs_buildcall =
-        parse_constraint(_error, rhs_con)
+    rhs_vectorized, rhs_parsecode, rhs_build_call =
+        parse_constraint(_error, rhs.args[1])
     if vectorized != rhs_vectorized
         _error("Inconsistent use of `.` in symbols to indicate vectorization.")
     end
-    if vectorized
-        buildcall = :(
-            _build_indicator_constraint.(
-                $_error,
-                $(esc(variable)),
-                $rhs_buildcall,
-                $S,
-            )
-        )
+    f, lhs_parse_code = _rewrite_expression(variable)
+    push!(rhs_parsecode.args, lhs_parse_code)
+    build_call = if vectorized
+        :(_build_indicator_constraint.($_error, $f, $rhs_build_call, $S))
     else
-        buildcall = :(_build_indicator_constraint(
-            $_error,
-            $(esc(variable)),
-            $rhs_buildcall,
-            $S,
-        ))
+        :(_build_indicator_constraint($_error, $f, $rhs_build_call, $S))
     end
-    return rhs_parsecode, buildcall
+    return rhs_parsecode, build_call
 end
 
 function constraint_string(
diff --git a/test/test_constraint.jl b/test/test_constraint.jl
index 20b43dab372..37585b61d4a 100644
--- a/test/test_constraint.jl
+++ b/test/test_constraint.jl
@@ -1783,4 +1783,14 @@ function test_SkipModelConvertScalarSetWrapper()
     return
 end
 
+function test_indicator_error()
+    model = Model()
+    @variable(model, x[1:2])
+    err = ErrorException(
+        "In `@constraint(model, x[1] >= 0 --> {x[2] == 0})`: unable to build indicator constraint with the left-hand side term `(x[1] >= 0)::JuMP.NonlinearExpr`. The left-hand side must be a binary decision variable.",
+    )
+    @test_throws_strip err @constraint(model, x[1] >= 0 --> {x[2] == 0})
+    return
+end
+
 end  # module