More sanity checking

Author: BenChung

src/systems/callbacks.jl

@@ -818,10 +818,10 @@ function compile_user_affect(affect::FunctionalAffect, cb, sys, dvs, ps; kwargs.
     end
 end
 
-invalid_variables(sys, expr) = filter(x -> !any(isequal(x), all_symbols(sys)), vars(expr))
+invalid_variables(sys, expr) = filter(x -> !any(isequal(x), all_symbols(sys)), reduce(vcat, vars(expr); init=[]))
 function unassignable_variables(sys, expr) 
     assignable_syms = vcat(unknowns(sys), parameters(sys))
-    return filter(x -> !any(isequal(x), assignable_syms), vars(expr))
+    return filter(x -> !any(isequal(x), assignable_syms), reduce(vcat, vars(expr); init=[]))
 end
 
 function compile_user_affect(affect::MutatingFunctionalAffect, sys, dvs, ps; kwargs...)
@@ -832,6 +832,21 @@ function compile_user_affect(affect::MutatingFunctionalAffect, sys, dvs, ps; kwa
         call the affect method - test if it's OOP or IP using applicable
         unpack and apply the resulting values
     =#
+    function check_dups(syms, exprs) # = (syms_dedup, exprs_dedup)
+        seen = Set{Symbol}()
+        syms_dedup = []; exprs_dedup = []
+        for (sym, exp) in Iterators.zip(syms, exprs)
+            if !in(sym, seen)
+                push!(syms_dedup, sym)
+                push!(exprs_dedup, exp)
+                push!(seen, sym)
+            else
+                @warn "Expression $(expr) is aliased as $sym, which has already been used. The first definition will be used."
+            end
+        end
+        return (syms_dedup, exprs_dedup)
+    end
+
     obs_exprs = observed(affect)
     for oexpr in obs_exprs 
         invalid_vars = invalid_variables(sys, oexpr)
@@ -840,6 +855,7 @@ function compile_user_affect(affect::MutatingFunctionalAffect, sys, dvs, ps; kwa
         end
     end
     obs_syms = observed_syms(affect)
+    obs_syms, obs_exprs = check_dups(obs_syms, obs_exprs)
     obs_size = size.(obs_exprs) # we will generate a work buffer of a ComponentArray that maps obs_syms to arrays of size obs_size
 
     mod_exprs = modified(affect)
@@ -849,12 +865,18 @@ function compile_user_affect(affect::MutatingFunctionalAffect, sys, dvs, ps; kwa
         end
         invalid_vars = unassignable_variables(sys, mexpr)
         if length(invalid_vars) > 0
-            error("Observed equation $(mexpr) in affect refers to missing variable(s) $(invalid_vars); the variables may not have been added (e.g. if a component is missing) or they may have been reduced away.")
+            error("Modified equation $(mexpr) in affect refers to missing variable(s) $(invalid_vars); the variables may not have been added (e.g. if a component is missing) or they may have been reduced away.")
         end
     end
     mod_syms = modified_syms(affect)
+    mod_syms, mod_exprs = check_dups(mod_syms, mod_exprs)
     _, mod_og_val_fun = build_explicit_observed_function(sys, mod_exprs; return_inplace=true) 
 
+    overlapping_syms = intersect(mod_syms, obs_syms)
+    if length(overlapping_syms) > 0
+        @warn "The symbols $overlapping_syms are declared as both observed and modified; this is a code smell because it becomes easy to confuse them and assign/not assign a value."
+    end
+
     # sanity checks done! now build the data and update function for observed values
     mkzero(sz) = if sz === () 0.0 else zeros(sz) end
     _, obs_fun = build_explicit_observed_function(sys, reduce(vcat, Symbolics.scalarize.(obs_exprs); init = []); return_inplace=true) 

test/symbolic_events.jl

@@ -1021,6 +1021,46 @@ end
     @test all(sol[temp][sol.t .> 1.0] .<= 0.79) && all(sol[temp][sol.t .> 1.0] .>= 0.49)
 end
 
+@testset "MutatingFunctionalAffect errors and warnings" begin 
+    @variables temp(t)
+    params = @parameters furnace_on_threshold=0.5 furnace_off_threshold=0.7 furnace_power=1.0 leakage=0.1 furnace_on::Bool=false
+    eqs = [
+        D(temp) ~ furnace_on * furnace_power - temp^2 * leakage
+    ]
+
+    furnace_off = ModelingToolkit.SymbolicContinuousCallback([temp ~ furnace_off_threshold], 
+        ModelingToolkit.MutatingFunctionalAffect(modified=(; furnace_on), observed=(; furnace_on)) do x, o, i, c
+            x.furnace_on = false
+        end)
+    @named sys = ODESystem(eqs, t, [temp], params; continuous_events = [furnace_off])
+    ss = structural_simplify(sys)
+    @test_logs (:warn, "The symbols Any[:furnace_on] are declared as both observed and modified; this is a code smell because it becomes easy to confuse them and assign/not assign a value.") prob = ODEProblem(ss, [temp => 0.0, furnace_on => true], (0.0, 100.0))
+
+    @variables tempsq(t) # trivially eliminated
+    eqs = [
+        tempsq ~ temp^2
+        D(temp) ~ furnace_on * furnace_power - temp^2 * leakage
+    ]
+
+    furnace_off = ModelingToolkit.SymbolicContinuousCallback([temp ~ furnace_off_threshold], 
+        ModelingToolkit.MutatingFunctionalAffect(modified=(; furnace_on, tempsq), observed=(; furnace_on)) do x, o, i, c
+            x.furnace_on = false
+        end)
+    @named sys = ODESystem(eqs, t, [temp, tempsq], params; continuous_events = [furnace_off])
+    ss = structural_simplify(sys)
+    @test_throws "refers to missing variable(s)" prob = ODEProblem(ss, [temp => 0.0, furnace_on => true], (0.0, 100.0))
+
+    
+    @parameters not_actually_here
+    furnace_off = ModelingToolkit.SymbolicContinuousCallback([temp ~ furnace_off_threshold], 
+        ModelingToolkit.MutatingFunctionalAffect(modified=(; furnace_on), observed=(; furnace_on, not_actually_here)) do x, o, i, c
+            x.furnace_on = false
+        end)
+    @named sys = ODESystem(eqs, t, [temp, tempsq], params; continuous_events = [furnace_off])
+    ss = structural_simplify(sys)
+    @test_throws "refers to missing variable(s)" prob = ODEProblem(ss, [temp => 0.0, furnace_on => true], (0.0, 100.0))
+end
+
 @testset "Quadrature" begin 
     @variables theta(t) omega(t)
     params = @parameters qA=0 qB=0 hA=0 hB=0 cnt=0
@@ -1040,8 +1080,6 @@ end
             return 0 # err is interpreted as no movement
         end
     end
-    # todo: warn about dups
-    # todo: warn if a variable appears in both observed and modified
     qAevt = ModelingToolkit.SymbolicContinuousCallback([cos(100 * theta) ~ 0], 
         ModelingToolkit.MutatingFunctionalAffect((; qB), (; qA, hA, hB, cnt)) do x, o, i, c
             x.hA = x.qA