Skip to content

fix: move std types into std.types module, disambiguate text and date types - #6155

Open
kgutwin wants to merge 3 commits into
PRQL:mainfrom
bioteam:kg/6146/types-module
Open

fix: move std types into std.types module, disambiguate text and date types#6155
kgutwin wants to merge 3 commits into
PRQL:mainfrom
bioteam:kg/6146/types-module

Conversation

@kgutwin

@kgutwin kgutwin commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Resolves #6146.

Moves all current type declarations in std.prql into a dedicated module types { ... }. To preserve backwards compatibility, also adds the std.types ident to the list of redirects on the root module and adjusts a hard coded std.relation reference to be std.types.relation.

Note that this does change "ambiguous name" error messages when the reference is to either the text or date names. Since those names will now resolve to both std.text and std.types.text, the error includes all three potential options (plus the probably expected choice of this.text). To me, the proper fix here is not to stick with the old error message (giving an extra unhelpful name choice doesn't make the underlying issue any worse) but rather to line up for an improvement to the type system that could potentially filter out ambiguous names that don't have the expected type.

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

Verified the restructure end-to-end on this branch: <std.types.text> and <std.types.date> now work as types, std.text.lower still resolves to the module, and bare <text> / <date> and unqualified relation are unaffected. Three things before it lands.

1. Snapshots are stale — test-rust is already red. cargo test -p prqlc fails locally on semantic::resolver::test::test_functions_pipeline, semantic::resolver::transforms::tests::test_aggregate_positional_arg and queries::debug_lineage::set_ops_remove, matching the failing test-rust job. Four snapshot files carry std.prql byte offsets that don't match the committed file, and the drift grows through the file (+2 at type array, +4 at type relation, +8 past the module) — which reads like they were accepted against a slightly different std.prql than the one that got committed. cargo insta test -p prqlc --accept regenerates all of them; I've put the exact corrected values inline.

2. Qualified std.<type> paths stop resolving, not just std.text / std.date. The root redirect rescues unqualified names but can't rescue qualified ones — lookup prepends the redirect to the whole ident, so std.int is tried as std.types.std.int. On 7bd7499, let f = func x <std.int> -> x compiles; on this branch it errors with Unknown name `std.int` . Same for float, bool, time, timestamp, relation, array, range, transform. That's arguably the point of the move and these aliases were mostly inert, so I'm not suggesting a code change — but it is user-facing, and the contributing guide asks for a CHANGELOG.md line for user-facing changes. A Language entry under [unreleased] would cover both this and the ambiguity help-text change. Happy to push it if you'd like.

3. Nothing tests what the PR fixes. The only new assertion is the ambiguity message. A small inline test that <std.types.text> / <std.types.date> resolve as types would pin down the actual resolution of #6146 — right now a regression there would only show up as a stray snapshot diff. Happy to push one.

Nit: "types" is a bare literal in both module.rs and resolver/stmt.rs, while semantic/mod.rs keeps NS_STD / NS_SELF / NS_PARAM as constants — an NS_TYPES alongside them would match the convention.

Separately: this supersedes #6151 (the _self bandaid), which can be closed once this lands.

@kgutwin

kgutwin commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Updated snapshots and added changelog entry.

Regarding testing -- my judgement call is that no additional tests to codify the existence of std.types are necessary. The existing behavior of the parser to directly resolve the date and text words as keywords would make testing this particular behavior hard; testing any other part of this change (such as moving std.relation to std.types.relation) is already covered implicitly by the test suite.

@prql-bot

prql-bot commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Snapshots and changelog both look right to me — the remaining problem is that CI hasn't actually seen them.

No tests run exists for 4b40c944 or 1da66672. gh api "repos/PRQL/prql/actions/runs?head_sha=1da6667228f263af6b597574a69791897e413fee" returns zero runs, and the same for the snapshot commit; the newest tests run on this branch is still 31126422189 on 2e21985b, from before the fix. So the red X on the PR is stale, not a current failure. The cause looks external: GitHub has had a critical open incident against Actions since 15:22 UTC (status page), and four scheduled/pull_request_target runs on this repo from 17:06 onward are still sitting in waiting. Nothing to do on the branch — worth a re-trigger once it clears.

In the meantime I ran the suite locally at 1da66672:

cargo test -p prqlc --quiet
  77 passed / 0 failed  (lib)
  27 passed / 0 failed
 475 passed / 0 failed  (integration)
   3 passed / 0 failed

That includes the three that were failing — test_functions_pipeline, test_aggregate_positional_arg, set_ops_remove — plus test_ambiguous against its new three-candidate help line. Caveat on scope: this is the prqlc crate only, so the prqlc-python debug_prql_lineage / debug_pl_to_lineage snapshots aren't covered by it; their node-id shifts match the cli/test.rs ones exactly, but the test-python job is what actually proves it.

On tests for std.types — your call, and the reasoning holds. For the record I re-checked the behaviour by hand at 1da66672, so it is verified even if not codified: <std.types.text> and <std.types.date> resolve as types, the type-mismatch message reads expected type `date` (no _self leak), std.text.lower still resolves to the module, and bare <text> / <date> are unaffected.

Only thing left from my review is the NS_TYPES nit, which is cosmetic — fine to leave.

Commands and outputs

CI absence:

$ gh api "repos/PRQL/prql/actions/runs?head_sha=1da6667228f263af6b597574a69791897e413fee" --jq '.total_count'
0
$ gh api "repos/PRQL/prql/actions/runs?branch=kg/6146/types-module" --jq '.workflow_runs[] | "\(.name) \(.head_sha[0:8]) \(.conclusion)"'
tests 2e21985b failure
pull-request-target 2e21985b success
tend-review 2e21985b success
$ gh api "repos/PRQL/prql/actions/runs?status=waiting" --jq '.workflow_runs[] | "\(.created_at) \(.name)"'
2026-08-06T21:04:45Z tend-notifications
2026-08-06T19:34:55Z tend-notifications
2026-08-06T17:18:35Z tend-review
2026-08-06T17:06:03Z tend-notifications

Behaviour checks (each piped to cargo run -q -p prqlc -- compile, compiler reports 0.13.14-39-g1da66672):

let f = func x <std.types.text> -> x
from t
derive {y = f "a"}
# -> SELECT *, 'a' AS y FROM t

let f = func x <std.types.date> -> x
from t
derive {y = f @2020-01-01}
# -> SELECT *, DATE '2020-01-01' AS y FROM t

let f = func x <std.types.date> -> x
from t
derive {y = f 1}
# -> function f, param `x` expected type `date`, but found type `int`
#    Help: Type `date` expands to `date`

from t | derive {y = std.text.lower name}
# -> SELECT *, LOWER(name) AS y FROM t

@kgutwin
kgutwin force-pushed the kg/6146/types-module branch from 1da6667 to da35c47 Compare August 7, 2026 13:55
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.

std.prql declares std.text and std.date twice; the type aliases are silently discarded

2 participants