Visit shared subexpressions once in is_canonical

src/Utilities/functions.jl

@@ -1003,15 +1003,19 @@ is_canonical(::Union{MOI.VariableIndex,MOI.VectorOfVariables}) = true
 function is_canonical(f::MOI.ScalarNonlinearFunction)
     # Don't use recursion here. This gets called for all scalar nonlinear
     # constraints.
-    stack = Any[arg for arg in f.args]
+    # A subexpression may appear several times in the expression graph, we
+    # visit it only once.
+    visited = Base.IdSet{MOI.ScalarNonlinearFunction}()
+    stack = MOI.ScalarNonlinearFunction[f]
     while !isempty(stack)
-        arg = pop!(stack)
-        if arg isa MOI.ScalarNonlinearFunction
-            for a in arg.args
-                push!(stack, a)
-            end
-        else
-            if !is_canonical(arg)
+        g = pop!(stack)
+        for arg in g.args
+            if arg isa MOI.ScalarNonlinearFunction
+                if !(arg in visited)
+                    push!(visited, arg)
+                    push!(stack, arg)
+                end
+            elseif !is_canonical(arg)
                 return false
             end
         end

test/Utilities/test_functions.jl

@@ -2101,6 +2101,23 @@ function test_ScalarNonlinearFunction_is_canonical()
     return
 end
 
+function test_is_canonical_ScalarNonlinearFunction_common_subexpressions()
+    x = MOI.VariableIndex(1)
+    # The tree has 2^N leaves, the expression graph has N nodes
+    N = 100
+    f = MOI.ScalarNonlinearFunction(:log, Any[1.0*x])
+    for _ in 1:N
+        f = MOI.ScalarNonlinearFunction(:+, Any[f, f])
+    end
+    @test MOI.Utilities.is_canonical(f)
+    g = MOI.ScalarNonlinearFunction(:log, Any[1.0*x+1.0*x])
+    for _ in 1:N
+        g = MOI.ScalarNonlinearFunction(:+, Any[g, g])
+    end
+    @test !MOI.Utilities.is_canonical(g)
+    return
+end
+
 function test_vector_type()
     for T in (Int, Float64)
         @test MOI.Utilities.vector_type(T) == Vector{T}