From 6caa7a0a56528743db5e969152d775deab0cd1c3 Mon Sep 17 00:00:00 2001 From: odow Date: Sun, 26 May 2019 12:43:59 -0500 Subject: [PATCH 1/6] Add nice error message for splatting in macros --- src/macros.jl | 39 ++++++++++++++++++++++++++++++++++++--- test/macros.jl | 27 +++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/macros.jl b/src/macros.jl index 68e08aea3f6..18ff69d76dd 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -83,6 +83,14 @@ Helper function for macros to construct container objects. Takes an `Expr` that """ _build_ref_sets(c) = _build_ref_sets(c, _get_name(c)) +function _expr_contains_splat(ex::Expr) + if ex.head == :(...) + return true + end + return any(_expr_contains_splat.(ex.args)) +end +_expr_contains_splat(::Any) = false + """ JuMP._get_looped_code(varname, code, condition, idxvars, idxsets, sym, requestedcontainer::Symbol; lowertri=false) @@ -593,6 +601,10 @@ function _constraint_macro(args, macro_name::Symbol, parsefun::Function) # we will wrap in loops to assign to the ConstraintRefs refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) + if any(_expr_contains_splat.(idxsets)) + _error("cannot use splatting operator `...`.") + end + vectorized, parsecode, buildcall = parsefun(_error, x.args...) _add_kw_args(buildcall, kw_args) if vectorized @@ -1008,7 +1020,7 @@ expr = @expression(m, [i=1:3], i*sum(x[j] for j=1:3)) ``` """ macro expression(args...) - + macro_error(str...) = _macro_error(:expression, args, str...) args, kw_args, requestedcontainer = _extract_kw_args(args) if length(args) == 3 m = esc(args[1]) @@ -1019,14 +1031,19 @@ macro expression(args...) c = gensym() x = args[2] else - error("@expression: needs at least two arguments.") + macro_error("needs at least two arguments.") end - length(kw_args) == 0 || error("@expression: unrecognized keyword argument") + length(kw_args) == 0 || macro_error("unrecognized keyword argument") anonvar = isexpr(c, :vect) || isexpr(c, :vcat) || length(args) == 2 variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) + + if any(_expr_contains_splat.(idxsets)) + macro_error("cannot use splatting operator `...`.") + end + newaff, parsecode = _parse_expr_toplevel(x, :q) code = quote q = Val{false}() @@ -1397,6 +1414,10 @@ macro variable(args...) # We now build the code to generate the variables (and possibly the # SparseAxisArray to contain them) refcall, idxvars, idxsets, condition = _build_ref_sets(var, variable) + if any(_expr_contains_splat.(idxsets)) + _error("cannot use splatting operator `...`.") + end + clear_dependencies(i) = (Containers.is_dependent(idxvars,idxsets[i],i) ? () : idxsets[i]) # Code to be used to create each variable of the container. @@ -1510,6 +1531,10 @@ macro NLconstraint(m, x, extra...) # Strategy: build up the code for non-macro add_constraint, and if needed # we will wrap in loops to assign to the ConstraintRefs refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) + if any(_expr_contains_splat.(idxsets)) + error("@NLconstraint: cannot use splatting operator `...`.") + end + # Build the constraint if isexpr(x, :call) # one-sided constraint # Simple comparison - move everything to the LHS @@ -1606,6 +1631,10 @@ macro NLexpression(args...) variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) + if any(_expr_contains_splat.(idxsets)) + error("@NLexpression: cannot use splatting operator `...`.") + end + code = quote $(refcall) = NonlinearExpression($(esc(m)), $(_process_NL_expr(m, x))) end @@ -1672,6 +1701,10 @@ macro NLparameter(m, ex, extra...) variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) + if any(_expr_contains_splat.(idxsets)) + error("@NLparameter: cannot use splatting operator `...`.") + end + code = quote if !isa($(esc(x)), Number) error(string("in @NLparameter (", $(string(ex)), "): expected ", diff --git a/test/macros.jl b/test/macros.jl index 46a2a8db503..01cbe56c554 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -481,6 +481,33 @@ end c = @NLconstraint(model, x == sum(1.0 for i in 1:0)) @test sprint(show, c) == "x - 0 = 0" || sprint(show, c) == "x - 0 == 0" end + + @testset "Splatting error" begin + model = Model() + A = [1 0; 0 1] + @variable(model, x) + + @test_macro_throws ErrorException( + "In `@variable(model, y[axes(A)...])`: cannot use splatting operator `...`." + ) @variable(model, y[axes(A)...]) + + @test_macro_throws ErrorException( + "In `@constraint(model, [i = [axes(A)...]], x >= i)`: cannot use splatting operator `...`." + ) @constraint(model, [i=[axes(A)...]], x >= i) + + @test_macro_throws ErrorException( + "@NLconstraint: cannot use splatting operator `...`." + ) @NLconstraint(model, [i=[axes(A)...]], x >= i) + + @test_macro_throws ErrorException( + "In `@expression(model, [i = [axes(A)...]], i * x)`: cannot use splatting operator `...`." + ) @expression(model, [i=[axes(A)...]], i * x) + + @test_macro_throws ErrorException( + "@NLexpression: cannot use splatting operator `...`." + ) @NLexpression(model, [i=[axes(A)...]], i * x) + end + end @testset "Macros for JuMPExtension.MyModel" begin From 128420efd35c37c543fa81781bdb21ed8a2dbfaa Mon Sep 17 00:00:00 2001 From: odow Date: Sun, 26 May 2019 13:30:01 -0500 Subject: [PATCH 2/6] Move splatting check --- src/macros.jl | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/src/macros.jl b/src/macros.jl index 18ff69d76dd..b752bf14e4f 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -584,6 +584,10 @@ function _constraint_macro(args, macro_name::Symbol, parsefun::Function) c = length(extra) == 1 ? x : gensym() x = length(extra) == 1 ? extra[1] : x + if _expr_contains_splat(c) + _error("cannot use splatting operator `...`.") + end + anonvar = isexpr(c, :vect) || isexpr(c, :vcat) || length(extra) != 1 variable = gensym() name = _get_name(c) @@ -601,10 +605,6 @@ function _constraint_macro(args, macro_name::Symbol, parsefun::Function) # we will wrap in loops to assign to the ConstraintRefs refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) - if any(_expr_contains_splat.(idxsets)) - _error("cannot use splatting operator `...`.") - end - vectorized, parsecode, buildcall = parsefun(_error, x.args...) _add_kw_args(buildcall, kw_args) if vectorized @@ -1034,16 +1034,15 @@ macro expression(args...) macro_error("needs at least two arguments.") end length(kw_args) == 0 || macro_error("unrecognized keyword argument") + if _expr_contains_splat(c) + macro_error("cannot use splatting operator `...`.") + end anonvar = isexpr(c, :vect) || isexpr(c, :vcat) || length(args) == 2 variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) - if any(_expr_contains_splat.(idxsets)) - macro_error("cannot use splatting operator `...`.") - end - newaff, parsecode = _parse_expr_toplevel(x, :q) code = quote q = Val{false}() @@ -1410,13 +1409,12 @@ macro variable(args...) final_variable = variable else isa(var,Expr) || _error("Expected $var to be a variable name") - + if _expr_contains_splat(var) + _error("cannot use splatting operator `...`.") + end # We now build the code to generate the variables (and possibly the # SparseAxisArray to contain them) refcall, idxvars, idxsets, condition = _build_ref_sets(var, variable) - if any(_expr_contains_splat.(idxsets)) - _error("cannot use splatting operator `...`.") - end clear_dependencies(i) = (Containers.is_dependent(idxvars,idxsets[i],i) ? () : idxsets[i]) @@ -1525,15 +1523,16 @@ macro NLconstraint(m, x, extra...) c = length(extra) == 1 ? x : gensym() x = length(extra) == 1 ? extra[1] : x + if _expr_contains_splat(c) + error("@NLconstraint: cannot use splatting operator `...`.") + end + anonvar = isexpr(c, :vect) || isexpr(c, :vcat) || length(extra) != 1 variable = gensym() # Strategy: build up the code for non-macro add_constraint, and if needed # we will wrap in loops to assign to the ConstraintRefs refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) - if any(_expr_contains_splat.(idxsets)) - error("@NLconstraint: cannot use splatting operator `...`.") - end # Build the constraint if isexpr(x, :call) # one-sided constraint @@ -1626,14 +1625,14 @@ macro NLexpression(args...) if length(args) > 3 || length(kw_args) > 0 error("in @NLexpression: To many arguments ($(length(args))).") end + if _expr_contains_splat(c) + error("@NLexpression: cannot use splatting operator `...`.") + end anonvar = isexpr(c, :vect) || isexpr(c, :vcat) || length(args) == 2 variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) - if any(_expr_contains_splat.(idxsets)) - error("@NLexpression: cannot use splatting operator `...`.") - end code = quote $(refcall) = NonlinearExpression($(esc(m)), $(_process_NL_expr(m, x))) @@ -1692,7 +1691,9 @@ macro NLparameter(m, ex, extra...) end c = ex.args[2] x = ex.args[3] - + if _expr_contains_splat(c) + error("@NLparameter: cannot use splatting operator `...`.") + end anonvar = isexpr(c, :vect) || isexpr(c, :vcat) if anonvar error("In @NLparameter($m, $ex): Anonymous nonlinear parameter syntax is not currently supported") @@ -1701,9 +1702,6 @@ macro NLparameter(m, ex, extra...) variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) - if any(_expr_contains_splat.(idxsets)) - error("@NLparameter: cannot use splatting operator `...`.") - end code = quote if !isa($(esc(x)), Number) From 6524533bc62d6355585a9f3a4755c6d8ae0c2572 Mon Sep 17 00:00:00 2001 From: odow Date: Sun, 26 May 2019 13:33:00 -0500 Subject: [PATCH 3/6] Remove extra blank lines --- src/macros.jl | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/macros.jl b/src/macros.jl index b752bf14e4f..294a41143b9 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -1042,7 +1042,6 @@ macro expression(args...) variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) - newaff, parsecode = _parse_expr_toplevel(x, :q) code = quote q = Val{false}() @@ -1415,7 +1414,6 @@ macro variable(args...) # We now build the code to generate the variables (and possibly the # SparseAxisArray to contain them) refcall, idxvars, idxsets, condition = _build_ref_sets(var, variable) - clear_dependencies(i) = (Containers.is_dependent(idxvars,idxsets[i],i) ? () : idxsets[i]) # Code to be used to create each variable of the container. @@ -1533,7 +1531,6 @@ macro NLconstraint(m, x, extra...) # Strategy: build up the code for non-macro add_constraint, and if needed # we will wrap in loops to assign to the ConstraintRefs refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) - # Build the constraint if isexpr(x, :call) # one-sided constraint # Simple comparison - move everything to the LHS @@ -1633,7 +1630,6 @@ macro NLexpression(args...) variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) - code = quote $(refcall) = NonlinearExpression($(esc(m)), $(_process_NL_expr(m, x))) end @@ -1702,7 +1698,6 @@ macro NLparameter(m, ex, extra...) variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) - code = quote if !isa($(esc(x)), Number) error(string("in @NLparameter (", $(string(ex)), "): expected ", From 440bcfbed4741f9e3ac2efe59703482c7c634f73 Mon Sep 17 00:00:00 2001 From: odow Date: Mon, 27 May 2019 15:52:18 -0500 Subject: [PATCH 4/6] Only test splatting on idxsets --- src/macros.jl | 46 +++++++++++++++++++++++----------------------- test/macros.jl | 12 ++++++++---- 2 files changed, 31 insertions(+), 27 deletions(-) diff --git a/src/macros.jl b/src/macros.jl index 294a41143b9..814284ad8cf 100644 --- a/src/macros.jl +++ b/src/macros.jl @@ -83,13 +83,15 @@ Helper function for macros to construct container objects. Takes an `Expr` that """ _build_ref_sets(c) = _build_ref_sets(c, _get_name(c)) -function _expr_contains_splat(ex::Expr) +function _expr_is_splat(ex::Expr) if ex.head == :(...) return true + elseif ex.head == :escape + return _expr_is_splat(ex.args[1]) end - return any(_expr_contains_splat.(ex.args)) + return false end -_expr_contains_splat(::Any) = false +_expr_is_splat(::Any) = false """ JuMP._get_looped_code(varname, code, condition, idxvars, idxsets, sym, requestedcontainer::Symbol; lowertri=false) @@ -584,10 +586,6 @@ function _constraint_macro(args, macro_name::Symbol, parsefun::Function) c = length(extra) == 1 ? x : gensym() x = length(extra) == 1 ? extra[1] : x - if _expr_contains_splat(c) - _error("cannot use splatting operator `...`.") - end - anonvar = isexpr(c, :vect) || isexpr(c, :vcat) || length(extra) != 1 variable = gensym() name = _get_name(c) @@ -604,6 +602,9 @@ function _constraint_macro(args, macro_name::Symbol, parsefun::Function) # Strategy: build up the code for add_constraint, and if needed # we will wrap in loops to assign to the ConstraintRefs refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) + if any(_expr_is_splat.(idxsets)) + _error("cannot use splatting operator `...`.") + end vectorized, parsecode, buildcall = parsefun(_error, x.args...) _add_kw_args(buildcall, kw_args) @@ -1034,14 +1035,14 @@ macro expression(args...) macro_error("needs at least two arguments.") end length(kw_args) == 0 || macro_error("unrecognized keyword argument") - if _expr_contains_splat(c) - macro_error("cannot use splatting operator `...`.") - end anonvar = isexpr(c, :vect) || isexpr(c, :vcat) || length(args) == 2 variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) + if any(_expr_is_splat.(idxsets)) + macro_error("cannot use splatting operator `...`.") + end newaff, parsecode = _parse_expr_toplevel(x, :q) code = quote q = Val{false}() @@ -1408,12 +1409,12 @@ macro variable(args...) final_variable = variable else isa(var,Expr) || _error("Expected $var to be a variable name") - if _expr_contains_splat(var) - _error("cannot use splatting operator `...`.") - end # We now build the code to generate the variables (and possibly the # SparseAxisArray to contain them) refcall, idxvars, idxsets, condition = _build_ref_sets(var, variable) + if any(_expr_is_splat.(idxsets)) + _error("cannot use splatting operator `...`.") + end clear_dependencies(i) = (Containers.is_dependent(idxvars,idxsets[i],i) ? () : idxsets[i]) # Code to be used to create each variable of the container. @@ -1521,16 +1522,15 @@ macro NLconstraint(m, x, extra...) c = length(extra) == 1 ? x : gensym() x = length(extra) == 1 ? extra[1] : x - if _expr_contains_splat(c) - error("@NLconstraint: cannot use splatting operator `...`.") - end - anonvar = isexpr(c, :vect) || isexpr(c, :vcat) || length(extra) != 1 variable = gensym() # Strategy: build up the code for non-macro add_constraint, and if needed # we will wrap in loops to assign to the ConstraintRefs refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) + if any(_expr_is_splat.(idxsets)) + error("@NLconstraint: cannot use splatting operator `...`.") + end # Build the constraint if isexpr(x, :call) # one-sided constraint # Simple comparison - move everything to the LHS @@ -1622,14 +1622,14 @@ macro NLexpression(args...) if length(args) > 3 || length(kw_args) > 0 error("in @NLexpression: To many arguments ($(length(args))).") end - if _expr_contains_splat(c) - error("@NLexpression: cannot use splatting operator `...`.") - end anonvar = isexpr(c, :vect) || isexpr(c, :vcat) || length(args) == 2 variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) + if any(_expr_is_splat.(idxsets)) + error("@NLexpression: cannot use splatting operator `...`.") + end code = quote $(refcall) = NonlinearExpression($(esc(m)), $(_process_NL_expr(m, x))) end @@ -1687,9 +1687,6 @@ macro NLparameter(m, ex, extra...) end c = ex.args[2] x = ex.args[3] - if _expr_contains_splat(c) - error("@NLparameter: cannot use splatting operator `...`.") - end anonvar = isexpr(c, :vect) || isexpr(c, :vcat) if anonvar error("In @NLparameter($m, $ex): Anonymous nonlinear parameter syntax is not currently supported") @@ -1698,6 +1695,9 @@ macro NLparameter(m, ex, extra...) variable = gensym() refcall, idxvars, idxsets, condition = _build_ref_sets(c, variable) + if any(_expr_is_splat.(idxsets)) + error("@NLparameter: cannot use splatting operator `...`.") + end code = quote if !isa($(esc(x)), Number) error(string("in @NLparameter (", $(string(ex)), "): expected ", diff --git a/test/macros.jl b/test/macros.jl index 01cbe56c554..b2ef263119e 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -491,21 +491,25 @@ end "In `@variable(model, y[axes(A)...])`: cannot use splatting operator `...`." ) @variable(model, y[axes(A)...]) + f(a, b) = [a, b] + @variable(model, z[f((1, 2)...)]) + @test length(z) == 2 + @test_macro_throws ErrorException( "In `@constraint(model, [i = [axes(A)...]], x >= i)`: cannot use splatting operator `...`." - ) @constraint(model, [i=[axes(A)...]], x >= i) + ) @constraint(model, [axes(A)...], x >= 1) @test_macro_throws ErrorException( "@NLconstraint: cannot use splatting operator `...`." - ) @NLconstraint(model, [i=[axes(A)...]], x >= i) + ) @NLconstraint(model, [axes(A)...], x >= i) @test_macro_throws ErrorException( "In `@expression(model, [i = [axes(A)...]], i * x)`: cannot use splatting operator `...`." - ) @expression(model, [i=[axes(A)...]], i * x) + ) @expression(model, [axes(A)...], i * x) @test_macro_throws ErrorException( "@NLexpression: cannot use splatting operator `...`." - ) @NLexpression(model, [i=[axes(A)...]], i * x) + ) @NLexpression(model, [axes(A)...], i * x) end end From 5d24b7db4175a45be93fefd078c46aa2c07af96a Mon Sep 17 00:00:00 2001 From: odow Date: Tue, 28 May 2019 08:43:55 -0500 Subject: [PATCH 5/6] Commit unsaved revisions --- test/macros.jl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/macros.jl b/test/macros.jl index b2ef263119e..863bedee394 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -496,20 +496,20 @@ end @test length(z) == 2 @test_macro_throws ErrorException( - "In `@constraint(model, [i = [axes(A)...]], x >= i)`: cannot use splatting operator `...`." + "In `@constraint(model, [axes(A)...], x >= 1)`: cannot use splatting operator `...`." ) @constraint(model, [axes(A)...], x >= 1) @test_macro_throws ErrorException( "@NLconstraint: cannot use splatting operator `...`." - ) @NLconstraint(model, [axes(A)...], x >= i) + ) @NLconstraint(model, [axes(A)...], x >= 1) @test_macro_throws ErrorException( - "In `@expression(model, [i = [axes(A)...]], i * x)`: cannot use splatting operator `...`." - ) @expression(model, [axes(A)...], i * x) + "In `@expression(model, [axes(A)...], 1 * x)`: cannot use splatting operator `...`." + ) @expression(model, [axes(A)...], 1 * x) @test_macro_throws ErrorException( "@NLexpression: cannot use splatting operator `...`." - ) @NLexpression(model, [axes(A)...], i * x) + ) @NLexpression(model, [axes(A)...], 1 * x) end end From 7a1842d41b6d6863be91766e08f1d55ce2e0ee64 Mon Sep 17 00:00:00 2001 From: odow Date: Tue, 28 May 2019 09:08:38 -0500 Subject: [PATCH 6/6] More unsaved changes --- test/macros.jl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/macros.jl b/test/macros.jl index 863bedee394..50847d2a8c6 100644 --- a/test/macros.jl +++ b/test/macros.jl @@ -504,12 +504,12 @@ end ) @NLconstraint(model, [axes(A)...], x >= 1) @test_macro_throws ErrorException( - "In `@expression(model, [axes(A)...], 1 * x)`: cannot use splatting operator `...`." - ) @expression(model, [axes(A)...], 1 * x) + "In `@expression(model, [axes(A)...], x)`: cannot use splatting operator `...`." + ) @expression(model, [axes(A)...], x) @test_macro_throws ErrorException( "@NLexpression: cannot use splatting operator `...`." - ) @NLexpression(model, [axes(A)...], 1 * x) + ) @NLexpression(model, [axes(A)...], x) end end