Fix kwargs and scoping in macro constructors

Fixes Nemocas#1630 and improves fix for Nemocas#1631.

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...)

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"]