Skip to content

fix: split mixed positional/keyword arguments in the comma form f(a, b = c) - #330

Merged
bvdmitri merged 3 commits into
4.9.0from
fix/328-mixed-comma-kwargs
Sep 21, 2026
Merged

bvdmitri merged 3 commits into
4.9.0from
fix/328-mixed-comma-kwargs

Conversation

@bvdmitri

Copy link
Copy Markdown
Member

Problem

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 "GraphPPL.proxylabel(:anonymous, 1.0, nothing, GraphPPL.False())"

which points at generated code rather than the user's line.

Root cause

convert_to_kwargs_expression was all-or-nothing: it rewrote a call into the keyword form only when is_kwargs_expression(args) held, and that 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:

input args kwargs combine_args
Normal(m; var = v) [:m] [:(var = v)] MixedArguments((m,), (var = v,)) ✅
Normal(m, var = v) [:m, :(var = v)] nothing (m, $(Expr(:kw, :var, :v))) ❌

The trap the issue warns about is real: Julia puts an explicit ; group first, in a :parameters node, so a naive "split off the trailing keywords" breaks 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 second half

convert_anonymous_variables runs after convert_to_kwargs_expression in the pipeline (src/backends/default.jl:8-20) and generates fresh tilde expressions for nested calls, splicing the captured arguments back verbatim — which reintroduced the comma form. That is the path the original report hit, since its repro is a nested call. Fixing only convert_to_kwargs_expression left the reported case still broken; reconstruct_call closes it.

What this does and does not buy

  • A nested deterministic call over constants is evaluated directly, so mixed arguments genuinely work there. This is the shape reported in the issue, and it now builds.
  • 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.

Error message

The materialized-node error is also rewritten. It read MixedArguments not supported for rhs_interfaces when node has to be materialized — rhs_interfaces means nothing to a model author, and this change makes the error easier to reach. It now names the function, reports what it got, and says what to do.

Known adjacent gap, not addressed here

A nested deterministic call with mixed arguments and a stochastic argument fails with MethodError: no method matching filter(::typeof(is_nodelabel), ::MixedArguments) — materialize_anonymous_variable! has no MixedArguments method. Verified this is pre-existing on 4.9.0 and reachable through the semicolon form identically, so it is a separate bug rather than a regression from this change. This PR's contract is that the two spellings behave the same, which now holds in that case too.

Tests

Asserted directly as "the comma and semicolon forms are indistinguishable", across ~, .~ and := — including the Normal(0, 1; a = 1, b = 2) regression guard, both spellings at once (f(a, b = c; d = e)), and a unit testitem for the splitter.

Verified red before the src/ change (5 failed, 6 errored) and green after. Full local suite: 265/265 test items, 76866 pass, 1 broken (pre-existing), Aqua included.

Closes #328

… 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) <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.04%. Comparing base (8882c5b) to head (94d696f).

Files with missing lines Patch % Lines
src/model_macro.jl 89.28% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            4.9.0     #330      +/-   ##
==========================================
+ Coverage   90.96%   91.04%   +0.08%     
==========================================
  Files          16       16              
  Lines        2279     2301      +22     
==========================================
+ Hits         2073     2095      +22     
  Misses        206      206              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bvdmitri and others added 2 commits September 21, 2026 11:21
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@bvdmitri
bvdmitri merged commit 849fd25 into 4.9.0 Sep 21, 2026
7 of 8 checks passed
@bvdmitri
bvdmitri deleted the fix/328-mixed-comma-kwargs branch September 21, 2026 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant