Skip to content

fix: backtick parameter names in transform argument errors - #6165

Merged
max-sixty merged 1 commit into
mainfrom
fix/transform-param-backticks
Aug 9, 2026
Merged

fix: backtick parameter names in transform argument errors#6165
max-sixty merged 1 commit into
mainfrom
fix/transform-param-backticks

Conversation

@prql-bot

@prql-bot prql-bot commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

#6104 moved join's side, append's by, tuple_uniq's take and from_text's format onto enum types, and in the process the reachable error path for each moved from a hand-written Error::new(Reason::Expected { … }) to the who argument of try_cast — which was spelled without backticks. The snapshot in that PR records the change:

-  `take` expected early or late, but found bar
+  take expected early or late, but found `this.foo.bar`

The hand-written arms are still there a few lines below each try_cast, still spelled `take` / `side` / `by` / `format`, so the two paths in the same block disagree on how they name the parameter. Everything else in transforms.rs backticks it too — parameter expanding, `parameter `rolling, format!("`{name}`"). This restores the backticks on the four try_cast calls, so the reachable message matches what the rest of the file produces.

The Some("sort") cast at the top of the file is left alone: its who is the transform, not a parameter, and it wasn't touched by #6104.

I only found one existing snapshot covering any of these paths (tuple_uniq_take_wrong), so transform_param_not_a_literal pins the other three. Reaching the cast needs an argument that resolves but isn't a literal — a plain wrong literal like side:"four" is now caught earlier by the enum type check and never gets here — so the new test uses s-strings.

Verification
  • cargo test -p prqlc -p prqlc-parser — green (lib 78 passed / 1 ignored, tests/integration 485 passed / 5 ignored, parser 97 passed).
  • cargo clippy -p prqlc --all-targets, cargo fmt --check -p prqlc — clean.
  • task prqlc:pull-request couldn't run here — it shells out to cargo insta, which isn't on the sandbox's PATH (ci: expose cargo-insta and cargo-nextest to the tend sandbox #6144). Ran cargo test over the same packages instead.
  • No CHANGELOG entry: feat: Add simple enumerations to type system #6104 is unreleased, so the wording this restores is the wording every published version already emits.

@prql-bot prql-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change is correct and the snapshots match what the compiler emits — I verified the four who strings against the hand-written Reason::Expected arms a few lines below each try_cast, and grepped for other recorded copies of these messages (expected inner, left, right or full / position or name / early or late / csv or json); the four snapshots in error_messages.rs are the only ones, so nothing else goes stale.

One leftover of the same class, outside this diff and not a #6104 regression, so worth a separate PR rather than widening this one: take still names itself two different ways depending on which check fires. The range validation in semantic/lowering.rs — the Reason::Expected with expected: "a positive int range" — passes who: Some("take".to_string()) unbackticked, while transforms.rs backticks the same transform name in its expected: "int or range" arm. Both are reachable:

$ echo "from x | take 0..-1" | prqlc compile
   ╰────── take expected a positive int range, but found 0..-1

$ echo "from x | take y" | prqlc compile
   ╰── `take` expected int or range, but found this.x.y

Backticking the lowering.rs one would also touch four snapshots in tests/integration/sql.rs. Happy to push that as a follow-up if it's wanted.

@max-sixty
max-sixty merged commit 86165ba into main Aug 9, 2026
38 checks passed
@max-sixty
max-sixty deleted the fix/transform-param-backticks branch August 9, 2026 11:58
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.

2 participants