From 7e756b835934033d023ad9aec017d26abda6bbaa Mon Sep 17 00:00:00 2001 From: Markus Kurtz Date: Fri, 1 Mar 2024 00:13:41 +0100 Subject: [PATCH] Fix kwargs and scoping in macro constructors (#1633) Fixes #1630 and improves fix for #1631. --- src/misc/VarNames.jl | 81 +++++++++++++++++++++++++++----------- test/generic/MPoly-test.jl | 52 +++++++++++++++++++++++- 2 files changed, 107 insertions(+), 26 deletions(-) diff --git a/src/misc/VarNames.jl b/src/misc/VarNames.jl index 089504ace4..9597a272f1 100644 --- a/src/misc/VarNames.jl +++ b/src/misc/VarNames.jl @@ -201,6 +201,42 @@ function keyword_arguments(kvs::Tuple{Vararg{Expr}}, default::Dict{Symbol}, return result end +raw""" + normalise_keyword_arguments(args::Union{Tuple, Vector}) :: Vector + +Turn argument list like `(1, a=2; b=3).args` into `(1; a=2, b=3).args`. + +Intended to let a macro call `@m(1, a=2; b=3)` mimic a usual function call. + +```jldoctest; setup = :(using AbstractAlgebra) +julia> args = AbstractAlgebra.normalise_keyword_arguments(:(1, a=2; b=3).args); :($(args...),) +:((1; a = 2, b = 3)) + +julia> macro nka_test(args...) AbstractAlgebra.normalise_keyword_arguments(args) end +@nka_test (macro with 1 method) + +julia> args = @nka_test(1, a=2; b=3); :($(args...),) +:((1; a = 2, b = 3)) + +julia> args = @nka_test(1+2, a=x; b=x^2, kw...); :($(args...),) +:((1 + 2; a = x, b = x^2, kw...)) +``` +""" +function normalise_keyword_arguments(args) + # Keyword arguments after `;` + if Meta.isexpr(first(args), :parameters) + kv = first(args) + args = args[2:end] + else + kv = Expr(:parameters) + end + # Keyword arguments without previous `;` + append!(kv.args, (Expr(:kw, e.args...) for e in args if Meta.isexpr(e, :(=)))) + # normal arguments + args = (e for e in args if !Meta.isexpr(e, :(=))) + return [kv, args...] +end + function _eval(m::Core.Module, e::Expr) try Base.eval(m, e) @@ -287,43 +323,31 @@ function varnames_macro(f, args_count, opt_in) opt_in === :(:yes) || return :() quote macro $f(args...) - # Keyword arguments after `;` end up in `kv`. - # Those without previous `;` get evaluated and end up in `kv2`. - # Note: one could work around evaluating the latter if necessary. - if Meta.isexpr(first(args), :parameters) - kv = first(args) - args = args[2:end] - else - kv = Expr(:parameters) - end - - req(length(args) >= $args_count+1, "Not enough arguments") - base_args = args[1:$args_count] + args = normalise_keyword_arguments(args) + req(length(args) >= $args_count+2, "Not enough arguments") + base_args = args[1:$args_count+1] # includes keyword arguments m = VERSION > v"9999" ? __module__ : $(esc(:__module__)) # julia issue #51602 - s, kv2 = _eval(m, :($$_varnames_macro($(args[$args_count+1:end]...)))) - - append!(kv.args, (Expr(:kw, k, v) for (k, v) in kv2)) - kv = isempty(kv.args) ? () : (kv,) + s = _eval(m, :($$_varnames_macro($(args[$args_count+2:end]...)))) - varnames_macro_code($f, base_args, s, kv) + varnames_macro_code($f, esc.(base_args), s) end end end -_varnames_macro(arg::VarName; kv...) = Symbol(arg), kv -_varnames_macro(args::VarNames...; kv...) = variable_names(args, Val(false)), kv +_varnames_macro(arg::VarName) = Symbol(arg) +_varnames_macro(args::VarNames...) = variable_names(args, Val(false)) -function varnames_macro_code(f, base_args, s::Symbol, kv) +function varnames_macro_code(f, args, s::Symbol) quote - X, $(esc(s)) = $f($(kv...), $(base_args...), $(QuoteNode(s))) + X, $(esc(s)) = $f($(args...), $(QuoteNode(s))) X end end -function varnames_macro_code(f, base_args, s::Vector{Symbol}, kv) +function varnames_macro_code(f, args, s::Vector{Symbol}) quote - X, ($(esc.(s)...),) = $f($(kv...), $(base_args...), $s) + X, ($(esc.(s)...),) = $f($(args...), $s) X end end @@ -362,7 +386,7 @@ Shorthand for `X, x = f(args..., "$s#" => 1:n; kv...)`. The name of the argument `n` can be changed via the `n` option. The range `1:n` is given via the `range` option. Setting `n=:no` disables creation of this method. - + --- X = @f(args..., varnames...; kv...) @@ -408,6 +432,15 @@ julia> @f("hello", "x#" => (1:1, 1:2), "y#" => (1:2), [:z]) julia> (x11, x12, y1, y2, z) ("x11", "x12", "y1", "y2", "z") + +julia> g(a, s::Vector{Symbol}; kv...) = (a, kv...), String.(s) +g (generic function with 1 method) + +julia> AbstractAlgebra.@varnames_interface g(a, s) +@g (macro with 1 method) + +julia> @g("parameters", [:x, :y], a=1, b=2; c=3) +("parameters", :c => 3, :a => 1, :b => 2) ``` """ macro varnames_interface(e::Expr, options::Expr...) diff --git a/test/generic/MPoly-test.jl b/test/generic/MPoly-test.jl index be1f486ef2..ae75f8a7dc 100644 --- a/test/generic/MPoly-test.jl +++ b/test/generic/MPoly-test.jl @@ -140,9 +140,12 @@ QQxxx3 = @polynomial_ring(QQ, :x=>1:3) @test QQxxx_ == (QQxxx3, [x1, x2, x3]) - # test support of kwargs - QQxxx4 = @polynomial_ring(QQ, "x#" => 1:3; internal_ordering=:lex) + # test support of kwargs (issue #1631) + ordering = :lex + QQxxx4 = @polynomial_ring(QQ, "x#" => 1:3; internal_ordering=ordering) @test QQxxx_ == (QQxxx4, [x1, x2, x3]) + QQxxx5 = @polynomial_ring(QQ, "x#" => 1:3, internal_ordering=ordering) + @test QQxxx_ == (QQxxx5, [x1, x2, x3]) QQxxx_deglex_ = @inferred polynomial_ring(QQ, "x#" => 1:3; internal_ordering=:deglex) @test internal_ordering(QQxxx_deglex_[1]) == :deglex @@ -177,6 +180,51 @@ end end +# these variables need to be in global scope +varname_demo = :a => 1:3 +varname_demo0 = "a#" => 1:3 # for use in function constructor +varname_three = 3 +@testset "Generic.MPoly.constructors.macroscoping" begin + # test scoping of variables in macro constructor (issue #1630) + S = ZZ + Sx, x = polynomial_ring(S, :x) + Sx2 = @polynomial_ring(S, :x) + @test Sx == Sx2 + + Svars, _ = polynomial_ring(S, varname_demo0) + Svars1 = @polynomial_ring(S, varname_demo) + Svars2 = @polynomial_ring(S, varname_demo0) + Svars3 = @polynomial_ring(S, :a => 1:varname_three) + @test Svars == Svars1 + @test Svars == Svars2 + @test Svars == Svars3 + + K = QQ + Kxy, _ = polynomial_ring(K, [:x, :y]) + Kxy2 = @polynomial_ring(K, [:x, :y]) + @test Kxy == Kxy2 + + # some more tests for different ways to specify kwargs (issue #1631) + # combined with scoping of keyword and all other args + R = Kxy + cached = true + ordering = :deglex + kwargs = (; internal_ordering = ordering, cached) + Rvars, _ = polynomial_ring(R, varname_demo0; internal_ordering=ordering, cached) + Rvars1 = @polynomial_ring(R, varname_demo; internal_ordering=ordering, cached) + Rvars2 = @polynomial_ring(R, varname_demo, internal_ordering=ordering; cached) + Rvars3 = @polynomial_ring(R, varname_demo, internal_ordering=ordering, cached=cached) + Rvars4 = @polynomial_ring(R, varname_demo, cached=true; kwargs...) + Rvars5 = @polynomial_ring(R, varname_demo; kwargs...) + Rvars6 = @polynomial_ring(R, :a => 1:varname_three; kwargs...) + @test Rvars == Rvars1 + @test Rvars == Rvars2 + @test Rvars == Rvars3 + @test Rvars == Rvars4 + @test Rvars == Rvars5 + @test Rvars == Rvars6 +end + @testset "Generic.MPoly.printing" begin S, (x, y) = ZZ["x", "y"]