Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Sep 14, 2023
+75 −3
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34952ab
Use let model=model in variable macro to improve type stability
odow Sep 11, 2023
be6beb3
Add to other macros
odow Sep 13, 2023
17429fd
Apply suggestions from code review
odow Sep 13, 2023
45a39c5
Update
odow Sep 14, 2023
2cd6bca
Update
odow Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions src/macros.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1334,7 +1334,13 @@ function _constraint_macro(

creation_code =
Containers.container_code(idxvars, indices, code, requested_container)

# Wrap the entire code block in a let statement to make the model act as
# a type stable local variable.
creation_code = quote
let $model = $model
$creation_code
end
end
if anonvar
# Anonymous constraint, no need to register it in the model-level
# dictionary nor to assign it to a variable in the user scope.
Expand Down Expand Up @@ -1939,6 +1945,13 @@ macro expression(args...)
end
code =
Containers.container_code(idxvars, indices, code, requested_container)
# Wrap the entire code block in a let statement to make the model act as
# a type stable local variable.
creation_code = quote
odow marked this conversation as resolved.
Show resolved Hide resolved
let $m = $m
$creation_code
odow marked this conversation as resolved.
Show resolved Hide resolved
end
end
# don't do anything with the model, but check that it's valid anyway
if anonvar
macro_code = code
Expand Down Expand Up @@ -3016,7 +3029,13 @@ macro variable(args...)
requested_container,
)
end

# Wrap the entire code block in a let statement to make the model act as
# a type stable local variable.
creation_code = quote
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let $model = $model
$creation_code
end
end
if anonvar
# Anonymous variable, no need to register it in the model-level
# dictionary nor to assign it to a variable in the user scope.
Expand Down
6 changes: 6 additions & 0 deletions test/test_hygiene.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,10 @@ Test.@test ex[3] == 6
Test.@test i == 10
Test.@test j == 10

# Test that `model` is inferred correctly inside macros, despite being a
# non-const global.
m = JuMP.Model()
JuMP.@variable(m, x[1:0])
Test.@test x == JuMP.VariableRef[]
odow marked this conversation as resolved.
Show resolved Hide resolved

end