Skip to content

Correctly tokenize nested comments #1629

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 5, 2025
Merged

Correctly tokenize nested comments #1629

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b56b9be
Correctly handle nested comments
hansott Dec 30, 2024
5c6d3ed
Fix comment
hansott Dec 30, 2024
f068f5e
Use loop with test cases
hansott Dec 30, 2024
d7c8b3b
Add character after nested comment
hansott Dec 30, 2024
e1bd5c3
Add option for nested comments to dialects
hansott Dec 30, 2024
4a6ab2b
Move if statements to match inside loop and avoid unwrap
hansott Jan 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/dialect/generic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,4 +131,8 @@ impl Dialect for GenericDialect {
fn supports_empty_projections(&self) -> bool {
true
}

fn supports_nested_comments(&self) -> bool {
true
}
}
6 changes: 6 additions & 0 deletions src/dialect/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,12 @@ pub trait Dialect: Debug + Any {
false
}

/// Returns true if the dialect supports nested comments
/// e.g. `/* /* nested */ */`
fn supports_nested_comments(&self) -> bool {
false
}

/// Returns true if this dialect supports treating the equals operator `=` within a `SelectItem`
/// as an alias assignment operator, rather than a boolean expression.
/// For example: the following statements are equivalent for such a dialect:
Expand Down
4 changes: 4 additions & 0 deletions src/dialect/postgresql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,10 @@ impl Dialect for PostgreSqlDialect {
fn supports_empty_projections(&self) -> bool {
true
}

fn supports_nested_comments(&self) -> bool {
true
}
}

pub fn parse_create(parser: &mut Parser) -> Option<Result<Statement, ParserError>> {
Expand Down
101 changes: 91 additions & 10 deletions src/tokenizer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1855,22 +1855,31 @@ impl<'a> Tokenizer<'a> {
) -> Result<Option<Token>, TokenizerError> {
let mut s = String::new();
let mut nested = 1;
let mut last_ch = ' ';
let supports_nested_comments = self.dialect.supports_nested_comments();

loop {
match chars.next() {
Some(ch) => {
if last_ch == '/' && ch == '*' {
if ch == '/' && matches!(chars.peek(), Some('*')) && supports_nested_comments {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we move some of the condition out to the match statement? thinking that way we avoid extra nesting + continue. And the different cases are clearer. e.g.

Some('/') if matches!(chars.peek(), Some('*')) && supports_nested_comments => { ... }
Some('*') if matches!(chars.peek(), Some('/')) => { ... }
Some(ch) => { ... }
None => { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, will do!

s.push(ch);
s.push(chars.next().unwrap()); // consume the '*'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we rewrite this to return an error in order to avoid the unwrap?

nested += 1;
} else if last_ch == '*' && ch == '/' {
continue;
}

if ch == '*' && matches!(chars.peek(), Some('/')) {
s.push(ch);
let slash = chars.next();
nested -= 1;
if nested == 0 {
s.pop();
s.pop(); // remove the last '*'
break Ok(Some(Token::Whitespace(Whitespace::MultiLineComment(s))));
}
s.push(slash.unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, it would be nice if we can avoid unwraping in code

continue;
}

s.push(ch);
last_ch = ch;
}
None => {
break self.tokenizer_error(
Expand Down Expand Up @@ -2718,18 +2727,90 @@ mod tests {

#[test]
fn tokenize_nested_multiline_comment() {
let sql = String::from("0/*multi-line\n* \n/* comment \n /*comment*/*/ */ /comment*/1");
let dialect = GenericDialect {};
let test_cases = vec![
(
"0/*multi-line\n* \n/* comment \n /*comment*/*/ */ /comment*/1",
vec![
Token::Number("0".to_string(), false),
Token::Whitespace(Whitespace::MultiLineComment(
"multi-line\n* \n/* comment \n /*comment*/*/ ".into(),
)),
Token::Whitespace(Whitespace::Space),
Token::Div,
Token::Word(Word {
value: "comment".to_string(),
quote_style: None,
keyword: Keyword::COMMENT,
}),
Token::Mul,
Token::Div,
Token::Number("1".to_string(), false),
],
),
(
"0/*multi-line\n* \n/* comment \n /*comment/**/ */ /comment*/*/1",
vec![
Token::Number("0".to_string(), false),
Token::Whitespace(Whitespace::MultiLineComment(
"multi-line\n* \n/* comment \n /*comment/**/ */ /comment*/".into(),
)),
Token::Number("1".to_string(), false),
],
),
(
"SELECT 1/* a /* b */ c */0",
vec![
Token::make_keyword("SELECT"),
Token::Whitespace(Whitespace::Space),
Token::Number("1".to_string(), false),
Token::Whitespace(Whitespace::MultiLineComment(" a /* b */ c ".to_string())),
Token::Number("0".to_string(), false),
],
),
];

for (sql, expected) in test_cases {
let tokens = Tokenizer::new(&dialect, sql).tokenize().unwrap();
compare(expected, tokens);
}
}

#[test]
fn tokenize_nested_multiline_comment_empty() {
let sql = "select 1/*/**/*/0";

let dialect = GenericDialect {};
let tokens = Tokenizer::new(&dialect, &sql).tokenize().unwrap();
let tokens = Tokenizer::new(&dialect, sql).tokenize().unwrap();
let expected = vec![
Token::make_keyword("select"),
Token::Whitespace(Whitespace::Space),
Token::Number("1".to_string(), false),
Token::Whitespace(Whitespace::MultiLineComment("/**/".to_string())),
Token::Number("0".to_string(), false),
];

compare(expected, tokens);
}

#[test]
fn tokenize_nested_comments_if_not_supported() {
let dialect = SQLiteDialect {};
let sql = "SELECT 1/*/* nested comment */*/0";
let tokens = Tokenizer::new(&dialect, sql).tokenize();
let expected = vec![
Token::make_keyword("SELECT"),
Token::Whitespace(Whitespace::Space),
Token::Number("1".to_string(), false),
Token::Whitespace(Whitespace::MultiLineComment(
"multi-line\n* \n/* comment \n /*comment*/*/ */ /comment".to_string(),
"/* nested comment ".to_string(),
)),
Token::Number("1".to_string(), false),
Token::Mul,
Token::Div,
Token::Number("0".to_string(), false),
];
compare(expected, tokens);

compare(expected, tokens.unwrap());
}

#[test]
Expand Down
Loading