fix: apply redirect correctly when resolving wildcard on this - #5875
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
CI is failing on queries::results::wildcard_this because the new integration test is missing the integration__queries__results__wildcard_this.snap snapshot — task prqlc:test-all (or task prqlc:pull-request) accepts the snapshot locally, but the file then has to be committed alongside the others.
Project guidance (prqlc/prqlc/tests/CLAUDE.md) actually prefers small inline insta::assert_snapshot! tests in prqlc/prqlc/tests/integration/sql.rs over .prql integration tests: each .prql file generates ~6 snapshots, and for a compilation-stage fix like this you can get equivalent coverage with one snapshot. There's a near-twin pattern at tests/integration/sql.rs:6020 (test_select_bare_wildcard). Replacing the new files with a single test_sort_this_wildcard would also sidestep the missing-results-snapshot issue entirely.
Substantively the fix reads correctly to me — the Module::lookup change drops the literal match from res only when a redirect actually resolves the same ident non-empty, and the only place that ambiguity arises in practice is _self lookups (since columns live in per-input sub-modules, not directly under this). The resolve_ident_wildcard cleanup harmonizes nicely with resolve_ident_core/resolve_ident_fallback.
The following PRQL fails compilation:
With the error:
This turns out to be because the process for resolving the wildcard
this.*ends up returning the incorrect tuple{albums = {this.albums.album_id, this.albums.title, this.albums.artist_id}}as seen in the debug log below:The tuple instead should be
{this.albums.album_id, this.albums.title, this.albums.artist_id}.The underlying cause of this issue is in this section of
resolve_ident_wildcard():prql/prqlc/prqlc/src/semantic/resolver/names.rs
Lines 221 to 228 in 6ffb2b9
When
ident_selfis["this", "_self"],self.root_mod.module.lookup(&ident_self)returns a HashSet containing bothident_selfand the redirect (["this", "albums", "_self"]). However, the nextif res.contains()statement drops the redirect, so the value ofmodule_fq_selfends up just being["this", "_self"]. This ends up getting wildcard-expanded to the tuple representing the literal modulethis._self, rather than the correct redirectthis.albums._self.This PR makes two small changes:
Module::lookup(), we remove the ident that triggered the redirect from the result set. This avoids inadvertently triggering "ambiguous name" results.resolve_ident_wildcard()function has been refactored to use the samematch decls.len()structure as used in otherresolve_identfunctions. This harmonizes error conditions across the name resolve process and avoids the bug behavior triggered by the currentif res.contains()logic.