fix: keep the type and the default on a function parameter in fmt - #6311
fix: keep the type and the default on a function parameter in fmt#6311prql-bot wants to merge 4 commits into
Conversation
The codegen for `ExprKind::Func` wrote `param.ty` for positional parameters but not for the ones in `named_params`, so `prqlc fmt` rewrote `func x<int>:5 -> x` as `func x:5 -> x` and dropped a constraint the compiler enforces. Also handles a `named_params` entry without a default rather than unwrapping it: the parser can't produce one, but a PL AST deserialized from JSON can.
…nd mutate The let-else arms the previous shape needed were unreachable by construction, which codecov reported as uncovered patch lines.
prql-bot
left a comment
There was a problem hiding this comment.
Self-review. Three findings, all of which I'm pushing to this branch.
The positional-parameter loop drops a default the same way named_params dropped the type. The params loop in the Func(c) arm writes param.name and param.ty and never reads param.default_value. That shape is reachable through exactly the entry point this PR's own rationale cites — json::to_pl hands back a pr::ModuleDef that pl_to_prql formats, and nothing there constrains which list a parameter lands in. Verified on this branch: take the AST from let a = func x <int>:5 -> x, move the parameter out of named_params into params, format it, and the result is let a = func x <int> -> x — the :5 is silently gone. That is the same loss of a compiler-enforced constraint the PR is fixing in the other direction, so handling one half and leaving the other is hard to justify.
After the fix the two loops are equivalent, so they should be one. For a parameter with no default, the new named_params body emits exactly the same sequence of opt.consume calls as the params body — name, " ", <ty>, " " — so the type-writing logic is now duplicated with no behavioural difference. Iterating c.params.iter().chain(&c.named_params) with the new body covers both lists and fixes the finding above in the same stroke, rather than adding a second special case beside the first.
No CHANGELOG.md entry. A change to what prqlc fmt emits is user-facing, and development.md under "Commits" asks for "a line in CHANGELOG.md, with {message}, ({@contributor, #X})" when a change is user-facing. The recent prqlc fixes in the unreleased section all carry one.
Smaller: rebuilding test_named_param_without_default around a directly-constructed AST in 6ae7818 dropped the type annotation from it, so the parameter is now x with neither a type nor a default. The combination the new branch actually enables — a named_params entry carrying a type but no default — is no longer exercised by any test. I'm restoring it in the same push.
…t implies The positional loop never read `default_value`, so a `params` entry carrying one had it silently dropped — the mirror of the type being dropped from `named_params`. Both lists are unconstrained in a PL AST deserialized by `json::to_pl`, which `pl_to_prql` accepts. With the type now written in both loops the bodies were identical, so they become one pass over `params.iter().chain(&named_params)`.
prql-bot
left a comment
There was a problem hiding this comment.
Self-review of bda5065d. The three findings from the previous round are addressed: the positional loop now reads default_value, the two loops are one pass over params.iter().chain(&named_params), the changelog entry is there, and param() in the new test carries ty: Some(int) again.
One inaccuracy left, in the changelog. Everything else checked out — the merged loop emits the same bytes as the old params body for a parameter with no default, so the whitespace is unchanged; docs_generator.rs is the only other place that walks named_params and it prints names only, so it has neither the unwrap() nor the dropped-default shape.
Verification
cargo test -p prqlc --lib codegen (20 tests) and cargo clippy -p prqlc --all-targets are clean at bda5065d. Round-tripped through the CLI: func x<int>:5, func a b:1 c, a parenthesised default, and a tuple type/default all format stably, and the reordering of b:1 past c is the pre-existing params-then-named order, not new here.
prqlc fmtdropped the type annotation from a function parameter that has a default value, so formatting a file silently removed a constraint the compiler was enforcing.Before formatting, a mistyped argument is rejected —
f x:"str"givesfunction f, param `x` expected type `int`, but found type `text`. After formatting, the same query compiles clean and emits'str' AS y. The output still parses, so nothing downstream notices; it just means something different.The cause is in the codegen for
ExprKind::Func, which wrote each parameter according to the list it was stored in rather than what it held. The parser partitions parameters by whether they have a default: one with a default goes tonamed_params, one without toparams. A type annotation is orthogonal to that, but thenamed_paramsloop wrote only the name and the default — sox<int>:5lost its<int>whilex<int>kept it.The
paramsloop had the mirror of the same bug: it wrote the name and the type and never readdefault_value, so aparamsentry carrying a default had it silently dropped. Neither shape is reachable from the parser, butpl_to_prqlalso accepts a PL AST deserialized from JSON throughprqlc::json::to_pl— both are documented entry points on the crate — and nothing there constrains which list a parameter lands in. Thenamed_paramscase was worse still: an entry without a default hit anunwrap()and panicked.Each parameter is now written as what it actually holds — the type when there is one,
:defaultwhen there is one. That makes both loop bodies identical, so they become a single pass overparams.iter().chain(&named_params). A parameter with no default is written as the positional parameter it is; one with a default is written as named.test_typed_func_paramscovers the round-trip for typed positional and typed defaulted parameters, and fails against the original codegen.test_func_params_mismatching_their_listbuilds the two AST shapes the parser can't reach and asserts the formatted output rather than a panic or a dropped default.Verification
cargo test -p prqlcandcargo clippy -p prqlc --all-targetsare clean atbda5065d.60d1a90conly rewords one line of the changelog entry, andprettierleaves it at that wrapping.