Audit usages of evaluate (#655)

Author: ericphanson

docs/src/developer/contributing.md

@@ -40,6 +40,12 @@ Convex.jl. Let's say you're adding the new function $f$.
  -   Ensure the atom is a mutable struct, so that `objectid` can be called.
  -   Add as a comment a description of what the atom does and its
      parameters.
+ -   Ensure `evaluate` is only called during the definition of `evaluate` itself, from `conic_form!`, or
+     on constants or matrices. Specifically, `evaluate` must not be called on a potentially `fix!`'d variable
+     when the expression tree is being built (for example, when constructing an atom or reformulating),
+     since then any changes to the variable's value (or it being `free!`'d) will not be recognized.
+     See [#653](https://github.com/jump-dev/Convex.jl/issues/653) and [#585](https://github.com/jump-dev/Convex.jl/issues/585)
+     for previous bugs caused by misuse here.
  -   The most mathematically interesting part is the `new_conic_form!`
      function. Following the example in the nuclear norm atom, you'll
      see that you can just construct the problem whose optimal value is

src/atoms/QuantumRelativeEntropyAtom.jl

@@ -159,6 +159,8 @@ function quantum_relative_entropy(
     k::Integer = 3,
     nullspace_tol::Real = 1e-6,
 )
+    # This `evaluate` is safe since it is not a `fix!`ed variable
+    # (it must be a constant or matrix)
     return QuantumRelativeEntropy2Atom(A, evaluate(B), m, k, nullspace_tol)
 end
 
@@ -168,6 +170,8 @@ function quantum_relative_entropy(
     m::Integer = 3,
     k::Integer = 3,
 )
+    # This `evaluate` is safe since it is not a `fix!`ed variable
+    # (it must be a constant or matrix)
     A = evaluate(A)
     return -quantum_entropy(A, m, k) - trace_logm(B, A, m, k)
 end
@@ -179,6 +183,8 @@ function quantum_relative_entropy(
     k::Integer = 0,
     nullspace_tol::Real = 1e-6,
 )
+    # These `evaluate`s are safe since it is not a `fix!`ed variable
+    # (it must be a constant or matrix)
     A, B = evaluate(A), evaluate(B)
     if size(A) != size(B)
         throw(DimensionMismatch("A and B must be the same size"))

src/atoms/TraceLogmAtom.jl

@@ -79,6 +79,8 @@ function trace_logm(
     m::Integer = 3,
     k::Integer = 3,
 )
+    # This `evaluate` is safe since it is not a `fix!`ed variable
+    # (it must be a constant or matrix)
     return TraceLogmAtom(X, evaluate(C), m, k)
 end
 

src/atoms/TraceMpowerAtom.jl

@@ -64,6 +64,8 @@ function trace_mpower(
     t::Rational,
     C::Union{AbstractMatrix,Constant},
 )
+    # This `evaluate` is safe since it is not a `fix!`ed variable
+    # (it must be a constant or matrix)
     return TraceMpowerAtom(A, t, evaluate(C))
 end
 

src/constraints/GenericConstraint.jl

@@ -86,6 +86,9 @@ end
 
 function _add_constraint!(context::Context, c::GenericConstraint)
     if vexity(c.child) == ConstVexity()
+        # This `evaluate` call is safe, since even if it refers to a `fix!`'d variable,
+        # it happens when we are formulating the problem (not at expression-time), so there
+        # is not time for the variable to be re-`fix!`'d to a different value (or `free!`'d)
         if !is_feasible(evaluate(c.child), c.set, CONSTANT_CONSTRAINT_TOL[])
             context.detected_infeasible_during_formulation = true
         end

src/reformulations/quadform.jl

@@ -85,6 +85,8 @@ function quadform(x::AbstractExpr, A::Value; assume_psd = false)
     return factor * square(norm2(P * x))
 end
 
+# These `evaluate` calls are safe since they are not a `fix!`ed variable
+# (must be a constant):
 function quadform(x::Constant, A::AbstractExpr; kwargs...)
     return quadform(evaluate(x), A; kwargs...)
 end

src/reformulations/transpose.jl

@@ -12,6 +12,7 @@ end
 
 LinearAlgebra.adjoint(x::AbstractExpr) = transpose(conj(x))
 
+# These `evaluate` calls are safe since they are not a `fix!`ed variable:
 function LinearAlgebra.transpose(x::Union{Constant,ComplexConstant})
     return constant(transpose(evaluate(x)))
 end