From c7354940af3d2621b97c21bc12ce902f348b3da3 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Tue, 8 Sep 2026 19:07:55 +0100 Subject: [PATCH] refactor(tui): drive cursor context from the token stream (T9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completion decided where the cursor was by scanning strings. Two near-identical scanners did it — `analyze_statement` when the partial query happened to parse, `analyze_partial` when it did not — using `rfind('.')`, `split_whitespace().last()`, `ends_with(" AND")` and `query_upper.contains("SELECT")`. `analyze_partial` even built a token stream first and then ignored it, and a third heuristic (`determine_context`) ran when both returned Unknown. Measured on data/countries.csv: WHERE region = ' WhereClause, 76 column names *inside the string literal* WHERE region = 'Am WhereClause, nothing WHERE region IN ('Asia', ' WhereClause, 76 column names WHERE "name.common" = no comparison operator found at all WHERE region= no comparison operator found at all GROUP BY suggested WHERE and ORDER BY LIMIT suggested a table name Replaced by src/sql/cursor_context.rs, the one owner of *where is the cursor*: truncate at the cursor, tokenize once, and match on the tail of the stream. The last token says what kind of position this is; the token before it disambiguates a bare word. Keywords are Token variants, so a keyword the lexer learns the completer learns with it — the completer knew six, Token::from_keyword knows fifty-five. Three fixes fall out as consequences rather than as separate changes: quoted and dotted columns reach their operator, because columns come from Identifier/QuotedIdentifier joined across Dot instead of `chars().all(|c| c.is_alphanumeric() || c == '_')`; operators no longer need surrounding spaces; and keywords inside a literal stay inside it, because the unterminated-StringLiteral check runs before anything else looks at the stream. The AST the old code went to the trouble of building was consulted only for `stmt.where_clause.is_some()`-shaped questions the tokens answer directly, so detect_cursor_context no longer parses the query — one less full parse per keystroke. 539 lines out of recursive_parser.rs. Settles char-vs-byte offsets rather than deferring them: the lexer gained tokenize_all_with_byte_positions, and the analyzer speaks bytes throughout. tokenize_all_with_positions indexes the internal Vec, while cursor_pos and replace_start (T1) are byte offsets; they agree only on ASCII, and a wrong offset would have silently corrupted a spliced value. T4 is now "put values in the empty vector": CursorContext::InStringLiteral { column, in_list, value_start } is produced, threaded through ParseResult::replace_start and exempted from identifier filtering. The completer deliberately offers nothing there, which is still an improvement on offering column names inside quotes. determine_context / ParseState survive as the Unknown fallback. Removing them is T12, kept separate because ParseState also has users in src/completer.rs and main.rs validation. One test changed rather than added: test_order_by_quoted_partial_completion asserted partial_word == Some("\"Customer"), opening quote included, which was an artefact of the deleted scanner. The partial now comes from the lexer, which has consumed the quote. Nothing filters on partial_word — that is find_completion_token's job and it still sees the quote — so the test's two substantive assertions are untouched. Tests: 20 unit tests in cursor_context.rs, 8 in tests/completion_schema.rs driving the real get_completions entry point against countries.csv. Full suite green: 800 lib, 482 integration, 534 Python, 152/153 examples (the one failure is a pre-existing smoke test with no expectations, on a path completion cannot reach). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JkqLCLn1Usij41xUjMUoPz --- docs/TUI_FEATURES.md | 201 +++++++++++- src/lib.rs | 1 + src/sql/cursor_aware_parser.rs | 84 ++++- src/sql/cursor_context.rs | 584 +++++++++++++++++++++++++++++++++ src/sql/hybrid_parser.rs | 11 + src/sql/mod.rs | 1 + src/sql/parser/ast.rs | 2 +- src/sql/parser/lexer.rs | 26 ++ src/sql/recursive_parser.rs | 566 +------------------------------- tests/completion_schema.rs | 178 ++++++++++ 10 files changed, 1074 insertions(+), 580 deletions(-) create mode 100644 src/sql/cursor_context.rs diff --git a/docs/TUI_FEATURES.md b/docs/TUI_FEATURES.md index aa83ef2c..ffbfe17a 100644 --- a/docs/TUI_FEATURES.md +++ b/docs/TUI_FEATURES.md @@ -53,19 +53,27 @@ dividing line is whether a correct engine would still leave the user annoyed. ## Where this effort is up to -**Phase: groundwork done.** T1 fixed the text half of completion — which span -gets replaced. T2 fixed the data half — what the completer knows about the -columns it is completing. Between them the completer now has both primitives -the remaining entries need: a byte span to splice over, and a typed schema. - -**Recommended order: T3 → T4 → T5**, with **T7** droppable anywhere — it is -independent of the others and mostly deletion. **T8** is done: it was T1's bug -in the other producer of column text, and it left behind -`src/sql/identifier.rs` as the one place the quoting rule lives. T3 is mechanical but wants doing -*before* T4, not as a retrofit. T4 is the first entry that consumes what T2 -captured (`ColumnInfo::cardinality`, `TableInfo::row_count`); those numbers are -already flowing and pinned by tests, so the gate can be designed against real -values rather than guessed at. +**Phase: the three primitives are in place.** T1 fixed which span gets +replaced. T2 fixed what the completer knows about its columns. T9 fixed how it +decides where the cursor is — from the token stream the lexer already produces, +rather than from `rfind` and `split_whitespace` over uppercased text. Two +near-identical string scanners went with it — 539 lines out of +`recursive_parser.rs` — along with the full parse of the query that used to run +on every keystroke. The third heuristic, `determine_context`, is now only +reachable via `Unknown` and is T12's to remove. + +What remains is mostly *content*: the completer now asks the right question in +the right place and has nothing to say in some of them. `WHERE region = ''` +is a recognised value position (`CursorContext::InStringLiteral`) that offers +nothing, because nobody has captured the values yet. + +**Recommended order: T3 → T11 → T4 → T5**, with **T7** (deletion), **T10** +(function lists from the registry) and **T12** (retire the `ParseState` +fallback) droppable anywhere. T3 is mechanical but wants doing before T4 rather +than as a retrofit. T11 is T4's data half and touches the loader rather than the +parser, so the two can run in either order. **T8** is done: it was T1's bug in +the other producer of column text, and it left `src/sql/identifier.rs` as the +one place the quoting rule lives. --- @@ -164,7 +172,7 @@ values rather than guessed at. the current behaviour so a change is visible. ### T3 — Suggestions are untyped strings -- **Status:** 🔴 OPEN — **do this next**; prerequisite for T4 +- **Status:** 🔴 OPEN — prerequisite for T4; **T9 goes first** (see the order above) - **Where:** `ParseResult::suggestions: Vec` and every site that builds one - **Observed:** A flat `Vec` cannot express a display label distinct @@ -178,11 +186,18 @@ values rather than guessed at. than retrofitting. ### T4 — No value completion for low-cardinality columns -- **Status:** 🔴 OPEN — depends on T3; T2 has landed +- **Status:** 🔴 OPEN — depends on **T9** and T3; T2 has landed - **Where:** `detect_cursor_context` in `src/sql/recursive_parser.rs` - **Observed:** `WHERE region = ''` offers nothing. There is `AfterComparisonOp(col, op)` for a cursor *after* an operator, but no context for a cursor *inside* a string literal. +- **Correction (T9, 2026-09-08):** measured, it is worse than "offers nothing" — + typing the opening quote collapses the context to `WhereClause` and the + completer offers **all 100 column names inside the string literal**; type a + letter and they filter to nothing. The missing context is only half of it: + the cursor-position analysis is string scanning that cannot see a literal at + all. **T9 produces `CursorContext::InStringLiteral`; this entry fills it with + values.** Do not start here. - **Why it is worth doing:** on `countries.csv`, `region` has 5 distinct values and `independent` has 2. Typing those from memory — with exact spelling and case — is the single most common friction in filtering unfamiliar data. @@ -194,7 +209,7 @@ values rather than guessed at. values, all unique) gets offered and the feature feels broken. Precedent exists: `advanced_csv_loader` already computes an `is_categorical` flag from a `cardinality_threshold` config (0.5). - - **Where the values come from — the real decision.** Snapshot distinct values + - **Where the values come from — now its own entry, T11.** Snapshot distinct values into the schema at load time for gated columns only, rather than giving the parser a live `DataView`. `infer_column_types()` already builds the distinct `HashSet` and throws it away, so capturing it is nearly free; memory is @@ -206,7 +221,7 @@ values rather than guessed at. gate; the two should probably agree on what "low cardinality" means. ### T5 — `IN (...)` lists do not iterate -- **Status:** 🔴 OPEN — depends on T4 +- **Status:** 🔴 OPEN — depends on T4, and through it on T9 - **Where:** as T4 - **Observed:** N/A — this is the feature T4 exists to enable, logged separately because it is a distinct slice with its own failure mode. @@ -339,3 +354,155 @@ Older, non-living notes that still contain usable thinking: pre-existing formatter bug about *what* it quotes, not *when* — unchanged here, and it wants fixing where the field is retired rather than in the quoting rule. + +### T9 — Cursor context is decided by string scanning, not by the token stream +- **Status:** 🟢 DONE 2026-09-08 +- **Where:** `src/sql/cursor_context.rs` (new), + `src/sql/recursive_parser.rs`, `src/sql/cursor_aware_parser.rs`, + `src/sql/parser/lexer.rs`, `src/sql/hybrid_parser.rs` +- **Observed:** `analyze_partial` called `tokenize_all_with_positions()` and + then decided almost everything from raw text anyway — `trimmed.rfind('.')`, + `before_dot.split_whitespace().last()`, `before_dot.ends_with('"')`, + `extract_partial_at_end`'s `split_whitespace().last()`. The tokens it built + were consulted only for AND/OR, ORDER BY, WHERE, FROM and SELECT. A second, + near-identical scanner (`analyze_statement`) ran whenever the partial query + happened to parse, and a third heuristic (`determine_context`) ran when both + returned `Unknown`. + + Measured against `data/countries.csv` through the ordinary loader, before and + after: + + | Typed | Was | Now | + |---|---|---| + | `WHERE region = ` | `AfterComparison`, `''` | unchanged ✓ | + | `WHERE region = '` | `WhereClause`, **76 column names** | `InStringLiteral(region)`, nothing | + | `WHERE region = 'Am` | `WhereClause`, nothing | `InStringLiteral(region)`, partial `Am` | + | `WHERE region IN ('Asia', '` | `WhereClause`, 76 column names | `InStringLiteral(region, list)` | + | `WHERE "name.common" = ` | `WhereClause`, 76 column names | `AfterComparison(name.common =)`, `''` | + | `WHERE region=` | `WhereClause` | `AfterComparison(region =)` | + | `GROUP BY ` | `AfterTable`, `WHERE`/`ORDER BY` | `GroupByClause`, 76 columns | + | `LIMIT ` | `FromClause`, `countries` | `LimitClause`, nothing | + | `FROM countries ` | `countries` again | `WHERE`/`GROUP BY`/`ORDER BY`/`LIMIT` | + +- **Fixed by:** `src/sql/cursor_context.rs`, the one owner of *where is the + cursor*. The text is truncated at the cursor and tokenized once; context is + then a match on the **tail** of the token stream rather than a series of + backward scans. The last token says what kind of position this is; the token + before it disambiguates a bare word. Keywords are `Token` variants, so a + keyword the lexer learns the completer learns with it. + + `analyze_statement`, `analyze_partial` and their private helpers are gone — + 539 lines out of `recursive_parser.rs` against 13 in. The AST those 539 lines + went to the trouble of building was consulted only for + `stmt.where_clause.is_some()`-shaped questions the token stream answers + directly, so `detect_cursor_context` no longer parses the query at all: one + less full parse per keystroke. + + Three defects fell out as consequences rather than as separate fixes: + - **Quoted and dotted columns reach their operator.** Columns come from + `Token::Identifier`/`QuotedIdentifier` joined across `Token::Dot`, not from + `chars().all(|c| c.is_alphanumeric() || c == '_')`. + - **Operators no longer need surrounding spaces.** `Token::Equal` is + `Token::Equal` whether or not it was typed as `" = "`. + - **Keywords inside a literal stay inside it.** `region = 'GROUP BY ` is a + value position, because the check for an unterminated `Token::StringLiteral` + happens before anything else looks at the stream. +- **The char/byte trap, settled:** `tokenize_all_with_positions` returns indices + into the lexer's `Vec`, while `cursor_pos` and T1's `replace_start` are + byte offsets — they agree only on ASCII. Rather than convert at each use, + the lexer gained `tokenize_all_with_byte_positions`, and the analyzer speaks + bytes throughout. Pinned by a test that puts `ö` in a value and one that + points the cursor at the second byte of it. +- **What T4 now has to do:** `CursorContext::InStringLiteral { column, in_list, + value_start }` is produced, threaded through `ParseResult::replace_start`, and + exempted from identifier filtering. Resolving the column walks back over + values already in the list and over `NOT`, so `region NOT IN ('a', '')` + reports `region` exactly as `region = ''` does. The completer deliberately returns **no** + suggestions for it. T4 is now "put values in the empty vector" — it does not + need to touch the parser, the span logic, or the filter. +- **Left standing, deliberately:** `determine_context` / `ParseState` in + `cursor_aware_parser.rs` remain as the `Unknown` fallback. `Unknown` is now + much harder to reach, but `ParseState` is also used by `src/completer.rs` and + `src/main.rs` validation, so removing the type is its own slice — see T12. +- **Tests:** 19 unit tests in `src/sql/cursor_context.rs` for the analyzer, and + 8 in `tests/completion_schema.rs` driving the real `get_completions` entry + point against `data/countries.csv` so the rows of the table above are pinned + as suggestions, not just as context enums. Full suite green (799 lib + 482 + integration). +- **One test changed rather than added:** `test_order_by_quoted_partial_completion` + asserted `partial_word == Some("\"Customer")`, with the opening quote, which + was an artefact of the deleted scanner. The partial now comes from the lexer, + which has consumed the quote. Nothing filters on `partial_word` — that is + `find_completion_token`'s job and it still sees the quote — so the test's two + substantive assertions are untouched. + +### T10 — Function suggestions are a hand-kept list, not the registry +- **Status:** 🔴 OPEN — small and self-contained +- **Where:** `src/sql/cursor_aware_parser.rs:105,149,250` +- **Observed:** the same 20-entry block (`"ROUND("`, `"ABS("`, `"FLOOR("`, …) + is pasted three times, once per context. The function registry + (`src/sql/functions/mod.rs`) holds ~370 functions and already exposes + `all_functions() -> Vec` and + `get_by_category(FunctionCategory)`. +- **Impact:** CLAUDE.md's first principle — *all functions go through the + registry* — holds everywhere except the place a user actually discovers + functions. A newly registered function stays invisible to completion until + someone remembers to paste it into three lists. The lists are also + context-blind: aggregates get offered in WHERE. +- **Design:** build from `all_functions()`, filtered by `FunctionCategory` per + context. `FunctionSignature.description` is the natural `detail` for T3's + `Suggestion`, so this is cheaper after T3 than before it — but it does not + block T9. Check the module direction first: `sql::functions` and + `sql::cursor_aware_parser` are siblings, so there should be no cycle. + +### T11 — Distinct values are computed at load and thrown away +- **Status:** 🔴 OPEN — the data half of T4 +- **Where:** `DataTable::infer_column_types` (`src/data/datatable.rs:560`), + `DataColumn` (`:69`), `ColumnInfo::from_data_column` +- **Observed:** `infer_column_types` builds a `HashSet` of every + non-null value per column and keeps only `.len()`, into + `DataColumn::unique_values: Option`. The values themselves are dropped + on every load path. +- **Design:** `DataColumn::distinct_values: Option>`, populated only + for columns that pass the cardinality gate, sorted so Tab cycling is stable. + `ColumnInfo` carries them through the existing snapshot, so the completer + reads *schema* and never touches a `DataView` — T2's purity boundary is + preserved and the parser stays testable without a terminal. +- **Why on the table rather than the view:** it is a property of the data, and + T2 already snapshots columns from the source table so that hiding a column in + the TUI does not make it uncompletable. The same argument applies to values. +- **The gate:** absolute cap *and* ratio, or `name.common` (250 distinct across + 250 rows) gets captured and the feature reads as broken. Prior art to + reconcile rather than reinvent: `advanced_csv_loader` has + `cardinality_threshold: 0.5` plus `is_likely_categorical` + (`cardinality < 100 && avg_length < 50`), and the nvim plugin has its own + notion in [`NVIM_SMART_COLUMN_COMPLETION.md`](NVIM_SMART_COLUMN_COMPLETION.md). + The three should agree on what "low cardinality" means. +- **Cost:** bounded by the gate, and the `HashSet` is already being built — so + this is retention, not computation. At the working size that motivated it + (~10k rows of CI output) it is free. +- **Note:** `unique_values` counts `value.to_string()` of non-null values, so + NULL is not a candidate value and the existing count is a lower bound on what + a NULL-aware gate would see. + +### T12 — Retire `ParseState` and the heuristic fallback +- **Status:** 🔴 OPEN — small, and only possible now that T9 has landed +- **Where:** `determine_context` / `get_suggestions_for_context` in + `src/sql/cursor_aware_parser.rs`; `ParseState` in `src/sql/parser/legacy.rs`; + `src/completer.rs`; `src/main.rs` +- **Observed:** `determine_context` is the pre-T9 heuristic — uppercase the + query, `split_whitespace()`, match six keywords, then guess with + `query_upper.contains("SELECT")`. It survives as the fallback for + `CursorContext::Unknown`, which T9 made much harder to reach but did not + make unreachable. +- **Why it was not done with T9:** the `ParseState` *type* has two other users + — `src/completer.rs` (the reedline REPL's completer) and `main.rs`'s query + validation — so deleting it is a different change from stopping the TUI + completer depending on it. Bundling them would have made T9's diff two + unrelated things at once. +- **Design:** establish what still reaches `Unknown` (a test that asserts a + corpus of realistic partial queries never does), then delete + `determine_context` and `get_suggestions_for_context` and let `Unknown` + return no suggestions. Whether `ParseState` itself goes depends on what + `src/completer.rs` should become — which overlaps with the + `CompletionManager` question in *Notes on the current design*. diff --git a/src/lib.rs b/src/lib.rs index 147c6b74..92422d81 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -129,6 +129,7 @@ pub use ui::tui_app; // Re-export SQL modules for backward compatibility pub use sql::cache; pub use sql::cursor_aware_parser; +pub use sql::cursor_context; pub use sql::hybrid_parser; pub use sql::parser; pub use sql::recursive_parser; diff --git a/src/sql/cursor_aware_parser.rs b/src/sql/cursor_aware_parser.rs index 1e8742bb..ab656cf5 100644 --- a/src/sql/cursor_aware_parser.rs +++ b/src/sql/cursor_aware_parser.rs @@ -136,6 +136,50 @@ impl CursorAwareParser { let tables = self.schema.get_table_names(); (tables, "FromClause".to_string()) } + CursorContext::AfterTable => { + // The table name is complete, so what follows is a clause. + // Previously this position suggested the table name again. + ( + vec![ + "WHERE".to_string(), + "GROUP BY".to_string(), + "ORDER BY".to_string(), + "LIMIT".to_string(), + ], + "AfterTable".to_string(), + ) + } + CursorContext::GroupByClause => { + let cols = self.column_suggestions(&default_table); + (cols, "GroupByClause".to_string()) + } + CursorContext::HavingClause => { + let mut cols = self.column_suggestions(&default_table); + // TODO(T10): from the function registry, filtered to aggregates. + cols.extend( + ["COUNT(", "SUM(", "AVG(", "MIN(", "MAX("] + .into_iter() + .map(String::from), + ); + (cols, "HavingClause".to_string()) + } + CursorContext::LimitClause => { + // A row count. Nothing in the schema helps, and the old + // fallback offered a table name here. + (Vec::new(), "LimitClause".to_string()) + } + CursorContext::InStringLiteral { + column, in_list, .. + } => { + // T4 fills this with the column's distinct values. Offering + // nothing is the correct answer until it does — before T9 this + // position offered every column name, inside the quotes. + let described = column.as_deref().unwrap_or("?"); + ( + Vec::new(), + format!("InStringLiteral({described}, list={in_list})"), + ) + } CursorContext::WhereClause | CursorContext::AfterLogicalOp(_) => { // We're in WHERE clause or after AND/OR - suggest columns let mut suggestions = self @@ -313,7 +357,10 @@ impl CursorAwareParser { cursor_context, CursorContext::AfterColumn(_) | CursorContext::InMethodCall(_, _) ); - let is_value_context = matches!(cursor_context, CursorContext::AfterComparisonOp(_, _)); + let is_value_context = matches!( + cursor_context, + CursorContext::AfterComparisonOp(_, _) | CursorContext::InStringLiteral { .. } + ); // Method suggestions arrive pre-filtered against the partial method // name; everything else is filtered here. Suggestions may be quoted @@ -339,12 +386,16 @@ impl CursorAwareParser { // `price.Contains('')`); everything else replaces the whole identifier. // After a quoted column the dot terminates the token, so the partial // method is already the whole token (`"name.common".Star` -> `Star`). - let replace_start = if is_method_context { - token.as_ref().map_or(cursor_pos, |t| { + let replace_start = match &cursor_context { + // Inside a literal the span to replace is the value text, which is + // not an identifier at all — `find_completion_token` returns + // nothing useful there. The analyzer knows where the quote is, so + // it says so directly. + CursorContext::InStringLiteral { value_start, .. } => *value_start, + _ if is_method_context => token.as_ref().map_or(cursor_pos, |t| { t.last_segment().map_or(t.start, |(start, _)| start) - }) - } else { - token.as_ref().map_or(cursor_pos, |t| t.start) + }), + _ => token.as_ref().map_or(cursor_pos, |t| t.start), }; ParseResult { @@ -355,6 +406,16 @@ impl CursorAwareParser { } } + /// The loaded table's columns, quoted where the lexer would not read them + /// back as one identifier (T8). + fn column_suggestions(&self, table: &str) -> Vec { + self.schema + .get_columns(table) + .into_iter() + .map(|col| quote_if_needed(&col)) + .collect() + } + /// Suggest real column names when the text at the cursor prefixes one. /// /// Only dotted text reaches here: undotted prefixes are already handled by @@ -1229,11 +1290,16 @@ mod tests { r#"select City,Company,Country,"Customer Id" from customers order by City, "Customer"#; let result = parser.get_completions(query, query.len()); - // The partial word should be "Customer + // The partial is the identifier's *text*. Before T9 it came from a + // string scanner and arrived as `"Customer`, opening quote included; + // it now comes from the lexer, which has already consumed the quote. + // Nothing filters on `partial_word` — that is `find_completion_token`'s + // job, and it still sees the quote — so the two asserts below are the + // ones with teeth. assert_eq!( result.partial_word, - Some("\"Customer".to_string()), - "Should extract '\"Customer' as partial" + Some("Customer".to_string()), + "Should extract 'Customer' as partial" ); // Should suggest "Customer Id" with proper quotes diff --git a/src/sql/cursor_context.rs b/src/sql/cursor_context.rs new file mode 100644 index 00000000..300d179a --- /dev/null +++ b/src/sql/cursor_context.rs @@ -0,0 +1,584 @@ +//! Where the cursor is, decided from the token stream (T9). +//! +//! Completion has three questions to answer: *what kind of thing goes here*, +//! *what has the user typed so far*, and *which span does an accepted +//! suggestion replace*. T1 gave the third one a single owner +//! ([`crate::sql::completion_token`]). T2 gave the first one a schema. This +//! module is the first question's owner. +//! +//! # Why it exists +//! +//! Before T9 the answer came from two near-identical string scanners — +//! `analyze_statement` for a query that parsed and `analyze_partial` for one +//! that did not — which between them used `rfind('.')`, +//! `split_whitespace().last()`, `ends_with(" AND")` and +//! `query_upper.contains("SELECT")`. `analyze_partial` even built a token +//! stream first and then ignored it. The consequences were not subtle: +//! +//! - `WHERE region = '` collapsed to `WhereClause` and offered every **column +//! name** from inside the string literal, because nothing looked at +//! [`Token::StringLiteral`]. +//! - `WHERE "name.common" = ` found no comparison operator at all, because the +//! column had to satisfy `chars().all(|c| c.is_alphanumeric() || c == '_')` +//! — which excludes every column that needs quoting. +//! - `WHERE region='x'` found none either, because the operator list was +//! `[" > ", " < ", " = ", …]` and required the spaces. +//! - `GROUP BY ` and `LIMIT ` fell through to a fallback that suggested +//! `WHERE`/`ORDER BY` and a table name respectively. The completer knew six +//! keywords; [`Token::from_keyword`] knows fifty-five. +//! +//! # How it works +//! +//! The text is truncated at the cursor and tokenized once. Context is then a +//! match on the **tail** of the token stream, not a series of backward scans: +//! the last token says what kind of position this is, and the token before it +//! disambiguates a bare word. Keywords are [`Token`] variants, so a keyword the +//! lexer learns the completer learns with it. +//! +//! Spans are byte offsets throughout — see +//! [`Lexer::tokenize_all_with_byte_positions`]. + +use crate::sql::parser::ast::LogicalOp; +use crate::sql::parser::lexer::{Lexer, Token}; + +/// What the cursor is sitting in. +/// +/// Produced only by [`analyze`]; consumed by +/// [`crate::sql::cursor_aware_parser::CursorAwareParser`]. +#[derive(Debug, Clone, PartialEq)] +pub enum CursorContext { + SelectClause, + FromClause, + /// A table name is complete; the next thing is a clause keyword. + AfterTable, + WhereClause, + GroupByClause, + HavingClause, + OrderByClause, + /// A row count is expected — nothing in the schema can help. + LimitClause, + AfterColumn(String), + AfterLogicalOp(LogicalOp), + /// column name, operator text + AfterComparisonOp(String, String), + /// The cursor is inside an unterminated string literal. `column` is what + /// the literal is being compared against, and is `None` when it is + /// attached to nothing recognisable — a literal is still not a place for + /// column names either way. `in_list` distinguishes `IN ('a', '')` + /// from `= ''`; `value_start` is the byte offset just past the + /// opening quote, which is the span an accepted value replaces. + /// + /// T4 fills this with the column's distinct values. Until then the + /// completer deliberately offers nothing here — which is still an + /// improvement on offering all 100 column names inside a string. + InStringLiteral { + column: Option, + in_list: bool, + value_start: usize, + }, + /// object, method + InMethodCall(String, String), + InExpression, + Unknown, +} + +/// Decide what the cursor is sitting in, and what has been typed so far. +/// +/// `cursor_pos` is a **byte** offset. Everything after it is ignored: context +/// is a question about what precedes the cursor, and truncating first is what +/// lets the tail-of-stream match below be so short. +#[must_use] +pub fn analyze(query: &str, cursor_pos: usize) -> (CursorContext, Option) { + let text = safe_slice_to(query, cursor_pos); + let tokens = Lexer::new(text).tokenize_all_with_byte_positions(); + + if tokens.is_empty() { + return (CursorContext::Unknown, None); + } + + let last = tokens.len() - 1; + let (start, stop, token) = (tokens[last].0, tokens[last].1, &tokens[last].2); + + // The cursor is inside an unterminated literal. Checked first, because the + // text inside quotes may be anything at all — keywords, dots, operators — + // and none of it means what it would mean outside. + if let Token::StringLiteral(value) = token { + if !literal_is_terminated(&text[start..stop]) { + let (column, in_list) = + value_target(&tokens, last).map_or((None, false), |(c, l)| (Some(c), l)); + return ( + CursorContext::InStringLiteral { + column, + in_list, + value_start: start + 1, + }, + non_empty(value), + ); + } + } + + // Positions *after* a complete token. Trailing whitespace is irrelevant + // here — `region = ` and `region =` are the same position. + if matches!(token, Token::Dot) { + if let Some(column) = column_chain_ending_at(&tokens, last.checked_sub(1)) { + return (CursorContext::AfterColumn(column), None); + } + } + if let Some(op) = logical_op(token) { + return (CursorContext::AfterLogicalOp(op), None); + } + if let Some(op) = comparison_op(token) { + if let Some(column) = column_chain_ending_at(&tokens, last.checked_sub(1)) { + return ( + CursorContext::AfterComparisonOp(column, op.to_string()), + None, + ); + } + } + + // A token that reaches the cursor with no whitespace between is the word + // being typed; one that stops short of it is finished. Only here does the + // distinction matter, and what the word is a partial *of* is decided by + // the token before it. + if stop == text.len() { + let partial = match token { + // An unterminated quoted identifier (`WHERE "na`) is a column being + // typed, not a value. The clause decides what to offer; which span + // it replaces is `find_completion_token`'s business, not ours. + Token::Identifier(word) + | Token::NumberLiteral(word) + | Token::QuotedIdentifier(word) => non_empty(word), + _ => None, + }; + + if partial.is_some() && !matches!(token, Token::QuotedIdentifier(_)) { + if let Some(prev) = last.checked_sub(1) { + let before = &tokens[prev].2; + if matches!(before, Token::Dot) { + if let Some(column) = column_chain_ending_at(&tokens, prev.checked_sub(1)) { + return (CursorContext::AfterColumn(column), partial); + } + } + if let Some(op) = logical_op(before) { + return (CursorContext::AfterLogicalOp(op), partial); + } + if let Some(op) = comparison_op(before) { + if let Some(column) = column_chain_ending_at(&tokens, prev.checked_sub(1)) { + return ( + CursorContext::AfterComparisonOp(column, op.to_string()), + partial, + ); + } + } + } + } + + return (clause_context(&tokens), partial); + } + + (clause_context(&tokens), None) +} + +fn logical_op(token: &Token) -> Option { + match token { + Token::And => Some(LogicalOp::And), + Token::Or => Some(LogicalOp::Or), + _ => None, + } +} + +/// The clause the cursor is in, from the last clause-introducing keyword. +/// +/// Truncation at the cursor is what makes "last" the right one to take. +fn clause_context(tokens: &[(usize, usize, Token)]) -> CursorContext { + let Some(idx) = tokens.iter().rposition(|(_, _, t)| is_clause_keyword(t)) else { + return CursorContext::Unknown; + }; + + match &tokens[idx].2 { + Token::Select => CursorContext::SelectClause, + Token::From => { + // `FROM ` wants a table; `FROM countries ` wants a clause keyword. + if tokens[idx + 1..] + .iter() + .any(|(_, _, t)| matches!(t, Token::Identifier(_) | Token::QuotedIdentifier(_))) + { + CursorContext::AfterTable + } else { + CursorContext::FromClause + } + } + // ON and QUALIFY are predicate positions: the same columns apply. + Token::Where | Token::On | Token::Qualify => CursorContext::WhereClause, + Token::GroupBy => CursorContext::GroupByClause, + Token::Having => CursorContext::HavingClause, + Token::OrderBy => CursorContext::OrderByClause, + Token::Limit | Token::Offset => CursorContext::LimitClause, + _ => CursorContext::Unknown, + } +} + +fn is_clause_keyword(token: &Token) -> bool { + matches!( + token, + Token::Select + | Token::From + | Token::Where + | Token::GroupBy + | Token::Having + | Token::OrderBy + | Token::Qualify + | Token::Limit + | Token::Offset + | Token::On + ) +} + +/// Which column an unterminated literal is being compared against, and whether +/// it sits in an `IN (...)` list. +/// +/// Walks back over any values already in the list, so +/// `region IN ('Asia', 'Eu` resolves to `region` just as `region = 'Eu` does. +fn value_target(tokens: &[(usize, usize, Token)], literal: usize) -> Option<(String, bool)> { + let mut idx = literal.checked_sub(1)?; + let mut in_list = false; + + loop { + match &tokens[idx].2 { + // Earlier entries in the same list. + Token::Comma | Token::StringLiteral(_) | Token::NumberLiteral(_) => {} + Token::LeftParen => { + in_list = true; + idx = idx.checked_sub(1)?; + // `IN (` and `NOT IN (` — step past the keywords to the column. + if matches!(tokens[idx].2, Token::In) { + idx = idx.checked_sub(1)?; + if matches!(tokens[idx].2, Token::Not) { + idx = idx.checked_sub(1)?; + } + } + break; + } + token if comparison_op(token).is_some() => { + idx = idx.checked_sub(1)?; + break; + } + _ => return None, + } + idx = idx.checked_sub(1)?; + } + + column_chain_ending_at(tokens, Some(idx)).map(|column| (column, in_list)) +} + +/// The column name ending at `idx`, following `Dot` links backwards. +/// +/// `translations.deu.common` is one column in this codebase, so the chain +/// matters; taking only the last segment is how dotted columns became +/// unreachable in the first place (T1). Quotes are stripped, because the +/// schema is keyed on the bare name. +fn column_chain_ending_at(tokens: &[(usize, usize, Token)], idx: Option) -> Option { + let mut idx = idx?; + let mut segments = vec![identifier_text(&tokens[idx].2)?.to_string()]; + + while idx >= 2 && matches!(tokens[idx - 1].2, Token::Dot) { + let Some(text) = identifier_text(&tokens[idx - 2].2) else { + break; + }; + segments.push(text.to_string()); + idx -= 2; + } + + segments.reverse(); + Some(segments.join(".")) +} + +fn identifier_text(token: &Token) -> Option<&str> { + match token { + Token::Identifier(s) | Token::QuotedIdentifier(s) => Some(s), + _ => None, + } +} + +/// The operator's text, or `None` if the token is not a comparison. +/// +/// Replaces a `[" > ", " < ", " = ", …]` scan that required the surrounding +/// spaces, so `region='x'` never matched. +fn comparison_op(token: &Token) -> Option<&'static str> { + match token { + Token::Equal => Some("="), + Token::NotEqual => Some("!="), + Token::LessThan => Some("<"), + Token::GreaterThan => Some(">"), + Token::LessThanOrEqual => Some("<="), + Token::GreaterThanOrEqual => Some(">="), + Token::Like => Some("LIKE"), + Token::ILike => Some("ILIKE"), + _ => None, + } +} + +/// Whether a literal's source span carries its closing quote. +/// +/// The lexer runs an unterminated string to end-of-input and returns its +/// contents with no complaint, which is exactly what completion wants — but it +/// means the token alone cannot say whether the user has closed the quote. +/// Mirrors `read_string`, doubled quotes included, so `'O''` reads as still +/// open. +fn literal_is_terminated(span: &str) -> bool { + let mut chars = span.chars(); + let Some(quote) = chars.next() else { + return false; + }; + let mut chars = chars.peekable(); + while let Some(ch) = chars.next() { + if ch == quote { + if chars.peek() == Some("e) { + chars.next(); + } else { + return true; + } + } + } + false +} + +fn non_empty(word: &str) -> Option { + (!word.is_empty()).then(|| word.to_string()) +} + +/// Truncate at `pos`, backing up to a character boundary rather than panicking. +fn safe_slice_to(s: &str, pos: usize) -> &str { + if pos >= s.len() { + return s; + } + let mut safe = pos; + while safe > 0 && !s.is_char_boundary(safe) { + safe -= 1; + } + &s[..safe] +} + +#[cfg(test)] +mod tests { + use super::*; + + fn ctx(query: &str) -> CursorContext { + analyze(query, query.len()).0 + } + + fn partial(query: &str) -> Option { + analyze(query, query.len()).1 + } + + // ---- the cases T9 exists to fix ---- + + #[test] + fn cursor_inside_a_literal_is_a_value_position() { + assert_eq!( + ctx("SELECT * FROM t WHERE region = '"), + CursorContext::InStringLiteral { + column: Some("region".to_string()), + in_list: false, + value_start: 32, + } + ); + } + + #[test] + fn a_partial_value_is_reported_as_the_partial() { + let (context, partial) = analyze("SELECT * FROM t WHERE region = 'Am", 34); + assert!(matches!(context, CursorContext::InStringLiteral { .. })); + assert_eq!(partial.as_deref(), Some("Am")); + } + + #[test] + fn in_list_is_distinguished_and_walks_past_existing_values() { + assert_eq!( + ctx("SELECT * FROM t WHERE region IN ('Asia', 'Eu"), + CursorContext::InStringLiteral { + column: Some("region".to_string()), + in_list: true, + value_start: 42, + } + ); + } + + #[test] + fn not_in_still_resolves_to_the_column() { + // The negation sits between the column and the keyword, so the walk + // back has to step over it or the list loses its column. + assert_eq!( + ctx("SELECT * FROM t WHERE region NOT IN ('"), + CursorContext::InStringLiteral { + column: Some("region".to_string()), + in_list: true, + value_start: 38, + } + ); + } + + #[test] + fn a_closed_literal_is_not_a_value_position() { + // The quote is closed, so the cursor is after a complete value. + assert_eq!(ctx("SELECT * FROM t WHERE region = 'Asia'"), { + CursorContext::WhereClause + }); + } + + #[test] + fn a_doubled_quote_leaves_the_literal_open() { + // `'O''` is an unterminated literal containing one quote, not a + // finished one — the same rule `read_string` applies. + assert!(matches!( + ctx("SELECT * FROM t WHERE name = 'O''"), + CursorContext::InStringLiteral { .. } + )); + } + + #[test] + fn keywords_inside_a_literal_do_not_move_the_cursor() { + assert!(matches!( + ctx("SELECT * FROM t WHERE region = 'GROUP BY FROM "), + CursorContext::InStringLiteral { .. } + )); + } + + #[test] + fn a_quoted_column_reaches_the_comparison_operator() { + // Was `WhereClause`: the old check required + // `chars().all(alphanumeric || '_')`, which no quoted column passes. + assert_eq!( + ctx(r#"SELECT * FROM t WHERE "name.common" = "#), + CursorContext::AfterComparisonOp("name.common".to_string(), "=".to_string()) + ); + } + + #[test] + fn an_operator_without_spaces_is_still_an_operator() { + // Was missed entirely: the old list was `[" = ", " > ", …]`. + assert_eq!( + ctx("SELECT * FROM t WHERE region="), + CursorContext::AfterComparisonOp("region".to_string(), "=".to_string()) + ); + } + + #[test] + fn clause_keywords_the_old_scanner_did_not_know() { + assert_eq!( + ctx("SELECT * FROM t GROUP BY "), + CursorContext::GroupByClause + ); + assert_eq!( + ctx("SELECT * FROM t GROUP BY a HAVING "), + CursorContext::HavingClause + ); + // Was `FromClause`, which suggested a table name where a row count goes. + assert_eq!(ctx("SELECT * FROM t LIMIT "), CursorContext::LimitClause); + assert_eq!(ctx("SELECT * FROM t QUALIFY "), CursorContext::WhereClause); + } + + #[test] + fn a_named_table_moves_past_the_from_clause() { + assert_eq!(ctx("SELECT * FROM "), CursorContext::FromClause); + assert_eq!(ctx("SELECT * FROM countries "), CursorContext::AfterTable); + } + + // ---- behaviour that must not regress ---- + + #[test] + fn clauses_still_resolve() { + assert_eq!(ctx("SELECT "), CursorContext::SelectClause); + assert_eq!(ctx("SELECT * FROM t WHERE "), CursorContext::WhereClause); + assert_eq!( + ctx("SELECT * FROM t ORDER BY "), + CursorContext::OrderByClause + ); + } + + #[test] + fn logical_operators_still_resolve() { + assert_eq!( + ctx("SELECT * FROM t WHERE a = 1 AND "), + CursorContext::AfterLogicalOp(LogicalOp::And) + ); + assert_eq!( + ctx("SELECT * FROM t WHERE a = 1 OR reg"), + CursorContext::AfterLogicalOp(LogicalOp::Or) + ); + assert_eq!( + partial("SELECT * FROM t WHERE a = 1 OR reg").as_deref(), + Some("reg") + ); + } + + #[test] + fn a_dot_is_a_method_position_on_the_whole_dotted_chain() { + assert_eq!( + ctx("SELECT * FROM t WHERE name."), + CursorContext::AfterColumn("name".to_string()) + ); + assert_eq!( + ctx("SELECT translations.deu."), + CursorContext::AfterColumn("translations.deu".to_string()) + ); + assert_eq!( + ctx("SELECT * FROM t WHERE name.Con"), + CursorContext::AfterColumn("name".to_string()) + ); + assert_eq!( + partial("SELECT * FROM t WHERE name.Con").as_deref(), + Some("Con") + ); + } + + #[test] + fn an_open_method_call_is_not_a_method_position() { + // `price.Contains(` — the method is already chosen. + assert!(!matches!( + ctx("SELECT * FROM t WHERE price.Contains("), + CursorContext::AfterColumn(_) + )); + } + + #[test] + fn context_ignores_everything_after_the_cursor() { + // Cursor in the SELECT list of a query that goes on to have a WHERE. + let (context, _) = analyze("SELECT FROM t WHERE region = 'Asia'", 7); + assert_eq!(context, CursorContext::SelectClause); + } + + #[test] + fn trailing_whitespace_means_no_partial() { + assert_eq!(partial("SELECT * FROM t WHERE region "), None); + assert_eq!(partial("SELECT * FROM t WHERE reg").as_deref(), Some("reg")); + } + + // ---- the char/byte trap ---- + + #[test] + fn spans_are_byte_offsets_not_char_indices() { + // `ö` is two bytes. A char index would land short of the real + // offset and the spliced value would come out mangled. + let query = "SELECT * FROM t WHERE city = 'Malmö' AND name = '"; + let CursorContext::InStringLiteral { value_start, .. } = ctx(query) else { + panic!("expected a value position"); + }; + assert_eq!(value_start, query.len()); + assert!(query.is_char_boundary(value_start)); + } + + #[test] + fn a_cursor_inside_a_multibyte_character_does_not_panic() { + let query = "SELECT * FROM t WHERE city = 'Malmö'"; + // Byte 35 is the second byte of `ö`. + assert!(!query.is_char_boundary(35)); + let _ = analyze(query, 35); + } + + #[test] + fn empty_input_is_unknown() { + assert_eq!(ctx(""), CursorContext::Unknown); + assert_eq!(ctx(" "), CursorContext::Unknown); + } +} diff --git a/src/sql/hybrid_parser.rs b/src/sql/hybrid_parser.rs index f7fa6c6a..d83d8f94 100644 --- a/src/sql/hybrid_parser.rs +++ b/src/sql/hybrid_parser.rs @@ -66,8 +66,19 @@ impl HybridParser { let recursive_context = match cursor_context { CursorContext::SelectClause => "SelectClause", CursorContext::FromClause => "FromClause", + CursorContext::AfterTable => "AfterTable", CursorContext::WhereClause => "WhereClause", + CursorContext::GroupByClause => "GroupByClause", + CursorContext::HavingClause => "HavingClause", CursorContext::OrderByClause => "OrderByClause", + CursorContext::LimitClause => "LimitClause", + CursorContext::InStringLiteral { in_list, .. } => { + if in_list { + "InStringLiteral(list)" + } else { + "InStringLiteral" + } + } CursorContext::AfterColumn(_) => "AfterColumn", CursorContext::AfterLogicalOp(LogicalOp::And) => "AfterAND", CursorContext::AfterLogicalOp(LogicalOp::Or) => "AfterOR", diff --git a/src/sql/mod.rs b/src/sql/mod.rs index d38d3161..151517c7 100644 --- a/src/sql/mod.rs +++ b/src/sql/mod.rs @@ -8,6 +8,7 @@ pub mod aggregates; pub mod cache; pub mod completion_token; pub mod cursor_aware_parser; +pub mod cursor_context; pub mod functions; pub mod generators; pub mod hybrid_parser; diff --git a/src/sql/parser/ast.rs b/src/sql/parser/ast.rs index 5b9107bc..67ad28ab 100644 --- a/src/sql/parser/ast.rs +++ b/src/sql/parser/ast.rs @@ -266,7 +266,7 @@ pub struct Condition { pub connector: Option, // AND/OR connecting to next condition } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq)] pub enum LogicalOp { And, Or, diff --git a/src/sql/parser/lexer.rs b/src/sql/parser/lexer.rs index 32ebf775..5b1f7817 100644 --- a/src/sql/parser/lexer.rs +++ b/src/sql/parser/lexer.rs @@ -991,6 +991,32 @@ impl Lexer { tokens } + /// Same as [`Lexer::tokenize_all_with_positions`], but the spans are + /// **byte** offsets into the original input rather than indices into the + /// internal `Vec`. + /// + /// The two agree only while the input is ASCII. Completion compares these + /// spans against a byte `cursor_pos` and splices replacement text at them, + /// so a char index there silently corrupts any query containing a + /// non-ASCII character — a column name, or a value being filtered on. T9 + /// makes the completer token-driven, so it converts once, here, rather + /// than at each use. + pub fn tokenize_all_with_byte_positions(&mut self) -> Vec<(usize, usize, Token)> { + let mut byte_at: Vec = Vec::with_capacity(self.input.len() + 1); + let mut acc = 0usize; + for ch in &self.input { + byte_at.push(acc); + acc += ch.len_utf8(); + } + byte_at.push(acc); + let last = byte_at.len() - 1; + + self.tokenize_all_with_positions() + .into_iter() + .map(|(start, end, token)| (byte_at[start.min(last)], byte_at[end.min(last)], token)) + .collect() + } + /// Tokenize all tokens including comments /// This is useful for formatting tools that need to preserve comments pub fn tokenize_all_with_comments(&mut self) -> Vec { diff --git a/src/sql/recursive_parser.rs b/src/sql/recursive_parser.rs index ec2418bb..21631d29 100644 --- a/src/sql/recursive_parser.rs +++ b/src/sql/recursive_parser.rs @@ -205,8 +205,6 @@ impl Parser { } /// Get comparison operator string representation (for autocomplete context) - const COMPARISON_OPERATORS: [&'static str; 6] = [" > ", " < ", " >= ", " <= ", " = ", " != "]; - pub fn consume(&mut self, expected: Token) -> Result<(), String> { self.trace_token(&format!("Consuming expected {:?}", expected)); if std::mem::discriminant(&self.current_token) == std::mem::discriminant(&expected) { @@ -539,17 +537,6 @@ impl Parser { } } - /// Helper function to check if an identifier is valid (quoted or regular) - fn is_valid_identifier(name: &str) -> bool { - if name.starts_with('"') && name.ends_with('"') { - // Quoted identifier - always valid - true - } else { - // Regular identifier - check if it's alphanumeric or underscore - name.chars().all(|c| c.is_alphanumeric() || c == '_') - } - } - /// Helper function to update parentheses depth tracking fn update_paren_depth(&mut self, token: &Token) -> Result<(), String> { match token { @@ -2255,69 +2242,21 @@ impl Parser { } } -// Context detection for cursor position -#[derive(Debug, Clone)] -pub enum CursorContext { - SelectClause, - FromClause, - WhereClause, - OrderByClause, - AfterColumn(String), - AfterLogicalOp(LogicalOp), - AfterComparisonOp(String, String), // column_name, operator - InMethodCall(String, String), // object, method - InExpression, - Unknown, -} - -/// Safe UTF-8 string slicing that ensures we don't slice in the middle of a character -fn safe_slice_to(s: &str, pos: usize) -> &str { - if pos >= s.len() { - return s; - } - - // Find the nearest valid character boundary at or before pos - let mut safe_pos = pos; - while safe_pos > 0 && !s.is_char_boundary(safe_pos) { - safe_pos -= 1; - } - - &s[..safe_pos] -} - -/// Safe UTF-8 string slicing from a position to the end -fn safe_slice_from(s: &str, pos: usize) -> &str { - if pos >= s.len() { - return ""; - } - - // Find the nearest valid character boundary at or after pos - let mut safe_pos = pos; - while safe_pos < s.len() && !s.is_char_boundary(safe_pos) { - safe_pos += 1; - } - - &s[safe_pos..] -} - +// Cursor context now lives with the token-driven analyzer that produces it +// (T9). Re-exported so `use recursive_parser::CursorContext` keeps working. +pub use crate::sql::cursor_context::CursorContext; + +/// Where the cursor is, for completion. +/// +/// Delegates to [`crate::sql::cursor_context::analyze`]. Before T9 this +/// branched on whether the partial query happened to parse and ran one of two +/// near-identical string scanners; the AST it went to the trouble of building +/// was consulted only for `stmt.where_clause.is_some()`-shaped questions the +/// token stream answers directly. The parse went with them, so this is also +/// one less full parse of the query per keystroke. #[must_use] pub fn detect_cursor_context(query: &str, cursor_pos: usize) -> (CursorContext, Option) { - let truncated = safe_slice_to(query, cursor_pos); - let mut parser = Parser::new(truncated); - - // Try to parse as much as possible - if let Ok(stmt) = parser.parse() { - let (ctx, partial) = analyze_statement(&stmt, truncated, cursor_pos); - #[cfg(test)] - println!("analyze_statement returned: {ctx:?}, {partial:?} for query: '{truncated}'"); - (ctx, partial) - } else { - // Partial parse - analyze what we have - let (ctx, partial) = analyze_partial(truncated, cursor_pos); - #[cfg(test)] - println!("analyze_partial returned: {ctx:?}, {partial:?} for query: '{truncated}'"); - (ctx, partial) - } + crate::sql::cursor_context::analyze(query, cursor_pos) } #[must_use] @@ -2327,228 +2266,6 @@ pub fn tokenize_query(query: &str) -> Vec { tokens.iter().map(|t| format!("{t:?}")).collect() } -#[must_use] -/// Helper function to find the start of a quoted string searching backwards -fn find_quote_start(bytes: &[u8], mut pos: usize) -> Option { - // Skip the closing quote and search backwards - if pos > 0 { - pos -= 1; - while pos > 0 { - if bytes[pos] == b'"' { - // Check if it's not an escaped quote - if pos == 0 || bytes[pos - 1] != b'\\' { - return Some(pos); - } - } - pos -= 1; - } - // Check position 0 separately - if bytes[0] == b'"' { - return Some(0); - } - } - None -} - -/// Helper function to handle method call context after validation -fn handle_method_call_context(col_name: &str, after_dot: &str) -> (CursorContext, Option) { - // Check if there's a partial method name after the dot - let partial_method = if after_dot.is_empty() { - None - } else if after_dot.chars().all(|c| c.is_alphanumeric() || c == '_') { - Some(after_dot.to_string()) - } else { - None - }; - - // For AfterColumn context, strip quotes if present for consistency - let col_name_for_context = - if col_name.starts_with('"') && col_name.ends_with('"') && col_name.len() > 2 { - col_name[1..col_name.len() - 1].to_string() - } else { - col_name.to_string() - }; - - ( - CursorContext::AfterColumn(col_name_for_context), - partial_method, - ) -} - -/// Helper function to check if we're after a comparison operator -fn check_after_comparison_operator(query: &str) -> Option<(CursorContext, Option)> { - for op in &Parser::COMPARISON_OPERATORS { - if let Some(op_pos) = query.rfind(op) { - let before_op = safe_slice_to(query, op_pos); - let after_op_start = op_pos + op.len(); - let after_op = if after_op_start < query.len() { - &query[after_op_start..] - } else { - "" - }; - - // Check if we have a column name before the operator - if let Some(col_name) = before_op.split_whitespace().last() { - if col_name.chars().all(|c| c.is_alphanumeric() || c == '_') { - // Check if we're at or near the end of the query - let after_op_trimmed = after_op.trim(); - if after_op_trimmed.is_empty() - || (after_op_trimmed - .chars() - .all(|c| c.is_alphanumeric() || c == '_') - && !after_op_trimmed.contains('(')) - { - let partial = if after_op_trimmed.is_empty() { - None - } else { - Some(after_op_trimmed.to_string()) - }; - return Some(( - CursorContext::AfterComparisonOp( - col_name.to_string(), - op.trim().to_string(), - ), - partial, - )); - } - } - } - } - } - None -} - -fn analyze_statement( - stmt: &SelectStatement, - query: &str, - _cursor_pos: usize, -) -> (CursorContext, Option) { - // First check for method call context (e.g., "columnName." or "columnName.Con") - let trimmed = query.trim(); - - // Check if we're after a comparison operator (e.g., "createdDate > ") - if let Some(result) = check_after_comparison_operator(query) { - return result; - } - - // First check if we're after AND/OR - this takes precedence - // Helper function to check if string ends with a logical operator - let ends_with_logical_op = |s: &str| -> bool { - let s_upper = s.to_uppercase(); - s_upper.ends_with(" AND") || s_upper.ends_with(" OR") - }; - - if ends_with_logical_op(trimmed) { - // Don't check for method context if we're clearly after a logical operator - } else { - // Look for the last dot in the query - if let Some(dot_pos) = trimmed.rfind('.') { - // Check if we're after a column name and dot - let before_dot = safe_slice_to(trimmed, dot_pos); - let after_dot_start = dot_pos + 1; - let after_dot = if after_dot_start < trimmed.len() { - &trimmed[after_dot_start..] - } else { - "" - }; - - // Check if the part after dot looks like an incomplete method call - // (not a complete method call like "Contains(...)") - if !after_dot.contains('(') { - // Try to extract the column name - could be quoted or regular - let col_name = if before_dot.ends_with('"') { - // Handle quoted identifier - search backwards for matching opening quote - let bytes = before_dot.as_bytes(); - let pos = before_dot.len() - 1; // Position of closing quote - - find_quote_start(bytes, pos).map(|start| safe_slice_from(before_dot, start)) - } else { - // Regular identifier - get the last word, handling parentheses - // Strip all leading parentheses - before_dot - .split_whitespace() - .last() - .map(|word| word.trim_start_matches('(')) - }; - - if let Some(col_name) = col_name { - // For quoted identifiers, keep the quotes, for regular identifiers check validity - let is_valid = Parser::is_valid_identifier(col_name); - - if is_valid { - return handle_method_call_context(col_name, after_dot); - } - } - } - } - } - - // Check if we're in WHERE clause - if let Some(where_clause) = &stmt.where_clause { - // Check if query ends with AND/OR (with or without trailing space/partial) - let trimmed_upper = trimmed.to_uppercase(); - if trimmed_upper.ends_with(" AND") || trimmed_upper.ends_with(" OR") { - let op = if trimmed_upper.ends_with(" AND") { - LogicalOp::And - } else { - LogicalOp::Or - }; - return (CursorContext::AfterLogicalOp(op), None); - } - - // Check if we have AND/OR followed by a partial word - let query_upper = query.to_uppercase(); - if let Some(and_pos) = query_upper.rfind(" AND ") { - let after_and = safe_slice_from(query, and_pos + 5); - let partial = extract_partial_at_end(after_and); - if partial.is_some() { - return (CursorContext::AfterLogicalOp(LogicalOp::And), partial); - } - } - - if let Some(or_pos) = query_upper.rfind(" OR ") { - let after_or = safe_slice_from(query, or_pos + 4); - let partial = extract_partial_at_end(after_or); - if partial.is_some() { - return (CursorContext::AfterLogicalOp(LogicalOp::Or), partial); - } - } - - if let Some(last_condition) = where_clause.conditions.last() { - if let Some(connector) = &last_condition.connector { - // We're after AND/OR - return ( - CursorContext::AfterLogicalOp(connector.clone()), - extract_partial_at_end(query), - ); - } - } - // We're in WHERE clause but not after AND/OR - return (CursorContext::WhereClause, extract_partial_at_end(query)); - } - - // Check if we're after ORDER BY - let query_upper = query.to_uppercase(); - if query_upper.ends_with(" ORDER BY") { - return (CursorContext::OrderByClause, None); - } - - // Check other contexts based on what's in the statement - if stmt.order_by.is_some() { - return (CursorContext::OrderByClause, extract_partial_at_end(query)); - } - - if stmt.from_table.is_some() && stmt.where_clause.is_none() && stmt.order_by.is_none() { - return (CursorContext::FromClause, extract_partial_at_end(query)); - } - - if !stmt.columns.is_empty() && stmt.from_table.is_none() { - return (CursorContext::SelectClause, extract_partial_at_end(query)); - } - - (CursorContext::Unknown, None) -} - /// Render a token for an error message: the literal text where we have it, so the /// user sees what they typed rather than an internal variant name. fn describe_token(token: &Token) -> String { @@ -2570,254 +2287,6 @@ fn describe_token(token: &Token) -> String { } } -/// Helper function to find the last occurrence of a token type in the token stream -fn find_last_token(tokens: &[(usize, usize, Token)], target: &Token) -> Option { - tokens - .iter() - .rposition(|(_, _, t)| t == target) - .map(|idx| tokens[idx].0) -} - -/// Helper function to find the last occurrence of any matching token -fn find_last_matching_token( - tokens: &[(usize, usize, Token)], - predicate: F, -) -> Option<(usize, &Token)> -where - F: Fn(&Token) -> bool, -{ - tokens - .iter() - .rposition(|(_, _, t)| predicate(t)) - .map(|idx| (tokens[idx].0, &tokens[idx].2)) -} - -/// Helper function to check if we're in a specific clause based on tokens -fn is_in_clause( - tokens: &[(usize, usize, Token)], - clause_token: Token, - exclude_tokens: &[Token], -) -> bool { - // Find the last occurrence of the clause token - if let Some(clause_pos) = find_last_token(tokens, &clause_token) { - // Check if any exclude tokens appear after it - for (pos, _, token) in tokens.iter() { - if *pos > clause_pos && exclude_tokens.contains(token) { - return false; - } - } - return true; - } - false -} - -fn analyze_partial(query: &str, cursor_pos: usize) -> (CursorContext, Option) { - // Tokenize the query up to cursor position - let mut lexer = Lexer::new(query); - let tokens = lexer.tokenize_all_with_positions(); - - let trimmed = query.trim(); - - #[cfg(test)] - { - if trimmed.contains("\"Last Name\"") { - eprintln!("DEBUG analyze_partial: query='{query}', trimmed='{trimmed}'"); - } - } - - // Check if we're after a comparison operator (e.g., "createdDate > ") - if let Some(result) = check_after_comparison_operator(query) { - return result; - } - - // Look for the last dot in the query (method call context) - check this FIRST - // before AND/OR detection to properly handle cases like "AND (Country." - if let Some(dot_pos) = trimmed.rfind('.') { - #[cfg(test)] - { - if trimmed.contains("\"Last Name\"") { - eprintln!("DEBUG: Found dot at position {dot_pos}"); - } - } - // Check if we're after a column name and dot - let before_dot = &trimmed[..dot_pos]; - let after_dot = &trimmed[dot_pos + 1..]; - - // Check if the part after dot looks like an incomplete method call - // (not a complete method call like "Contains(...)") - if !after_dot.contains('(') { - // Try to extract the column name before the dot - // It could be a quoted identifier like "Last Name" or a regular identifier - let col_name = if before_dot.ends_with('"') { - // Handle quoted identifier - search backwards for matching opening quote - let bytes = before_dot.as_bytes(); - let pos = before_dot.len() - 1; // Position of closing quote - - #[cfg(test)] - { - if trimmed.contains("\"Last Name\"") { - eprintln!("DEBUG: before_dot='{before_dot}', looking for opening quote"); - } - } - - let found_start = find_quote_start(bytes, pos); - - if let Some(start) = found_start { - // Extract the full quoted identifier including quotes - let result = safe_slice_from(before_dot, start); - #[cfg(test)] - { - if trimmed.contains("\"Last Name\"") { - eprintln!("DEBUG: Extracted quoted identifier: '{result}'"); - } - } - Some(result) - } else { - #[cfg(test)] - { - if trimmed.contains("\"Last Name\"") { - eprintln!("DEBUG: No opening quote found!"); - } - } - None - } - } else { - // Regular identifier - get the last word, handling parentheses - // Strip all leading parentheses - before_dot - .split_whitespace() - .last() - .map(|word| word.trim_start_matches('(')) - }; - - if let Some(col_name) = col_name { - #[cfg(test)] - { - if trimmed.contains("\"Last Name\"") { - eprintln!("DEBUG: col_name = '{col_name}'"); - } - } - - // For quoted identifiers, keep the quotes, for regular identifiers check validity - let is_valid = Parser::is_valid_identifier(col_name); - - #[cfg(test)] - { - if trimmed.contains("\"Last Name\"") { - eprintln!("DEBUG: is_valid = {is_valid}"); - } - } - - if is_valid { - return handle_method_call_context(col_name, after_dot); - } - } - } - } - - // Check if we're after AND/OR using tokens - but only after checking for method calls - if let Some((pos, token)) = - find_last_matching_token(&tokens, |t| matches!(t, Token::And | Token::Or)) - { - // Check if cursor is after the logical operator - let token_end_pos = if matches!(token, Token::And) { - pos + 3 // "AND" is 3 characters - } else { - pos + 2 // "OR" is 2 characters - }; - - if cursor_pos > token_end_pos { - // Extract any partial word after the operator - let after_op = safe_slice_from(query, token_end_pos + 1); // +1 for the space - let partial = extract_partial_at_end(after_op); - let op = if matches!(token, Token::And) { - LogicalOp::And - } else { - LogicalOp::Or - }; - return (CursorContext::AfterLogicalOp(op), partial); - } - } - - // Check if the last token is AND or OR (handles case where it's at the very end) - if let Some((_, _, last_token)) = tokens.last() { - if matches!(last_token, Token::And | Token::Or) { - let op = if matches!(last_token, Token::And) { - LogicalOp::And - } else { - LogicalOp::Or - }; - return (CursorContext::AfterLogicalOp(op), None); - } - } - - // Check if we're in ORDER BY clause using tokens - if let Some(order_pos) = find_last_token(&tokens, &Token::OrderBy) { - // Check if there's a BY token after ORDER - let has_by = tokens - .iter() - .any(|(pos, _, t)| *pos > order_pos && matches!(t, Token::By)); - if has_by - || tokens - .last() - .map_or(false, |(_, _, t)| matches!(t, Token::OrderBy)) - { - return (CursorContext::OrderByClause, extract_partial_at_end(query)); - } - } - - // Check if we're in WHERE clause using tokens - if is_in_clause(&tokens, Token::Where, &[Token::OrderBy, Token::GroupBy]) { - return (CursorContext::WhereClause, extract_partial_at_end(query)); - } - - // Check if we're in FROM clause using tokens - if is_in_clause( - &tokens, - Token::From, - &[Token::Where, Token::OrderBy, Token::GroupBy], - ) { - return (CursorContext::FromClause, extract_partial_at_end(query)); - } - - // Check if we're in SELECT clause using tokens - if find_last_token(&tokens, &Token::Select).is_some() - && find_last_token(&tokens, &Token::From).is_none() - { - return (CursorContext::SelectClause, extract_partial_at_end(query)); - } - - (CursorContext::Unknown, None) -} - -fn extract_partial_at_end(query: &str) -> Option { - let trimmed = query.trim(); - - // First check if the last word itself starts with a quote (unclosed quoted identifier being typed) - if let Some(last_word) = trimmed.split_whitespace().last() { - if last_word.starts_with('"') && !last_word.ends_with('"') { - // This is an unclosed quoted identifier like "Cust - return Some(last_word.to_string()); - } - } - - // Regular identifier extraction - let last_word = trimmed.split_whitespace().last()?; - - // Check if it's a partial identifier (not a keyword or operator) - // First check if it's alphanumeric (potential identifier) - if last_word.chars().all(|c| c.is_alphanumeric() || c == '_') { - // Use lexer to determine if it's a keyword or identifier - if !is_sql_keyword(last_word) { - Some(last_word.to_string()) - } else { - None - } - } else { - None - } -} - // Implement the ParsePrimary trait for Parser to use the modular expression parsing impl ParsePrimary for Parser { fn current_token(&self) -> &Token { @@ -3040,15 +2509,6 @@ impl ParseCase for Parser { } } -fn is_sql_keyword(word: &str) -> bool { - // Use the lexer to check if this word produces a keyword token - let mut lexer = Lexer::new(word); - let token = lexer.next_token(); - - // Check if it's a keyword token (not an identifier) - !matches!(token, Token::Identifier(_) | Token::Eof) -} - #[cfg(test)] mod tests { use super::*; diff --git a/tests/completion_schema.rs b/tests/completion_schema.rs index cb34a3af..79e8a038 100644 --- a/tests/completion_schema.rs +++ b/tests/completion_schema.rs @@ -132,3 +132,181 @@ fn snapshot_carries_cardinality_for_the_value_completion_gate() { "every country name is distinct, so the gate must exclude it" ); } + +// --------------------------------------------------------------------------- +// T9: cursor context from the token stream. +// +// Each case below is a row of the table in `docs/TUI_FEATURES.md`, measured +// against this file before the change. They run through the same +// `get_completions` entry point the TUI calls, so they pin the suggestions the +// user actually sees, not just the context enum. +// --------------------------------------------------------------------------- + +/// The headline case. Typing the opening quote used to collapse the context to +/// `WhereClause`, which offered all 76 column names *inside the string +/// literal*; typing a letter then filtered them to nothing. +/// +/// Offering nothing is the correct answer until T4 supplies values — the point +/// of this test is that a column name is never one of them. +#[test] +fn a_cursor_inside_a_string_literal_is_not_a_column_position() { + let table = countries(); + let parser = parser_for(&table); + + for query in [ + "SELECT * FROM countries WHERE region = '", + "SELECT * FROM countries WHERE region = 'Am", + "SELECT * FROM countries WHERE region IN ('Asia', '", + // The literal's contents are text, not syntax. + "SELECT * FROM countries WHERE region = 'GROUP BY ", + ] { + let result = parser.get_completions(query, query.len()); + assert!( + result.context.starts_with("InStringLiteral"), + "{query:?} should be a value position, got {}", + result.context + ); + assert!( + result.suggestions.is_empty(), + "{query:?} should offer nothing inside the quotes, got {:?}", + result.suggestions + ); + } +} + +/// `IN (...)` is distinguished from `=` at the context level, which is what +/// lets T5 iterate a list without T4 having to know about lists. +#[test] +fn an_in_list_is_distinguished_from_a_plain_comparison() { + let table = countries(); + let parser = parser_for(&table); + + let plain = "SELECT * FROM countries WHERE region = '"; + assert!(parser + .get_completions(plain, plain.len()) + .context + .contains("list=false")); + + let list = "SELECT * FROM countries WHERE region IN ('Asia', '"; + assert!(parser + .get_completions(list, list.len()) + .context + .contains("list=true")); +} + +/// The span an accepted value replaces is the text *between* the quotes. +/// `find_completion_token` returns nothing useful there — a value is not an +/// identifier — so the analyzer reports it, and T4 can splice without +/// re-deriving anything. +#[test] +fn a_value_replaces_the_span_just_past_the_opening_quote() { + let table = countries(); + let parser = parser_for(&table); + + let query = "SELECT * FROM countries WHERE region = 'Am"; + let result = parser.get_completions(query, query.len()); + assert_eq!(&query[result.replace_start..], "Am"); +} + +/// Was `WhereClause`: the old check required the column to satisfy +/// `chars().all(|c| c.is_alphanumeric() || c == '_')`, which excludes every +/// column that has to be quoted — i.e. most of this file. +#[test] +fn a_quoted_column_reaches_its_comparison_operator() { + let table = countries(); + let parser = parser_for(&table); + + let query = r#"SELECT * FROM countries WHERE "name.common" = "#; + let result = parser.get_completions(query, query.len()); + assert!( + result.context.starts_with("AfterComparison(name.common"), + "expected a value position for a quoted column, got {}", + result.context + ); + // `name.common` is a string column, so the offer is an empty literal. + assert_eq!(result.suggestions, vec!["''".to_string()]); +} + +/// The old operator list was `[" > ", " < ", " = ", …]` — the spaces were part +/// of the pattern, so an operator typed without them was invisible. +#[test] +fn an_operator_without_surrounding_spaces_is_still_an_operator() { + let table = countries(); + let parser = parser_for(&table); + + let query = "SELECT * FROM countries WHERE region="; + let result = parser.get_completions(query, query.len()); + assert!( + result.context.starts_with("AfterComparison(region"), + "got {}", + result.context + ); +} + +/// The completer knew six keywords and guessed at everything else, so clauses +/// the lexer has always understood got confidently wrong answers. +#[test] +fn clauses_the_old_scanner_did_not_know_are_no_longer_guessed() { + let table = countries(); + let parser = parser_for(&table); + + // Was `AfterTable`, which suggested `WHERE` and `ORDER BY` *after* GROUP BY. + let group_by = "SELECT region FROM countries GROUP BY "; + let result = parser.get_completions(group_by, group_by.len()); + assert!(result.context.starts_with("GroupByClause")); + assert!( + result.suggestions.iter().any(|s| s == "region"), + "GROUP BY should offer columns, got {:?}", + result.suggestions + ); + + // Was `FromClause`, which suggested the table name where a row count goes. + let limit = "SELECT * FROM countries LIMIT "; + let result = parser.get_completions(limit, limit.len()); + assert!(result.context.starts_with("LimitClause")); + assert!( + result.suggestions.is_empty(), + "LIMIT takes a number, got {:?}", + result.suggestions + ); +} + +/// A named table means the next thing is a clause, not another table. +#[test] +fn a_completed_table_name_moves_on_to_clause_keywords() { + let table = countries(); + let parser = parser_for(&table); + + let query = "SELECT * FROM countries "; + let result = parser.get_completions(query, query.len()); + assert!(result.suggestions.iter().any(|s| s == "WHERE")); + assert!( + !result.suggestions.iter().any(|s| s == "countries"), + "the table is already named, got {:?}", + result.suggestions + ); +} + +/// Column completion in the ordinary positions must be untouched by all of the +/// above — this is the behaviour T1 and T2 established. +#[test] +fn ordinary_column_completion_still_works() { + let table = countries(); + let parser = parser_for(&table); + + for (query, expected) in [ + ("SELECT * FROM countries WHERE reg", "region"), + ( + "SELECT * FROM countries WHERE region = 'Asia' AND reg", + "region", + ), + ("SELECT nam", "\"name.common\""), + ] { + let result = parser.get_completions(query, query.len()); + assert!( + result.suggestions.iter().any(|s| s == expected), + "{query:?} should still suggest {expected}, got {:?}", + result.suggestions + ); + } +}