SQLite: parse the full PRAGMA value grammar - #2469
Conversation
| let span = self.peek_token_ref().span; | ||
| let v = match self.parse_one_of_keywords(&[ | ||
| Keyword::TRUE, | ||
| Keyword::FALSE, | ||
| Keyword::ON, | ||
| Keyword::OFF, | ||
| Keyword::YES, | ||
| Keyword::NO, | ||
| ]) { | ||
| Some(keyword) => Value::Boolean(matches!( | ||
| keyword, | ||
| Keyword::TRUE | Keyword::YES | Keyword::ON | ||
| )) | ||
| .with_span(span), | ||
| None => self.parse_value()?, | ||
| }; | ||
| match &v.value { | ||
| Value::SingleQuotedString(_) => Ok(v), | ||
| Value::DoubleQuotedString(_) => Ok(v), | ||
| Value::Number(_, _) => Ok(v), | ||
| Value::Boolean(_) => Ok(v), | ||
| Value::Placeholder(_) => Ok(v), | ||
| _ => { | ||
| self.prev_token(); | ||
| self.expected_ref("number or string or ? placeholder", self.peek_token_ref()) | ||
| self.expected_ref( | ||
| "boolean, number, string, or ? placeholder", | ||
| self.peek_token_ref(), | ||
| ) |
There was a problem hiding this comment.
I think we could simplify the logic to have it accept whatever parse_value returns. and keep the sqlite semantics for validation upstream? that would have avoided this bug in the initial version
There was a problem hiding this comment.
I checked SQLite docs hoping for a limited set of allowed values, it states there is explicitly no such set, and I therefore switched to Expr
0fa5f24 to
4dfcf28
Compare
LucaCappelletti94
changed the title
4dfcf28 to
582c4e4
Compare
| if matches!(self.peek_token_ref().token, Token::Plus | Token::Minus) { | ||
| let op = match self.next_token().token { | ||
| Token::Plus => UnaryOperator::Plus, | ||
| _ => UnaryOperator::Minus, | ||
| }; | ||
| return Ok(Expr::UnaryOp { | ||
| op, | ||
| expr: Box::new(Expr::Value(self.parse_value()?)), | ||
| }); | ||
| } | ||
| match self.maybe_parse(|parser| parser.parse_value())? { | ||
| Some(value) => Ok(Expr::Value(value)), | ||
| None => Ok(Expr::Identifier(self.parse_identifier()?)), | ||
| } |
There was a problem hiding this comment.
hmm can this be replaced by a call to parse_expr()?
There was a problem hiding this comment.
In terms of accepting things, yes it would work, but parse_expr() would over-extend the grammar accepting arbitrary expressions like PRAGMA p = 1 + 2, foo(bar), a AND b or (SELECT 1).
SQLite defines a pragma value as signed-number | name | signed-literal, so parse_pragma_value mirrors that production exactly while still leaving the per-pragma value validation to execution.
I understand I tend to lean much more strongly on the strict parser approach, but here I really believe allowing any expr would be excessively loose.
2 participants
PRAGMAvalues such asjournal_mode = WAL,synchronous = NORMAL,cache_size = -2000andoptimize = 0x10002were rejected. The earlier boolean handling also kept its own keyword list plus a value-type allowlist, and that duplicated allowlist is what rejectedPRAGMA case_sensitive_like = truein the first place.parse_pragma_valuenow follows SQLite's own definition of a pragma value,signed-number | name | signed-literal, and stores the result as anExpr. A bare name likeWALis kept as an identifier, so the parser no longer decides which value a given pragma accepts and leaves that to execution the way SQLite does.parse_pragmabuilds the statement in a single place.This changes
Statement::Pragma.valuefromOption<ValueWithSpan>toOption<Expr>.