Skip to content

fix: only treat a lone tuple arg to std.not as column exclusion - #6136

Merged
kgutwin merged 5 commits into
mainfrom
fix/not-exclusion-panic
Aug 4, 2026
Merged

fix: only treat a lone tuple arg to std.not as column exclusion#6136
kgutwin merged 5 commits into
mainfrom
fix/not-exclusion-panic

Conversation

@prql-bot

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

Copy link
Copy Markdown
Collaborator

select !{a} desugars to a std.not call with a single tuple argument, and the resolver special-cases that shape to turn it into a column exclusion. The guard only looked at args[0], so it claimed calls it wasn't written for — with three different bad outcomes:

Query Before After
std.not {a} b panic on exactly_one reports "Too many arguments to function not"
std.not b:1 panic indexing args[0] (empty args) reports "unknown named argument b"
std.not {a} b:1 named arg silently dropped, compiled to SELECT * reports "unknown named argument b"

Requiring args.len() == 1 and named_args.is_empty() keeps the exclusion path for exactly the shape it was written for and sends everything else down the ordinary call path, which already reports these properly:

Error:
   ╭─[ :1:18 ]
   │
 1 │ from x | select (std.not {a} b)
   │                  ──────┬──────
   │                        ╰──────── Too many arguments to function `not`
───╯

select !{a}, select {a, b} | select !{a}, filter !(a == 1) and derive y = !true are unaffected. Regression tests in error_messages.rs cover the two spanned "Too many arguments" cases; the two named-argument snapshots live in bad_error_messages.rs, since that message still has no span and Debug-prints the ident.

Found during the nightly survey of prqlc/prqlc/src/semantic/resolver/expr.rs.

Note for whoever merges: #6125 has merged and is merged in here — its // special case: handle the syntax !{tuple..} comment is kept and the duplicate one this PR had added is gone. #6137 is still open and appends to the same spot at the end of error_messages.rs, so whichever of the two lands second needs a trivial conflict resolution.

The exclusion-syntax guard ignored `named_args`, so `std.not {a} b:1` took the column-exclusion branch and the named argument vanished. Requiring `named_args.is_empty()` sends it down the ordinary call path, which reports it.

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

Fix is right and I confirmed it locally: both panics reproduce on 81dceef and are gone here, select !{a} / select {a, b} | select !{a} / filter !(a == 1) / derive y = !true are unaffected, and the named-arg tightening turns a silent drop (std.not {a} b:1 compiled to SELECT *) into a real error. One test gap inline.

Two coordination notes:

  • #6125 adds a // special case: handle the syntax !{tuple..} comment directly above this same match arm, and #6137 appends to the same spot at the end of error_messages.rs. Both will conflict textually with this PR; whichever lands second needs a trivial resolution, and #6125's comment then duplicates the one added here.
  • The third snapshot pins the unknown named argument ... to closure Some([...]) message, which has no span and Debug-prints the ident. That's pre-existing (the unknown named argument branch in functions.rs) and not this PR's job to fix, but the repo keeps bad_error_messages.rs for exactly this shape — so it may be worth recording there rather than pinning it in the good-messages file.

Comment thread prqlc/prqlc/tests/integration/error_messages.rs Outdated
@prql-bot

prql-bot commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Took the bad_error_messages.rs suggestion — 9264f00 moves both named-argument snapshots into a new not_with_named_arg test there, leaving error_messages::not_with_extra_args holding just the two spanned "Too many arguments" cases. They're still regression tests for the panic (both forms crashed before this PR); recording them under the bad-messages file just marks the message itself as wanting a span and a rendered ident rather than Some(["std", "not"]). I also rewrote the PR description, which described the args[0] panic as latent when it's reachable today.

On the conflicts: #6125 and #6137 are both still open, so whichever of the three lands last does the resolution. #6125's comment above the match arm says the same thing as the one here — if that one merges first I'll drop mine when rebasing rather than leave both.

@prql-bot prql-bot changed the title fix: don't panic on std.not with multiple arguments fix: only treat a lone tuple arg to std.not as column exclusion Aug 4, 2026

@kgutwin kgutwin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I merged #6125 so the added comment here can be removed.

# Conflicts:
#	prqlc/prqlc/src/semantic/resolver/expr.rs
@prql-bot

prql-bot commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Done — 0c47ef4 merges main in and resolves the conflict by keeping #6125's // special case: handle the syntax !{tuple..} via resolve_column_exclusion comment and dropping the duplicate this PR had added. The diff against main is now just the guard itself: expr.rs L164-171. prqlc integration and lib tests pass locally, and CI is green on the merge commit including check-ok-to-merge.

#6137 still appends to the same spot at the end of error_messages.rs, so it'll need the same trivial resolution whenever it lands after this.

@kgutwin
kgutwin merged commit 2e70589 into main Aug 4, 2026
37 checks passed
@kgutwin
kgutwin deleted the fix/not-exclusion-panic branch August 4, 2026 13:29
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