From f5fd93955fdee59b901a8b3ba7b6558f34394372 Mon Sep 17 00:00:00 2001 From: Bagaev Dmitry Date: Mon, 21 Sep 2026 11:01:54 +0200 Subject: [PATCH 1/3] fix: split mixed positional/keyword arguments in the comma form `f(a, b = c)` In Julia `f(a, b = c)` and `f(a; b = c)` are the same call. Inside a `@model` body they were not: only the semicolon form worked, and the comma form failed during macro expansion with `syntax: invalid named tuple element ...`, pointing at generated code rather than at the user's line. `convert_to_kwargs_expression` was all-or-nothing -- it rewrote a call into the keyword form only when `is_kwargs_expression(args)` held, which requires *every* argument to be `:kw`/`:parameters`. Anything mixed fell through untouched, so the `:kw` node stayed among the positional arguments and `combine_args` emitted a tuple literal containing it, which is not valid syntax. Both spellings are now normalized to the same `(positional, keywords)` split. The trap here is that Julia puts an explicit `;` group *first*, in a `:parameters` node, so a naive "split off the trailing keywords" breaks calls like `Normal(0, 1; a = 1, b = 2)`; `split_positional_and_keyword_args` handles both shapes, and rebuilding an already-correct semicolon call reproduces it exactly. The same normalization is applied in `convert_to_anonymous`, which runs *after* `convert_to_kwargs_expression` in the pipeline and generates fresh tilde expressions for nested calls -- splicing the captured arguments back verbatim reintroduced the comma form there. This is the path the original report hit: a nested deterministic call such as `mixed_det(1.0, s = 2.0)` now builds. What this does and does not buy: - A nested deterministic call over constants is evaluated directly, so mixed arguments genuinely work there. That is the shape reported in the issue. - A node that has to be materialized still cannot take both, by design. It now reaches that stated limitation instead of failing as invalid syntax. The materialized-node error is also rewritten. It said "MixedArguments not supported for rhs_interfaces when node has to be materialized" -- `rhs_interfaces` means nothing to someone writing a model, and this change makes the error easier to reach. It now names the function, reports what it got, and says what to do. Tests assert the contract directly: the comma and semicolon forms are indistinguishable, across `~`, `.~` and `:=`, including the `Normal(0, 1; a = 1, b = 2)` case a naive fix would break. Closes #328 Co-Authored-By: Claude Opus 5 (1M context) --- src/graph_engine.jl | 5 +- src/model_macro.jl | 81 +++++++++++++++++++++++++++++--- test/graph_construction_tests.jl | 63 +++++++++++++++++++++++++ test/model_macro_tests.jl | 73 ++++++++++++++++++++++++++++ 4 files changed, 215 insertions(+), 7 deletions(-) diff --git a/src/graph_engine.jl b/src/graph_engine.jl index e8cd49cf..bf8c4c89 100644 --- a/src/graph_engine.jl +++ b/src/graph_engine.jl @@ -2034,8 +2034,11 @@ make_node!(materialize::True, node_type::NodeType, behaviour::NodeBehaviour, mod GraphPPL.default_parametrization(model, node_type, fform, rhs_interfaces) ) +# A node that gets materialized takes its arguments either all positionally or all by name, since the +# two have to be matched against the node's interfaces. Mixing them is reported here rather than +# further down, where the mismatch would surface as an unreadable dispatch failure. make_node!(::True, node_type::NodeType, behaviour::NodeBehaviour, model::Model, ctx::Context, options::NodeCreationOptions, fform::F, lhs_interface::Union{NodeLabel, ProxyLabel, VariableRef}, rhs_interfaces::MixedArguments) where {F} = error( - "MixedArguments not supported for rhs_interfaces when node has to be materialized" + lazy"MixedArguments not supported for `$(fform)`: a node that has to be materialized cannot be called with both positional and keyword arguments. Got $(length(rhs_interfaces.args)) positional argument(s) and the keyword argument(s) $(keys(rhs_interfaces.kwargs)). Use either all positional or all keyword arguments." ) make_node!(materialize::True, node_type::Composite, behaviour::Stochastic, model::Model, ctx::Context, options::NodeCreationOptions, fform::F, lhs_interface::Union{NodeLabel, ProxyLabel, VariableRef}, rhs_interfaces::Tuple{}) where {F} = make_node!( diff --git a/src/model_macro.jl b/src/model_macro.jl index b4f5aaae..75a80275 100644 --- a/src/model_macro.jl +++ b/src/model_macro.jl @@ -276,6 +276,66 @@ end is_kwargs_expression(e) = false +""" + split_positional_and_keyword_args(args::Vector) + +Split a call's argument list into its positional and keyword parts, returning them as a +`(positional, keywords)` tuple. + +Julia writes an explicit `; ...` group into a single leading `:parameters` node, but keyword +arguments written in the comma form stay inline as `:kw` nodes among the positional arguments. +`f(a; b = c)` and `f(a, b = c)` are the same call, so both spellings are normalized here to the +same split. Everything downstream assumes the `:parameters` form, so the comma form has to be +rewritten into it before it reaches `combine_args`. +""" +function split_positional_and_keyword_args(args::Vector) + positional = Any[] + keywords = Any[] + for arg in args + if arg isa Expr && arg.head === :parameters + append!(keywords, arg.args) + elseif arg isa Expr && arg.head === :kw + push!(keywords, arg) + else + push!(positional, arg) + end + end + return positional, keywords +end + +""" + mixed_kwargs_rhs(f, args::Vector, options::Vector) + +Rebuild the right-hand side `f(a, b = c) where { ... }` in the canonical form +`f(a; b = c) where { ... }`, or return `nothing` if `args` holds no such mix and the expression +should be left alone. + +Returns only the right-hand side, because the three operators this serves do not share an +expression head: `~` and `.~` are `:call`s, while `:=` has its own head. +""" +function mixed_kwargs_rhs(f, args::Vector, options::Vector) + positional, keywords = split_positional_and_keyword_args(args) + if isempty(positional) || isempty(keywords) + return nothing + end + return :($f($(positional...); $(keywords...)) where {$(options...)}) +end + +""" + reconstruct_call(f, args::Vector) + +Rebuild the call `f(args...)` with its keyword arguments in the canonical `:parameters` form. + +`convert_anonymous_variables` runs *after* `convert_to_kwargs_expression` in the pipeline, so the +tilde expressions it generates for nested calls never pass through that normalization. Splicing +the captured arguments back verbatim would reintroduce the comma form there, so anything that +generates a new call expression builds it through this function instead. +""" +function reconstruct_call(f, args::Vector) + positional, keywords = split_positional_and_keyword_args(args) + return isempty(keywords) ? :($f($(positional...))) : :($f($(positional...); $(keywords...))) +end + """ convert_to_kwargs_expression(expr::Expr) @@ -289,7 +349,10 @@ function convert_to_kwargs_expression(e::Expr) if GraphPPL.is_kwargs_expression(args) return :($lhs ~ $f(; $(args...)) where {$(options...)}) else - return e + # Mixed positional and keyword arguments, e.g. `f(a, b = c)`. Everything downstream + # expects the keywords in a `:parameters` group, so normalize before giving up. + rhs = GraphPPL.mixed_kwargs_rhs(f, args, options) + return isnothing(rhs) ? e : :($lhs ~ $rhs) end # Logic for .~ operator elseif @capture(e, (lhs_ .~ f_(; kwargs__) where {options__})) @@ -298,7 +361,10 @@ function convert_to_kwargs_expression(e::Expr) if GraphPPL.is_kwargs_expression(args) return :($lhs .~ $f(; $(args...)) where {$(options...)}) else - return e + # Mixed positional and keyword arguments, e.g. `f(a, b = c)`. Everything downstream + # expects the keywords in a `:parameters` group, so normalize before giving up. + rhs = GraphPPL.mixed_kwargs_rhs(f, args, options) + return isnothing(rhs) ? e : :($lhs .~ $rhs) end # Logic for := operator elseif @capture(e, (lhs_ := f_(; kwargs__) where {options__})) @@ -307,7 +373,10 @@ function convert_to_kwargs_expression(e::Expr) if GraphPPL.is_kwargs_expression(args) return :($lhs := $f(; $(args...)) where {$(options...)}) else - return e + # Mixed positional and keyword arguments, e.g. `f(a, b = c)`. Everything downstream + # expects the keywords in a `:parameters` group, so normalize before giving up. + rhs = GraphPPL.mixed_kwargs_rhs(f, args, options) + return isnothing(rhs) ? e : :($lhs := $rhs) end else return e @@ -329,21 +398,21 @@ function convert_to_anonymous(e::Expr, created_by) f = Symbol(string(f)[2:end]) return quote let $sym = GraphPPL.create_anonymous_variable!(__model__, __context__) - $sym .~ $f($(args...)) where {anonymous = true, created_by = $created_by} + $sym .~ $(reconstruct_call(f, args)) where {anonymous = true, created_by = $created_by} end end end sym = gensym(:anon) return quote let $sym = GraphPPL.create_anonymous_variable!(__model__, __context__) - $sym ~ $f($(args...)) where {anonymous = true, created_by = $created_by} + $sym ~ $(reconstruct_call(f, args)) where {anonymous = true, created_by = $created_by} end end elseif @capture(e, f_.(args__)) sym = gensym(:anon) return quote let $sym = GraphPPL.create_anonymous_variable!(__model__, __context__) - $sym .~ $f($(args...)) where {anonymous = true, created_by = $created_by} + $sym .~ $(reconstruct_call(f, args)) where {anonymous = true, created_by = $created_by} end end end diff --git a/test/graph_construction_tests.jl b/test/graph_construction_tests.jl index 50eca372..153196d1 100644 --- a/test/graph_construction_tests.jl +++ b/test/graph_construction_tests.jl @@ -2088,3 +2088,66 @@ end # The node property `name` is untouched, so `is_anonymous` keeps working @test length(collect(filter(v -> is_anonymous(getproperties(model[v])), collect(variable_nodes(model))))) === 2 end + +@testitem "Mixed positional and keyword arguments in the comma form `f(a, b = c)`" begin + using Distributions + import GraphPPL: create_model, getcontext, factor_nodes, variable_nodes + + include("testutils.jl") + + using .TestUtils.ModelZoo + + # `f(a, b = c)` and `f(a; b = c)` are the same call in Julia, but the comma form used to leave + # the keyword among the positional arguments, so the generated code contained a tuple literal + # with a `:kw` node in it and the model failed to even macro-expand: + # syntax: invalid named tuple element ... + # The contract asserted here is that the two spellings are now indistinguishable. + mixed_det(a; s) = a + s + GraphPPL.NodeBehaviour(::TestUtils.TestGraphPPLBackend, ::typeof(mixed_det)) = GraphPPL.Deterministic() + + # A nested deterministic call over constants is evaluated directly, so mixed arguments + # genuinely work here. This is the shape reported in the original issue. + @model function anon_comma() + x ~ NormalMeanVariance(mixed_det(1.0, s = 2.0), 1.0) + end + + @model function anon_semicolon() + x ~ NormalMeanVariance(mixed_det(1.0; s = 2.0), 1.0) + end + + model_comma = create_model(anon_comma()) + model_semicolon = create_model(anon_semicolon()) + @test model_comma isa GraphPPL.Model + @test length(collect(factor_nodes(model_comma))) === length(collect(factor_nodes(model_semicolon))) + @test length(collect(variable_nodes(model_comma))) === length(collect(variable_nodes(model_semicolon))) + + # A node that has to be materialized still cannot take both, but the comma form now reaches + # that stated limitation instead of failing as invalid syntax, exactly like the semicolon form + @model function materialized_comma() + x ~ NormalMeanVariance(0, var = 1) + end + + @model function materialized_semicolon() + x ~ NormalMeanVariance(0; var = 1) + end + + @test_throws "MixedArguments not supported" create_model(materialized_comma()) + @test_throws "cannot be called with both positional and keyword arguments" create_model(materialized_comma()) + @test_throws "MixedArguments not supported" create_model(materialized_semicolon()) + + # `:=` takes the same path and must agree as well + @model function deterministic_comma() + y ~ NormalMeanVariance(0, 1) + z := mixed_det(y, s = 3.0) + x ~ NormalMeanVariance(z, 1.0) + end + + @model function deterministic_semicolon() + y ~ NormalMeanVariance(0, 1) + z := mixed_det(y; s = 3.0) + x ~ NormalMeanVariance(z, 1.0) + end + + @test_throws "MixedArguments not supported" create_model(deterministic_comma()) + @test_throws "MixedArguments not supported" create_model(deterministic_semicolon()) +end diff --git a/test/model_macro_tests.jl b/test/model_macro_tests.jl index 9caacd1a..d0b802a4 100644 --- a/test/model_macro_tests.jl +++ b/test/model_macro_tests.jl @@ -757,6 +757,79 @@ end x ~ Normal(; μ = μ, σ = σ) where {created_by = (x ~ Normal(μ = μ, σ = σ) where {q = MeanField()}), q = MeanField()} end @test_expression_generating apply_pipeline(input, convert_to_kwargs_expression) output + + # Test 28: mixed positional and keyword arguments written in the comma form are split into + # the keyword form, exactly as if they had been written with an explicit `;`. Previously the + # `var = v` stayed among the positional arguments and `combine_args` emitted a tuple literal + # containing a `:kw` node, which is not valid syntax. + input = quote + x ~ Normal(m, var = v) where {created_by = (x ~ Normal(m, var = v))} + end + output = quote + x ~ Normal(m; var = v) where {created_by = (x ~ Normal(m, var = v))} + end + @test_expression_generating apply_pipeline(input, convert_to_kwargs_expression) output + + # Test 29: the same for `.~` + input = quote + x .~ Normal(m, var = v) where {created_by = (x .~ Normal(m, var = v))} + end + output = quote + x .~ Normal(m; var = v) where {created_by = (x .~ Normal(m, var = v))} + end + @test_expression_generating apply_pipeline(input, convert_to_kwargs_expression) output + + # Test 30: ... and for `:=` + input = quote + x := f(m, s = v) where {created_by = (x := f(m, s = v))} + end + output = quote + x := f(m; s = v) where {created_by = (x := f(m, s = v))} + end + @test_expression_generating apply_pipeline(input, convert_to_kwargs_expression) output + + # Test 31: both spellings at once collapse into a single keyword group. Julia puts the explicit + # `;` group first in the argument list, so the keywords from it lead + input = quote + x ~ Normal(m, var = v; mean = q) where {created_by = (x ~ Normal(m, var = v; mean = q))} + end + output = quote + x ~ Normal(m; mean = q, var = v) where {created_by = (x ~ Normal(m, var = v; mean = q))} + end + @test_expression_generating apply_pipeline(input, convert_to_kwargs_expression) output + + # Test 32: a call with several positional arguments and several comma-form keywords + input = quote + x ~ Normal(μ, σ, a = τ, b = θ) where {created_by = (x ~ Normal(μ, σ, a = τ, b = θ))} + end + output = quote + x ~ Normal(μ, σ; a = τ, b = θ) where {created_by = (x ~ Normal(μ, σ, a = τ, b = θ))} + end + @test_expression_generating apply_pipeline(input, convert_to_kwargs_expression) output +end + +@testitem "split_positional_and_keyword_args" begin + import GraphPPL: split_positional_and_keyword_args + import MacroTools: @capture + + include("testutils.jl") + + split_of(s) = (@capture(s, f_(args__)); split_positional_and_keyword_args(args)) + + # Comma form: the keyword sits inline among the positional arguments as a `:kw` node + @test split_of(:(foo(a, b = c))) == (Any[:a], Any[Expr(:kw, :b, :c)]) + + # Semicolon form: Julia collects it into a leading `:parameters` node instead. Both spellings + # mean the same call, so both must produce the same split + @test split_of(:(foo(a; b = c))) == (Any[:a], Any[Expr(:kw, :b, :c)]) + + # Both at once -- the `:parameters` group comes first in the argument list + @test split_of(:(foo(a, b = c; d = e))) == (Any[:a], Any[Expr(:kw, :d, :e), Expr(:kw, :b, :c)]) + + # Degenerate cases: nothing to split + @test split_of(:(foo(a, b))) == (Any[:a, :b], Any[]) + @test split_of(:(foo(a = 1, b = 2))) == (Any[], Any[Expr(:kw, :a, 1), Expr(:kw, :b, 2)]) + @test split_of(:(foo())) == (Any[], Any[]) end @testitem "convert_to_anonymous" begin From de09f967a942654947316d4f7dd1352bf87d2061 Mon Sep 17 00:00:00 2001 From: Bagaev Dmitry Date: Mon, 21 Sep 2026 11:21:43 +0200 Subject: [PATCH 2/3] docs: list the new argument-splitting helpers in the developers guide `makedocs` runs with `missing_docs` as an error, so every docstring in the module has to appear in a canonical `@docs` block. The three new helpers had docstrings but no entry, which failed the Documentation job. Listed next to `is_kwargs_expression` and `convert_to_kwargs_expression`, which is where the rest of the model macro pipeline lives. Co-Authored-By: Claude Opus 5 (1M context) --- docs/src/developers_guide.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/src/developers_guide.md b/docs/src/developers_guide.md index 589ddbb7..31e0f51c 100644 --- a/docs/src/developers_guide.md +++ b/docs/src/developers_guide.md @@ -182,6 +182,9 @@ GraphPPL.keyword_expressions_to_named_tuple GraphPPL.convert_anonymous_variables GraphPPL.is_kwargs_expression GraphPPL.convert_to_kwargs_expression +GraphPPL.split_positional_and_keyword_args +GraphPPL.mixed_kwargs_rhs +GraphPPL.reconstruct_call GraphPPL.convert_deterministic_statement GraphPPL.proxy_args GraphPPL.save_expression_in_tilde From 94d696ff1dfa628a6563adf63f0a24c1a0ed1e4f Mon Sep 17 00:00:00 2001 From: Bagaev Dmitry Date: Mon, 21 Sep 2026 11:23:45 +0200 Subject: [PATCH 3/3] docs: declare the GraphPPL source as a relative path Building the docs required `Pkg.develop(PackageSpec(path=pwd()))` first, which writes an absolute, machine-specific path into `docs/Project.toml`. A `[sources]` entry pointing at `".."` does the same job, is portable, and makes `julia --project=docs docs/make.jl` work directly from a fresh checkout. Co-Authored-By: Claude Opus 5 (1M context) --- docs/Project.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/Project.toml b/docs/Project.toml index b8526875..65a3150b 100644 --- a/docs/Project.toml +++ b/docs/Project.toml @@ -6,5 +6,8 @@ GraphPPL = "b3f8163a-e979-4e85-b43e-1f63d8c8b42c" GraphPlot = "a2cc645c-3eea-5389-862e-a155d0052231" GraphViz = "f526b714-d49f-11e8-06ff-31ed36ee7ee0" +[sources] +GraphPPL = {path = ".."} + [compat] Documenter = "1.0"