internal: stop toposort panicking when a dependency key is repeated

[prql-bot]

toposort builds dag with one node per element of dependencies, but sized nodes by index.len() — the count of distinct keys. The two are equal only when every key is unique. When a key repeats, nodes is short, and visit's self.nodes.get_mut(n).unwrap() panics on any node index past the end. A different arrangement of the same input hangs instead: the driver loop spun while order.len() < dependencies.len() over a range that could never push that many entries, so it retried a completed sort forever.

The fix sizes nodes and order by dependencies.len(), matching dag. With the bound corrected, one pass over every node completes the sort — visit returns immediately for a node already marked done — so the while wrapper is redundant and is dropped.

toposort_tables in semantic/lowering.rs builds its input from a HashMap and always passes Some(main_table), so no compilation path reaches this. It matters because utils is pub under the cli feature, which is on by default, making prqlc::utils::toposort public API of the crate as published.

Follow-up from review: toposort now also carries a doc comment stating its contract — one output element per input element rather than per distinct key, repeated keys permitted with the last declaration winning for edge resolution, undeclared dependency keys ignored, and the start precondition under # Panics. The bare unwrap on an unknown start key keeps its behaviour but gains an expect message; whether that panic should instead be reportable through the return type is left as an open question on the review thread. A later review round found the doc's cycle claim too strong — with start set, only the reachable portion is visited, so a cycle elsewhere goes undetected — so the sentence is qualified and cycle_unreachable_from_root pins it.

Regression test repeated_key panics at toposort.rs:61 on main and passes with the fix. cargo test -p prqlc is green (608 tests), as is cargo clippy -p prqlc --lib -- -D warnings. No changelog entry — no compiler behaviour changes, matching #6190.

Why the test expects `["a", "c", "a"]`

For [("c", vec!["a"]), ("a", vec![]), ("a", vec![])], index maps a to the last entry that declares it, index 2, so dag is [[2], [], []]. Visiting node 0 recurses into node 2 first, giving order [2, 0, 1] — that is, the a that c actually resolves to, then c, then the shadowed a, which nothing depends on. Every input entry appears exactly once and the resolved dependency precedes its dependent.

The hang variant isn't covered by a test on purpose: reproducing it means a test that never returns, which would tie up a CI job rather than fail it. Removing the while makes it unreachable, and repeated_key covers the loop bound that caused both.

task prqlc:pull-request, the gate CLAUDE.md names, could not run — cargo-insta is blocked in the tend sandbox. This is the known problem from #6235; cargo test -p prqlc and the clippy run above are what was actually executed.

[prql-bot]

Traced the fix against the merged tree and it holds up. index collapses repeated keys to the last declaration while dag keeps one entry per element of dependencies, so nodes was the only vector sized off the deduplicated count — the panic and the while spin both follow from that single mismatch, and sizing off dependencies.len() closes both. With the bound corrected the while wrapper really is dead: 0..dependencies.len() reaches every node and visit short-circuits on done, so order lands at exactly dependencies.len() in one pass. repeated_key's expected ["a", "c", "a"] matches what the code produces (dag == [[2], [], []], order [2, 0, 1]), and the reasoning about why is in the test's own comment rather than left for the next reader to re-derive — good.

Ran cargo test -p prqlc --lib -- utils::toposort: 6 passed. The full check rollup on this commit is green.

One adjacent observation, not blocking and not necessarily this PR's job — see the inline note.

[prql-bot]

This line has the same shape as the bug being fixed: a pub function that panics on an input that violates an undocumented precondition. index.get(s).unwrap() aborts when start names a key that isn't in dependencies, which is the same class of caller-visible panic the PR body argues matters here (utils is pub under the default-on cli feature).

I'm deliberately not attaching a suggestion, because the fix isn't mechanical. Propagating with ? would return None, which the function already uses to mean "there's a cycle" — and toposort_tables does toposort(...).unwrap(), so an unknown start key would still panic, just with a worse message than today's. So the choice is between widening the return type, documenting the precondition and keeping the panic as an assertion, or accepting the conflation. That's a semantic call worth a maintainer's opinion rather than something to fold in silently.

Related, and cheaper: toposort has no doc comment at all, and this PR makes its contract subtler — repeated keys are now permitted, later declarations win for edge resolution, and the output has one entry per input element, so it can contain the same key twice. Worth a few lines on the function, since the only place that's written down right now is inside a test.

[prql-bot]

Took the doc half in 4f0d4a2 — toposort now carries a doc comment stating the contract this PR changes: one output element per input element rather than per distinct key, repeated keys permitted with the last declaration winning for edge resolution, dependencies on undeclared keys ignored, and start limiting the output to what's reachable from that key. That was only recorded inside repeated_key's comment before.

Left the semantic call alone, as you framed it. The doc comment records the precondition under a # Panics section and notes why it can't currently be reported through the return type (None already means "there's a cycle"), so the option is documented rather than decided — widening the return type stays open and would edit the same two places.

The one thing I did change on that line is the panic's message. The bare unwrap aborted with the generic "called Option::unwrap() on a None value", which says nothing about what went wrong; it's now an expect reading:

`start` must name a key declared in `dependencies`

Behaviour is identical, only the message differs. Key has no Debug bound, so the offending key itself can't be included without widening the signature.

Verified with cargo test -p prqlc --lib -- utils::toposort (6 passed), cargo clippy -p prqlc --lib -- -D warnings (clean), and cargo fmt -p prqlc -- --check.

[prql-bot]

Checked the new doc comment claim by claim against the code, since a contract statement on a pub function is the kind of prose that goes stale silently. Three of the four hold: edges resolving to the last declaration follows from index being a HashMap collected in order (later inserts overwrite); dependencies on undeclared keys are dropped by the flat_map over index.get(d); and start limiting the output to the reachable entries is what with_root already pins.

The fourth — None if the dependencies contain a cycle — isn't true when start is Some. Inline suggestion for it.

The expect message reads well and stays out of second person. Leaving the semantic question on that line where you put it — it's still a maintainer call, and restating it here wouldn't move it.