Use let model=model in variable macro to improve type stability

[odow]

Closes #3499

We probably need something similar in the other macros as well.

This is quite hard to test, because wrapping things in a function fixes the problem...

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (ca73f21) 98.13% compared to head (2cd6bca) 98.14%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3500   +/-   ##
=======================================
  Coverage   98.13%   98.14%           
=======================================
  Files          37       37           
  Lines        5536     5545    +9     
=======================================
+ Hits         5433     5442    +9     
  Misses        103      103           

Files Changed	Coverage Δ
src/macros.jl	98.00% <100.00%> (+0.02%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[odow]

@ExpandingMan could you try this branch?

[ExpandingMan]

This does seem to work yes. To be honest, I'm rather confused about what's going on here, I'm running inside a function where there should be no problems with inference. Guess it can be chalked up to some weird compiler quirk? 🤷

Regardless, looks fixed, thanks.

[odow]

Do you have a reproducible example with the Any?

What's going on here is a little complicated. The @variable macro ends up creating a function that looks like map(index -> add_variable(model, index...), 1:0) where model is captured in the closure. But when run in global scope (or anywhere that the compiler doesn't know the type of model, it can only prove that add_variable returns GenericVariableRef (or Any, in your case) instead of VariableRef if it knew the type of model.

The solution is to add a let model = model before this map so that the compiler can figure out the type.

[ExpandingMan]

I did get the Any when I was trying in global scope, I did not create an MWE for it happening inside a function.

Since I'm not entirely sure what's going on here, I should resist the urge to be sanctimonious, but, for whatever it's worth, I think this package's macros should output a lot less code, and the vast majority of macros should be exchangeable with simple function calls. Elaborate macro code is way too hard to diagnose and reason about.

[odow]

I think this package's macros should output a lot less code, and the vast majority of macros should be exchangeable with simple function calls. Elaborate macro code is way too hard to diagnose and reason about.

I think we all agree on this. It's an item on our roadmap: https://jump.dev/JuMP.jl/stable/developers/roadmap/

[blegat]

Should this be done in _finalize_macro to fix other macros as well ?

[odow]

So it has to go here, because it if we move it to _finalize_macro, it'll create something like:

quote
    JuMP._valid_model(m, :m)
    let m = m
        begin
            JuMP._error_if_cannot_register(m, :x)
            var"#3###277" = begin
                (JuMP.Containers.container)(((var"##278",)->begin
                            JuMP.add_variable(m, JuMP.model_convert(m, JuMP.build_variable(JuMP.var"#_error#115"{LineNumberNode}(:(#= REPL[5]:1 =#), Core.Box((:m, :(x[1:0])))), JuMP.VariableInfo(false, NaN, false, NaN, false, NaN, false, NaN, false, false))), if JuMP.set_string_names_on_creation(m)
                                    JuMP.string("x", "[", JuMP.string(var"##278"), "]")
                                else
                                    ""
                                end)
                        end), (JuMP.Containers).vectorized_product((JuMP.Base).OneTo(1:0)), JuMP.Containers.AutoContainerType, Any[Symbol("##278")])
            end
            m[:x] = var"#3###277"
            x = var"#3###277"
        end
    end
end

and then the x = var"#3###277" is a local variable inside the let scope.

I've added it to the other macros.

[odow]

This seems to have broken extensions: https://github.com/jump-dev/JuMP.jl/actions/runs/6179053153

[odow]

Ah

julia> using JuMP

julia> model = Model()
A JuMP Model
Feasibility problem with:
Variables: 0
Model mode: AUTOMATIC
CachingOptimizer state: NO_OPTIMIZER
Solver name: No optimizer attached.

julia> @variable(model, x[1:0])
VariableRef[]

julia> m = (; m = model)
(m = A JuMP Model
Feasibility problem with:
Variables: 0
Model mode: AUTOMATIC
CachingOptimizer state: NO_OPTIMIZER
Solver name: No optimizer attached.
Names registered in the model: x,)

julia> @variable(m.m, y[1:0])
ERROR: syntax: invalid let syntax around /Users/oscar/.julia/dev/JuMP/src/macros.jl:3035
Stacktrace:
 [1] top-level scope
   @ REPL[8]:1

[odow]

https://github.com/jump-dev/JuMP.jl/actions/runs/6180324219

[odow]

Okay, updated.

We can fix something like @variable(model, x[1:0]), but we can't fix something like @variable(get_model(), x[1:0]).

This leads to some weird behavior, like get_model() returning Any[], but I don't know what else we can do. We can't know the return type of the vector if we don't know what get_model() returns.

[blegat]

We can fix something like @variable(model, x[1:0]), but we can't fix something like @variable(get_model(), x[1:0]).

I think fixing @variable(model, x[1:0]) is enough