JET.jl identifies many problems

[MagicMuscleMan]

JET.jl 0.11.3 finds 217 possible errors in MathOptInterface.jl when using Julia 1.12.4.

While some of them probably are false positives, there are definitely a few locations, where not all possible cases are covered or where the variables are not correctly scoped in all cases.

I guess listing them here is no huge benefit. You can reproduce it with

using JET, MathOptInterface
report_package(MathOptInterface)

In addition to making MathOptInterface.jl more robust, this also slightly simplifies checking for JET problems in packages using MathOptInterface.jl directly or indirectly. Furthermore, not having JET errors should be a step towards JuliaC compatibility.

[odow]

I periodically go through and fix some:

• https://github.com/jump-dev/MathOptInterface.jl/issues?q=JET
• https://github.com/jump-dev/JuMP.jl/issues?q=JET

I'll review PRs that fix any, but this isn't a high priority for me. Note that it's unlikely that full features of MathOptInterface will be compatible with JuliaC's --trim in any near term; there are many parts of the codebase that are purposefully not type stable.

But documenting (and testing) where we're currently at is on my TODO list: jump-dev/JuMP.jl#2348 (comment)

[odow]

Many of these are just wrong.

The first one I get is:

┌ MathOptInterface.VectorOfVariables(variables::Any) @ MathOptInterface /Users/odow/git/jump-dev/MathOptInterface/src/functions.jl:450
│┌ convert(::Type{Vector{MathOptInterface.VariableIndex}}, a::AbstractArray) @ Base ./array.jl:614
││┌ Vector{MathOptInterface.VariableIndex}(B::BitVector) @ Base ./bitarray.jl:496
│││┌ setindex!(A::Vector{MathOptInterface.VariableIndex}, x::Bool, i::Int64) @ Base ./array.jl:985
││││ no matching method found `convert(::Type{MathOptInterface.VariableIndex}, ::Bool)`: convert(T::Type{MathOptInterface.VariableIndex}, x::Bool)
│││└────────────────────

Which points to

MathOptInterface.jl/src/functions.jl

Lines 449 to 451 in ec5c0e3

	struct VectorOfVariables <: AbstractVectorFunction
	variables::Vector{VariableIndex}
	end

I don't see how the JET error is relevant.

I'm more open to issues like this:

┌ add_constrained_variable(model::MathOptInterface.ModelLike, set::Tuple{…}) @ MathOptInterface /Users/odow/git/jump-dev/MathOptInterface/src/variables.jl:167
│┌ add_constrained_variable(m::MathOptInterface.Utilities.CachingOptimizer, set::S) where S<:MathOptInterface.GreaterThan @ MathOptInterface.Utilities /Users/odow/git/jump-dev/MathOptInterface/src/Utilities/cachingoptimizer.jl:472
││ local variable `vindex_optimizer` may be undefined: vindex_optimizer::MathOptInterface.VariableIndex
│└────────────────────

which points to

MathOptInterface.jl/src/Utilities/cachingoptimizer.jl

Lines 441 to 478 in ec5c0e3

	function MOI.add_constrained_variable(
	m::CachingOptimizer,
	set::S,
	) where {S<:MOI.AbstractScalarSet}
	if m.state == ATTACHED_OPTIMIZER
	if m.mode == AUTOMATIC
	try
	vindex_optimizer, cindex_optimizer =
	MOI.add_constrained_variable(
	m.optimizer,
	set,
	)::Tuple{
	MOI.VariableIndex,
	MOI.ConstraintIndex{MOI.VariableIndex,S},
	}
	catch err
	_rethrow_if_not_NotAllowedError(err)
	reset_optimizer(m)
	end
	else
	vindex_optimizer, cindex_optimizer = MOI.add_constrained_variable(
	m.optimizer,
	set,
	)::Tuple{
	MOI.VariableIndex,
	MOI.ConstraintIndex{MOI.VariableIndex,S},
	}
	end
	end
	vindex, cindex = MOI.add_constrained_variable(m.model_cache, set)
	if m.state == ATTACHED_OPTIMIZER
	m.model_to_optimizer_map[vindex] = vindex_optimizer
	m.optimizer_to_model_map[vindex_optimizer] = vindex
	m.model_to_optimizer_map[cindex] = cindex_optimizer
	m.optimizer_to_model_map[cindex_optimizer] = cindex
	end
	return vindex, cindex
	end

but the code infers properly:

julia> @code_warntype MOI.add_constrained_variable(model, MOI.GreaterThan(0.0))
MethodInstance for MathOptInterface.add_constrained_variable(::MathOptInterface.Utilities.CachingOptimizer{MathOptInterface.Utilities.MockOptimizer{MathOptInterface.Utilities.Model{Float64}, Float64}, MathOptInterface.Utilities.Model{Float64}}, ::MathOptInterface.GreaterThan{Float64})
  from add_constrained_variable(m::MathOptInterface.Utilities.CachingOptimizer, set::S) where S<:AbstractScalarSet @ MathOptInterface.Utilities ~/git/jump-dev/MathOptInterface/src/Utilities/cachingoptimizer.jl:441
Static Parameters
  S = MathOptInterface.GreaterThan{Float64}
Arguments
  #self#::Core.Const(MathOptInterface.add_constrained_variable)
  m::MathOptInterface.Utilities.CachingOptimizer{MathOptInterface.Utilities.MockOptimizer{MathOptInterface.Utilities.Model{Float64}, Float64}, MathOptInterface.Utilities.Model{Float64}}
  set::MathOptInterface.GreaterThan{Float64}
Locals
  @_4::Int64
  @_5::Int64
  cindex::MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}
  vindex::MathOptInterface.VariableIndex
  cindex_optimizer::MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}
  vindex_optimizer::MathOptInterface.VariableIndex
  @_10::Int64
  err::Any
Body::Tuple{MathOptInterface.VariableIndex, MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}}
1 ──        Core.NewvarNode(:(@_4))
│           Core.NewvarNode(:(@_5))
│           Core.NewvarNode(:(cindex))
│           Core.NewvarNode(:(vindex))
│           Core.NewvarNode(:(cindex_optimizer))
│           Core.NewvarNode(:(vindex_optimizer))
│    %7   = MathOptInterface.Utilities.:(==)::Core.Const(==)
│    %8   = Base.getproperty(m, :state)::MathOptInterface.Utilities.CachingOptimizerState
│    %9   = MathOptInterface.Utilities.ATTACHED_OPTIMIZER::Core.Const(MathOptInterface.Utilities.ATTACHED_OPTIMIZER)
│    %10  = (%7)(%8, %9)::Bool
└───        goto #9 if not %10
2 ── %12  = MathOptInterface.Utilities.:(==)::Core.Const(==)
│    %13  = Base.getproperty(m, :mode)::MathOptInterface.Utilities.CachingOptimizerMode
│    %14  = MathOptInterface.Utilities.AUTOMATIC::Core.Const(MathOptInterface.Utilities.AUTOMATIC)
│    %15  = (%12)(%13, %14)::Bool
└───        goto #8 if not %15
3 ── %17  = enter #6
4 ── %18  = MathOptInterface.Utilities.MOI::Core.Const(MathOptInterface)
│    %19  = Base.getproperty(%18, :add_constrained_variable)::Core.Const(MathOptInterface.add_constrained_variable)
│    %20  = Base.getproperty(m, :optimizer)::Union{Nothing, MathOptInterface.Utilities.MockOptimizer{MathOptInterface.Utilities.Model{Float64}, Float64}}
│    %21  = (%19)(%20, set)::Tuple{MathOptInterface.VariableIndex, MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}}
│    %22  = MathOptInterface.Utilities.Tuple::Core.Const(Tuple)
│    %23  = MathOptInterface.Utilities.MOI::Core.Const(MathOptInterface)
│    %24  = Base.getproperty(%23, :VariableIndex)::Core.Const(MathOptInterface.VariableIndex)
│    %25  = MathOptInterface.Utilities.MOI::Core.Const(MathOptInterface)
│    %26  = Base.getproperty(%25, :ConstraintIndex)::Core.Const(MathOptInterface.ConstraintIndex)
│    %27  = MathOptInterface.Utilities.MOI::Core.Const(MathOptInterface)
│    %28  = Base.getproperty(%27, :VariableIndex)::Core.Const(MathOptInterface.VariableIndex)
│    %29  = $(Expr(:static_parameter, 1))::Core.Const(MathOptInterface.GreaterThan{Float64})
│    %30  = Core.apply_type(%26, %28, %29)::Core.Const(MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}})
│    %31  = Core.apply_type(%22, %24, %30)::Core.Const(Tuple{MathOptInterface.VariableIndex, MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}})
│    %32  = Core.typeassert(%21, %31)::Tuple{MathOptInterface.VariableIndex, MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}}
│    %33  = Base.indexed_iterate(%32, 1)::Core.PartialStruct(Tuple{MathOptInterface.VariableIndex, Int64}, Any[MathOptInterface.VariableIndex, Core.Const(2)])
│           (vindex_optimizer = Core.getfield(%33, 1))
│           (@_10 = Core.getfield(%33, 2))
│    %36  = @_10::Core.Const(2)
│    %37  = Base.indexed_iterate(%32, 2, %36)::Core.PartialStruct(Tuple{MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}, Int64}, Any[MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}, Core.Const(3)])
│           (cindex_optimizer = Core.getfield(%37, 1))
└───        $(Expr(:leave, :(%17)))
5 ──        goto #7
6 ┄─        (err = $(Expr(:the_exception)))
│    %42  = MathOptInterface.Utilities._rethrow_if_not_NotAllowedError::Core.Const(MathOptInterface.Utilities._rethrow_if_not_NotAllowedError)
│    %43  = err::Any
│           (%42)(%43)
│    %45  = MathOptInterface.Utilities.reset_optimizer::Core.Const(MathOptInterface.Utilities.reset_optimizer)
│           (%45)(m)
└───        $(Expr(:pop_exception, :(%17)))
7 ┄─        goto #9
8 ── %49  = MathOptInterface.Utilities.MOI::Core.Const(MathOptInterface)
│    %50  = Base.getproperty(%49, :add_constrained_variable)::Core.Const(MathOptInterface.add_constrained_variable)
│    %51  = Base.getproperty(m, :optimizer)::Union{Nothing, MathOptInterface.Utilities.MockOptimizer{MathOptInterface.Utilities.Model{Float64}, Float64}}
│    %52  = (%50)(%51, set)::Tuple{MathOptInterface.VariableIndex, MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}}
│    %53  = MathOptInterface.Utilities.Tuple::Core.Const(Tuple)
│    %54  = MathOptInterface.Utilities.MOI::Core.Const(MathOptInterface)
│    %55  = Base.getproperty(%54, :VariableIndex)::Core.Const(MathOptInterface.VariableIndex)
│    %56  = MathOptInterface.Utilities.MOI::Core.Const(MathOptInterface)
│    %57  = Base.getproperty(%56, :ConstraintIndex)::Core.Const(MathOptInterface.ConstraintIndex)
│    %58  = MathOptInterface.Utilities.MOI::Core.Const(MathOptInterface)
│    %59  = Base.getproperty(%58, :VariableIndex)::Core.Const(MathOptInterface.VariableIndex)
│    %60  = $(Expr(:static_parameter, 1))::Core.Const(MathOptInterface.GreaterThan{Float64})
│    %61  = Core.apply_type(%57, %59, %60)::Core.Const(MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}})
│    %62  = Core.apply_type(%53, %55, %61)::Core.Const(Tuple{MathOptInterface.VariableIndex, MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}})
│    %63  = Core.typeassert(%52, %62)::Tuple{MathOptInterface.VariableIndex, MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}}
│    %64  = Base.indexed_iterate(%63, 1)::Core.PartialStruct(Tuple{MathOptInterface.VariableIndex, Int64}, Any[MathOptInterface.VariableIndex, Core.Const(2)])
│           (vindex_optimizer = Core.getfield(%64, 1))
│           (@_5 = Core.getfield(%64, 2))
│    %67  = @_5::Core.Const(2)
│    %68  = Base.indexed_iterate(%63, 2, %67)::Core.PartialStruct(Tuple{MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}, Int64}, Any[MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}, Core.Const(3)])
└───        (cindex_optimizer = Core.getfield(%68, 1))
9 ┄─ %70  = MathOptInterface.Utilities.MOI::Core.Const(MathOptInterface)
│    %71  = Base.getproperty(%70, :add_constrained_variable)::Core.Const(MathOptInterface.add_constrained_variable)
│    %72  = Base.getproperty(m, :model_cache)::MathOptInterface.Utilities.Model{Float64}
│    %73  = (%71)(%72, set)::Tuple{MathOptInterface.VariableIndex, MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}}
│    %74  = Base.indexed_iterate(%73, 1)::Core.PartialStruct(Tuple{MathOptInterface.VariableIndex, Int64}, Any[MathOptInterface.VariableIndex, Core.Const(2)])
│           (vindex = Core.getfield(%74, 1))
│           (@_4 = Core.getfield(%74, 2))
│    %77  = @_4::Core.Const(2)
│    %78  = Base.indexed_iterate(%73, 2, %77)::Core.PartialStruct(Tuple{MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}, Int64}, Any[MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}, Core.Const(3)])
│           (cindex = Core.getfield(%78, 1))
│    %80  = MathOptInterface.Utilities.:(==)::Core.Const(==)
│    %81  = Base.getproperty(m, :state)::MathOptInterface.Utilities.CachingOptimizerState
│    %82  = MathOptInterface.Utilities.ATTACHED_OPTIMIZER::Core.Const(MathOptInterface.Utilities.ATTACHED_OPTIMIZER)
│    %83  = (%80)(%81, %82)::Bool
└───        goto #11 if not %83
10 ─ %85  = Base.getproperty(m, :model_to_optimizer_map)::MathOptInterface.Utilities.IndexMap
│    %86  = vindex_optimizer::MathOptInterface.VariableIndex
│    %87  = vindex::MathOptInterface.VariableIndex
│           Base.setindex!(%85, %86, %87)
│    %89  = Base.getproperty(m, :optimizer_to_model_map)::MathOptInterface.Utilities.IndexMap
│    %90  = vindex::MathOptInterface.VariableIndex
│    %91  = vindex_optimizer::MathOptInterface.VariableIndex
│           Base.setindex!(%89, %90, %91)
│    %93  = Base.getproperty(m, :model_to_optimizer_map)::MathOptInterface.Utilities.IndexMap
│    %94  = cindex_optimizer::MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}
│    %95  = cindex::MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}
│           Base.setindex!(%93, %94, %95)
│    %97  = Base.getproperty(m, :optimizer_to_model_map)::MathOptInterface.Utilities.IndexMap
│    %98  = cindex::MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}
│    %99  = cindex_optimizer::MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}
└───        Base.setindex!(%97, %98, %99)
11 ┄ %101 = vindex::MathOptInterface.VariableIndex
│    %102 = cindex::MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}
│    %103 = Core.tuple(%101, %102)::Tuple{MathOptInterface.VariableIndex, MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.GreaterThan{Float64}}}
└───        return %103

[odow]

After #2908

julia> using JET, MathOptInterface
julia> report_package(MathOptInterface; target_modules = (MathOptInterface,))

julia> report_package(MathOptInterface; target_modules = (MathOptInterface,))
...
[toplevel-info] Analyzed all top-level definitions (all: 6821 | analyzed: 0 | cached: 6787 | took: 0.027 sec)
═════ 54 possible errors found ═════
...

There are quite a few of the remaining that come from the expansion of Test.@testset. Others are because Union{} shows up.

┌ add_constrained_variable(model::MathOptInterface.ModelLike, set::Tuple{…}) @ MathOptInterface /Users/odow/git/jump-dev/MathOptInterface/src/variables.jl:167
│┌ add_constrained_variable(b::MathOptInterface.Bridges.AbstractBridgeOptimizer, set::MathOptInterface.GreaterThan) @ MathOptInterface.Bridges /Users/odow/git/jump-dev/MathOptInterface/src/Bridges/bridge_optimizer.jl:2340
││┌ add_variable(b::MathOptInterface.Bridges.AbstractBridgeOptimizer) @ MathOptInterface.Bridges /Users/odow/git/jump-dev/MathOptInterface/src/Bridges/bridge_optimizer.jl:2242
│││┌ add_constrained_variables(m::MathOptInterface.Utilities.CachingOptimizer, set::MathOptInterface.Reals) @ MathOptInterface.Utilities /Users/odow/git/jump-dev/MathOptInterface/src/Utilities/cachingoptimizer.jl:495
││││┌ bridge_type(b::MathOptInterface.Bridges.LazyBridgeOptimizer, S::Type{MathOptInterface.Reals}) @ MathOptInterface.Bridges /Users/odow/git/jump-dev/MathOptInterface/src/Bridges/lazy_bridge_optimizer.jl:523
│││││┌ bridge_index(b::MathOptInterface.Bridges.LazyBridgeOptimizer, S::Type{MathOptInterface.Reals}) @ MathOptInterface.Bridges /Users/odow/git/jump-dev/MathOptInterface/src/Bridges/lazy_bridge_optimizer.jl:469
││││││┌ node(b::MathOptInterface.Bridges.LazyBridgeOptimizer, S::Type{MathOptInterface.Reals}) @ MathOptInterface.Bridges /Users/odow/git/jump-dev/MathOptInterface/src/Bridges/lazy_bridge_optimizer.jl:301
│││││││┌ _edge(b::MathOptInterface.Bridges.LazyBridgeOptimizer, index::Int64, BT::Type{Union{}}) @ MathOptInterface.Bridges /Users/odow/git/jump-dev/MathOptInterface/src/Bridges/lazy_bridge_optimizer.jl:205
││││││││┌ _variable_nodes(b::MathOptInterface.Bridges.LazyBridgeOptimizer, BT::Type{Union{}}) @ MathOptInterface.Bridges /Users/odow/git/jump-dev/MathOptInterface/src/Bridges/lazy_bridge_optimizer.jl:161
│││││││││ no matching method found `added_constrained_variable_types(::Type{Union{}})`: MathOptInterface.Bridges.added_constrained_variable_types(BT::Type{Union{}})
││││││││└────────────────────

We could perhaps fix a few more, but I'm tempted to close this issue and come back in another year or so.

[MagicMuscleMan]

Thanks for fixing so many JET issues!

Feel free to close this issue at any time. There is no value in tracking things twice.

The first one I get is:

┌ MathOptInterface.VectorOfVariables(variables::Any) @ MathOptInterface /Users/odow/git/jump-dev/MathOptInterface/src/functions.jl:450
│┌ convert(::Type{Vector{MathOptInterface.VariableIndex}}, a::AbstractArray) @ Base ./array.jl:614
││┌ Vector{MathOptInterface.VariableIndex}(B::BitVector) @ Base ./bitarray.jl:496
│││┌ setindex!(A::Vector{MathOptInterface.VariableIndex}, x::Bool, i::Int64) @ Base ./array.jl:985
││││ no matching method found `convert(::Type{MathOptInterface.VariableIndex}, ::Bool)`: convert(T::Type{MathOptInterface.VariableIndex}, x::Bool)
│││└────────────────────

Which points to

MathOptInterface.jl/src/functions.jl

Lines 449 to 451 in ec5c0e3

These are indeed tricky. I focus on the first of these I found, which is the first line in the following because of the Any in the second line of

┌ function_to_moi(::Val{…}, ::Type{…}, object::Dict, name_map::Dict{…}) where T @ MathOptInterface.FileFormats.MOF /home/pat/.julia/packages/MathOptInterface/03Qtw/src/FileFormats/MOF/read.jl:400
│┌ MathOptInterface.VectorOfVariables(variables::Any) @ MathOptInterface /home/pat/.julia/packages/MathOptInterface/03Qtw/src/functions.jl:450
││┌ convert(::Type{Vector{MathOptInterface.VariableIndex}}, a::AbstractArray) @ Base ./array.jl:614
│││┌ Vector{MathOptInterface.VariableIndex}(B::BitVector) @ Base ./bitarray.jl:496
││││┌ setindex!(A::Vector{MathOptInterface.VariableIndex}, x::Bool, i::Int64) @ Base ./array.jl:985
│││││ no matching method found `convert(::Type{MathOptInterface.VariableIndex}, ::Bool)`: convert(T::Type{MathOptInterface.VariableIndex}, x::Bool)
││││└────────────────────

pointing to function_to_moi.

I think this method should not specialize on T as it does not use it, so I suggest to constraint directly to the UnionAll ::Type instead of ::Type{T}.

But the main problem seems to be, that type inference can't infer that if the array comprehension must result in a concrete Vector type, then the elements of this comprehension must be of this type, too. Therefore, instead of constraining the Vector, you should constrain the generated elements. I think type inference should be improved here.

Until then, this results in

function function_to_moi(
    ::Val{:VectorOfVariables},
    ::Type,
    object::Dict,
    name_map::Dict{String,MOI.VariableIndex},
)
    return MOI.VectorOfVariables(
        [
            name_map[variable]::MOI.VariableIndex for variable::String in object["variables"]
        ],
    )
end

with 2 JET warnings gone and type inference of

object = Dict("variables" => fill("abc", 1000))
name_map = Dict("abc" => VariableIndex(1))
@code_warntype MathOptInterface.FileFormats.MOF.function_to_moi(Val(:VectorOfVariables), Type, object, name_map)

going from 50 assignments with 3 lines containing Unions down to 11 assignments with everything fully inferred.

[MagicMuscleMan]

This seems to be even more complicated. The above change still has a function_to_moi mentioned in JET.jl output. Only the following version is clean of function_to_moi, but does not reduce the overall number of possible errors. This means something else comes up. But with so many entries this is hard to identify. So it probably makes sense to use this variant and solve the simpler things first before continuing here.

return MOI.VectorOfVariables(
	[
		name_map[variable]::MOI.VariableIndex for variable::String in object["variables"]
	]::Vector{MOI.VariableIndex},
)