diff --git a/prqlc/prqlc-parser/src/lexer/mod.rs b/prqlc/prqlc-parser/src/lexer/mod.rs index 05767633ff1b..8b8c4e53328e 100644 --- a/prqlc/prqlc-parser/src/lexer/mod.rs +++ b/prqlc/prqlc-parser/src/lexer/mod.rs @@ -190,22 +190,29 @@ fn multi_char_operators<'a>() -> impl Parser<'a, ParserInput<'a>, TokenKind, Par )) } +/// Words the lexer reserves as [`TokenKind::Keyword`]; an identifier that +/// collides with one of these has to be written in backticks. +/// +/// [`RESERVED_LITERALS`] is the other half — together the two are the single +/// source of truth for which names need quoting. `prqlc`'s codegen reads both +/// to decide when to quote a name, while the syntax highlighting grammars under +/// `grammars/`, the playground's `prql-syntax.js`, and the docs in +/// `web/book/src/reference/syntax/keywords.md` track them by hand. +pub const KEYWORDS: [&str; 10] = [ + "let", "into", "case", "prql", "type", "module", "internal", "func", "import", "enum", +]; + +/// Words the lexer reserves as [`TokenKind::Literal`] rather than +/// [`TokenKind::Keyword`] — see `boolean()` and `null()`. They're reserved just +/// as firmly as [`KEYWORDS`], since both parsers end with `end_expr()`, so an +/// identifier that collides with one also has to be written in backticks. +pub const RESERVED_LITERALS: [&str; 3] = ["true", "false", "null"]; + fn keyword<'a>() -> impl Parser<'a, ParserInput<'a>, TokenKind, ParserError<'a>> { - choice(( - just("let"), - just("into"), - just("case"), - just("prql"), - just("type"), - just("module"), - just("internal"), - just("func"), - just("import"), - just("enum"), - )) - .to_slice() - .then_ignore(end_expr()) - .map(|s: &str| TokenKind::Keyword(s.to_string())) + choice(KEYWORDS.map(just)) + .to_slice() + .then_ignore(end_expr()) + .map(|s: &str| TokenKind::Keyword(s.to_string())) } fn param<'a>() -> impl Parser<'a, ParserInput<'a>, TokenKind, ParserError<'a>> { diff --git a/prqlc/prqlc-parser/src/lexer/test.rs b/prqlc/prqlc-parser/src/lexer/test.rs index b18d84250dc4..491a692a46e8 100644 --- a/prqlc/prqlc-parser/src/lexer/test.rs +++ b/prqlc/prqlc-parser/src/lexer/test.rs @@ -562,3 +562,18 @@ fn raw_string_delimiters() { ) "#); } + +/// `RESERVED_LITERALS` is hand-written next to `boolean()` / `null()`, so pin +/// it to what the lexer actually does: each entry must lex as a `Literal` +/// rather than an `Ident`, which is what makes it need backticks in codegen. +#[test] +fn test_reserved_literals_lex_as_literals() { + for word in crate::lexer::RESERVED_LITERALS { + let tokens = lexer().parse(word).output().unwrap().to_vec(); + assert!( + matches!(tokens[0].kind, TokenKind::Literal(_)), + "`{word}` lexed as {:?}, expected a Literal", + tokens[0].kind + ); + } +} diff --git a/prqlc/prqlc/src/codegen/ast.rs b/prqlc/prqlc/src/codegen/ast.rs index 8f8daa83d1bd..70350449f3ef 100644 --- a/prqlc/prqlc/src/codegen/ast.rs +++ b/prqlc/prqlc/src/codegen/ast.rs @@ -1,7 +1,7 @@ use std::borrow::Cow; -use std::collections::HashSet; use std::sync::OnceLock; +use prqlc_parser::lexer; use regex::Regex; use super::{WriteOpt, WriteSource}; @@ -91,7 +91,7 @@ impl WriteSource for pr::ExprKind { use pr::ExprKind::*; match &self { - Ident(ident) => Some(ident.to_string()), + Ident(ident) => Some(write_ident(ident)), Pipeline(pipeline) => SeparatedExprs { inline: " | ", @@ -318,23 +318,23 @@ impl WriteSource for pr::Ident { let width = self.path.iter().map(|p| p.len() + 1).sum::() + self.name.len(); opt.consume_width(width as u16)?; - let mut r = String::new(); - for part in &self.path { - r += &write_ident_part(part); - r += "."; - } - r += &write_ident_part(&self.name); - Some(r) + Some(write_ident(self)) } } -fn keywords() -> &'static HashSet<&'static str> { - static KEYWORDS: OnceLock> = OnceLock::new(); - KEYWORDS.get_or_init(|| { - HashSet::from_iter([ - "let", "into", "case", "prql", "type", "module", "internal", "func", - ]) - }) +/// Write a dotted identifier, quoting each part that needs it. +/// +/// `pr::Ident`'s `Display` impl looks similar but has its own copy of the +/// "needs backticks" rule which doesn't know about reserved words, so codegen +/// must not reach for `to_string()` here. +fn write_ident(ident: &pr::Ident) -> String { + let mut r = String::new(); + for part in &ident.path { + r += &write_ident_part(part); + r += "."; + } + r += &write_ident_part(&ident.name); + r } fn valid_prql_ident() -> &'static Regex { @@ -347,7 +347,8 @@ fn valid_prql_ident() -> &'static Regex { } pub fn write_ident_part(s: &str) -> Cow<'_, str> { - if valid_prql_ident().is_match(s) && !keywords().contains(s) { + let reserved = lexer::KEYWORDS.contains(&s) || lexer::RESERVED_LITERALS.contains(&s); + if valid_prql_ident().is_match(s) && !reserved { s.into() } else { format!("`{s}`").into() @@ -784,6 +785,25 @@ let `case` = 5 ); } + /// Every reserved word needs quoting, not just the ones we thought to list + /// by hand — `import` and `enum` were missing from the codegen's own copy + /// of the list, so `let `import` = 5` formatted to unparsable source. + /// + /// `true` / `false` / `null` lex as literals rather than keywords, and were + /// missing for the same reason. In declaration position they fail loudly + /// like `import`; in expression position they're worse, since the output + /// still parses — as a literal instead of the column that was written. + #[test] + fn test_every_reserved_word_is_quoted() { + for word in lexer::KEYWORDS + .iter() + .chain(lexer::RESERVED_LITERALS.iter()) + { + assert_is_formatted(&format!("let `{word}` = 5")); + assert_is_formatted(&format!("from t\nselect {{`{word}`}}")); + } + } + /// Named arguments need their backticks too. Unlike the declaration names /// above, dropping them produces output that still parses — as a different /// call, since the unquoted name splits into a positional argument. diff --git a/prqlc/prqlc/tests/integration/snapshots/integration__queries__fmt__distinct.snap b/prqlc/prqlc/tests/integration/snapshots/integration__queries__fmt__distinct.snap index 48957dc12cad..825bc298f6ed 100644 --- a/prqlc/prqlc/tests/integration/snapshots/integration__queries__fmt__distinct.snap +++ b/prqlc/prqlc/tests/integration/snapshots/integration__queries__fmt__distinct.snap @@ -5,5 +5,5 @@ input_file: prqlc/prqlc/tests/integration/queries/distinct.prql --- from tracks select {album_id, genre_id} -group tracks.`*` (take 1) -sort tracks.`*` +group tracks.* (take 1) +sort tracks.* diff --git a/prqlc/prqlc/tests/integration/snapshots/integration__queries__fmt__set_ops_remove.snap b/prqlc/prqlc/tests/integration/snapshots/integration__queries__fmt__set_ops_remove.snap index 3401bcc505e8..481775b95785 100644 --- a/prqlc/prqlc/tests/integration/snapshots/integration__queries__fmt__set_ops_remove.snap +++ b/prqlc/prqlc/tests/integration/snapshots/integration__queries__fmt__set_ops_remove.snap @@ -5,7 +5,7 @@ input_file: prqlc/prqlc/tests/integration/queries/set_ops_remove.prql --- let distinct = func rel -> ( from t = _param.rel - group {t.`*`} (take 1) + group {t.*} (take 1) ) from_text format:json '{ "columns": ["a"], "data": [[1], [2], [2], [3]] }' diff --git a/web/book/src/reference/syntax/keywords.md b/web/book/src/reference/syntax/keywords.md index bdb4506f3953..ed91bcc0cf95 100644 --- a/web/book/src/reference/syntax/keywords.md +++ b/web/book/src/reference/syntax/keywords.md @@ -93,9 +93,11 @@ PRQL uses following keywords: - **`into`** - variable definition [_more..._](../declarations/variables.md) - **`case`** - flow control [_more..._](../syntax/case.md) - **`type`** - type declaration +- **`enum`** - enumeration type declaration - **`func`** - explicit function declaration [_more..._](../declarations/functions.md) - **`module`** - used internally +- **`import`** - used internally - **`internal`** - used internally - **`true`** - boolean [_more..._](./literals.md#booleans) - **`false`** - boolean [_more..._](./literals.md#booleans)