fix: cleanup and fix some clippy warnings (sorry, unrelated)

Author: psteinroe

crates/pg_base_db/src/change.rs

@@ -42,9 +42,9 @@ impl ChangedStatement {
     }
 }
 
-fn apply_text_change(text: &String, range: Option<TextRange>, change_text: &String) -> String {
+fn apply_text_change(text: &str, range: Option<TextRange>, change_text: &str) -> String {
     if range.is_none() {
-        return change_text.clone();
+        return change_text.to_string();
     }
 
     let range = range.unwrap();
@@ -53,7 +53,7 @@ fn apply_text_change(text: &String, range: Option<TextRange>, change_text: &Stri
 
     let mut new_text = String::new();
     new_text.push_str(&text[..start]);
-    new_text.push_str(&change_text);
+    new_text.push_str(change_text);
     new_text.push_str(&text[end..]);
 
     new_text
@@ -97,7 +97,7 @@ impl Change {
         self.range.is_some() && self.text.len() < self.range.unwrap().len().into()
     }
 
-    pub fn apply_to_text(&self, text: &String) -> String {
+    pub fn apply_to_text(&self, text: &str) -> String {
         if self.range.is_none() {
             return self.text.clone();
         }
@@ -122,14 +122,10 @@ impl Change {
             changed_statements.extend(
                 doc.drain_statements()
                     .into_iter()
-                    .map(|s| StatementChange::Deleted(s)),
+                    .map(StatementChange::Deleted),
             );
             // TODO also use errors returned by extract sql statement ranges
-            doc.statement_ranges = pg_statement_splitter::split(&self.text)
-                .ranges
-                .iter()
-                .map(|r| r.clone())
-                .collect();
+            doc.statement_ranges = pg_statement_splitter::split(&self.text).ranges.to_vec();
             doc.text = self.text.clone();
             doc.line_index = LineIndex::new(&doc.text);
 
@@ -155,7 +151,7 @@ impl Change {
                         changed_statements.push(StatementChange::Modified(ChangedStatement {
                             statement: StatementRef {
                                 idx: pos,
-                                text: doc.text[r.clone()].to_string(),
+                                text: doc.text[*r].to_string(),
                                 document_url: doc.url.clone(),
                             },
                             // change must be relative to statement
@@ -166,15 +162,9 @@ impl Change {
                         // if addition, expand the range
                         // if deletion, shrink the range
                         if self.is_addition() {
-                            *r = TextRange::new(
-                                r.start(),
-                                r.end() + TextSize::from(self.diff_size()),
-                            );
+                            *r = TextRange::new(r.start(), r.end() + self.diff_size());
                         } else if self.is_deletion() {
-                            *r = TextRange::new(
-                                r.start(),
-                                r.end() - TextSize::from(self.diff_size()),
-                            );
+                            *r = TextRange::new(r.start(), r.end() - self.diff_size());
                         }
                     } else if self.is_addition() {
                         *r += self.diff_size();
@@ -206,7 +196,7 @@ impl Change {
             {
                 changed_statements.push(StatementChange::Deleted(StatementRef {
                     idx,
-                    text: doc.text[r.clone()].to_string(),
+                    text: doc.text[*r].to_string(),
                     document_url: doc.url.clone(),
                 }));
 
@@ -344,15 +334,14 @@ mod tests {
         assert_eq!(d.statement_ranges.len(), 2);
 
         for r in &pg_statement_splitter::split(&d.text).ranges {
-            assert_eq!(
-                d.statement_ranges.iter().position(|x| r == x).is_some(),
-                true,
+            assert!(
+                d.statement_ranges.iter().any(|x| r == x),
                 "should have stmt with range {:#?}",
                 r
             );
         }
 
-        assert_eq!(d.statement_ranges[0], TextRange::new(0.into(), 26.into()));
+        assert_eq!(d.statement_ranges[0], TextRange::new(0.into(), 25.into()));
         assert_eq!(d.statement_ranges[1], TextRange::new(26.into(), 35.into()));
     }
 
@@ -364,8 +353,8 @@ mod tests {
 
         assert_eq!(d.statement_ranges.len(), 2);
 
-        let stmt_1_range = d.statement_ranges[0].clone();
-        let stmt_2_range = d.statement_ranges[1].clone();
+        let stmt_1_range = d.statement_ranges[0];
+        let stmt_2_range = d.statement_ranges[1];
 
         let update_text = " contacts;";
 
@@ -522,8 +511,8 @@ mod tests {
 
         assert_eq!(d.statement_ranges.len(), 2);
 
-        let stmt_1_range = d.statement_ranges[0].clone();
-        let stmt_2_range = d.statement_ranges[1].clone();
+        let stmt_1_range = d.statement_ranges[0];
+        let stmt_2_range = d.statement_ranges[1];
 
         let update_text = ",test";
 

crates/pg_base_db/src/document.rs

@@ -1,4 +1,4 @@
-use std::{hash::Hash, hash::Hasher, ops::RangeBounds, usize};
+use std::{hash::Hash, hash::Hasher, ops::RangeBounds};
 
 use line_index::LineIndex;
 use text_size::{TextRange, TextSize};
@@ -44,18 +44,11 @@ impl Document {
     pub fn new(url: PgLspPath, text: Option<String>) -> Document {
         Document {
             version: 0,
-            line_index: LineIndex::new(&text.as_ref().unwrap_or(&"".to_string())),
+            line_index: LineIndex::new(text.as_ref().unwrap_or(&"".to_string())),
             // TODO: use errors returned by split
-            statement_ranges: text.as_ref().map_or_else(
-                || Vec::new(),
-                |f| {
-                    pg_statement_splitter::split(&f)
-                        .ranges
-                        .iter()
-                        .map(|range| range.clone())
-                        .collect()
-                },
-            ),
+            statement_ranges: text.as_ref().map_or_else(Vec::new, |f| {
+                pg_statement_splitter::split(f).ranges.to_vec()
+            }),
             text: text.unwrap_or("".to_string()),
             url,
         }
@@ -99,7 +92,7 @@ impl Document {
             .enumerate()
             .map(|(idx, range)| StatementRef {
                 document_url: self.url.clone(),
-                text: self.text[range.clone()].to_string(),
+                text: self.text[range].to_string(),
                 idx,
             })
             .collect()
@@ -112,10 +105,10 @@ impl Document {
             .enumerate()
             .map(|(idx, range)| {
                 (
-                    range.clone(),
+                    *range,
                     StatementRef {
                         document_url: self.url.clone(),
-                        text: self.text[range.clone()].to_string(),
+                        text: self.text[*range].to_string(),
                         idx,
                     },
                 )
@@ -130,7 +123,7 @@ impl Document {
             .enumerate()
             .map(|(idx, range)| StatementRef {
                 document_url: self.url.clone(),
-                text: self.text[range.clone()].to_string(),
+                text: self.text[*range].to_string(),
                 idx,
             })
             .collect()
@@ -142,7 +135,7 @@ impl Document {
             .get(pos)
             .map(|range| StatementRef {
                 document_url: self.url.clone(),
-                text: self.text[range.clone()].to_string(),
+                text: self.text[*range].to_string(),
                 idx: pos,
             })
             .unwrap()
@@ -154,10 +147,10 @@ impl Document {
             .get(pos)
             .map(|range| {
                 (
-                    range.clone(),
+                    *range,
                     StatementRef {
                         document_url: self.url.clone(),
-                        text: self.text[range.clone()].to_string(),
+                        text: self.text[*range].to_string(),
                         idx: pos,
                     },
                 )

crates/pg_statement_splitter/src/lib.rs

@@ -17,6 +17,9 @@ pub fn split(sql: &str) -> Parse {
 #[cfg(test)]
 mod tests {
     use ntest::timeout;
+    use pg_lexer::SyntaxKind;
+    use syntax_error::SyntaxError;
+    use text_size::TextRange;
 
     use super::*;
 
@@ -35,18 +38,42 @@ mod tests {
     }
 
     impl Tester {
-        fn expect_statements(&self, expected: Vec<&str>) {
+        fn expect_statements(&self, expected: Vec<&str>) -> &Self {
             assert_eq!(
                 self.parse.ranges.len(),
                 expected.len(),
-                "Expected {} statements, got {}",
+                "Expected {} statements, got {}: {:?}",
                 expected.len(),
-                self.parse.ranges.len()
+                self.parse.ranges.len(),
+                self.parse
+                    .ranges
+                    .iter()
+                    .map(|r| &self.input[*r])
+                    .collect::<Vec<_>>()
             );
 
             for (range, expected) in self.parse.ranges.iter().zip(expected.iter()) {
                 assert_eq!(*expected, self.input[*range].to_string());
             }
+
+            self
+        }
+
+        fn expect_errors(&self, expected: Vec<SyntaxError>) -> &Self {
+            assert_eq!(
+                self.parse.errors.len(),
+                expected.len(),
+                "Expected {} errors, got {}: {:?}",
+                expected.len(),
+                self.parse.errors.len(),
+                self.parse.errors
+            );
+
+            for (err, expected) in self.parse.errors.iter().zip(expected.iter()) {
+                assert_eq!(expected, err);
+            }
+
+            self
         }
     }
 
@@ -72,6 +99,16 @@ mod tests {
         ]);
     }
 
+    #[test]
+    fn insert_expect_error() {
+        Tester::from("\ninsert select 1\n\nselect 3")
+            .expect_statements(vec!["insert select 1", "select 3"])
+            .expect_errors(vec![SyntaxError::new(
+                format!("Expected {:?}", SyntaxKind::Into),
+                TextRange::new(8.into(), 14.into()),
+            )]);
+    }
+
     #[test]
     fn insert_with_select() {
         Tester::from("\ninsert into tbl (id) select 1\n\nselect 3")
@@ -84,6 +121,28 @@ mod tests {
             .expect_statements(vec!["select case when select 2 then 1 else 0 end"]);
     }
 
+    #[test]
+    #[timeout(1000)]
+    fn simple_select() {
+        Tester::from(
+            "
+select id, name, test1231234123, unknown from co;
+
+select 14433313331333
+
+alter table test drop column id;
+
+select lower('test');
+",
+        )
+        .expect_statements(vec![
+            "select id, name, test1231234123, unknown from co;",
+            "select 14433313331333",
+            "alter table test drop column id;",
+            "select lower('test');",
+        ]);
+    }
+
     #[test]
     fn create_rule() {
         Tester::from(
@@ -103,7 +162,7 @@ values ('insert', new.id, now());",
     #[test]
     fn insert_into() {
         Tester::from("randomness\ninsert into tbl (id) values (1)\nselect 3").expect_statements(
-            vec!["randomness", "insert into tbl (id) values (1)", "select 3"],
+            vec!["randomness", "insert into tbl (id) values (1)\nselect 3"],
         );
     }
 

crates/pg_statement_splitter/src/parser.rs

@@ -178,12 +178,10 @@ impl Parser {
             return;
         }
 
-        self.error_at(format!("Expected {:#?}", kind));
-    }
-
-    /// collects an SyntaxError with an `error` message at the current position
-    fn error_at(&mut self, error: String) {
-        todo!("{error}");
+        self.errors.push(SyntaxError::new(
+            format!("Expected {:#?}", kind),
+            self.peek().span,
+        ));
     }
 }
 

crates/pg_statement_splitter/src/parser/common.rs

@@ -2,7 +2,7 @@ use pg_lexer::{SyntaxKind, Token, TokenType};
 
 use super::{
     data::at_statement_start,
-    ddl::create,
+    ddl::{alter, create},
     dml::{cte, delete, insert, select, update},
     Parser,
 };
@@ -52,8 +52,11 @@ pub(crate) fn statement(p: &mut Parser) {
         SyntaxKind::Create => {
             create(p);
         }
+        SyntaxKind::Alter => {
+            alter(p);
+        }
         _ => {
-            unknown(p);
+            unknown(p, &[]);
         }
     }
     p.close_stmt();
@@ -91,7 +94,7 @@ pub(crate) fn case(p: &mut Parser) {
     }
 }
 
-pub(crate) fn unknown(p: &mut Parser) {
+pub(crate) fn unknown(p: &mut Parser, exclude: &[SyntaxKind]) {
     loop {
         match p.peek() {
             Token {
@@ -119,7 +122,7 @@ pub(crate) fn unknown(p: &mut Parser) {
             } => {
                 parenthesis(p);
             }
-            t => match at_statement_start(t.kind) {
+            t => match at_statement_start(t.kind, exclude) {
                 Some(SyntaxKind::Select) => {
                     // we need to check for `as` here to not break on `select as`
                     if p.look_back().map(|t| t.kind) != Some(SyntaxKind::As) {

crates/pg_statement_splitter/src/parser/data.rs

@@ -2,15 +2,21 @@ use pg_lexer::SyntaxKind;
 
 // All tokens listed here must be explicitly handled in the `unknown` function to ensure that we do
 // not break in the middle of another statement that contains a statement start token.
+//
+// All of these statements must have a dedicated parser function called from the `statement` function
 static STATEMENT_START_TOKENS: &[SyntaxKind] = &[
     SyntaxKind::With,
     SyntaxKind::Select,
     SyntaxKind::Insert,
     SyntaxKind::Update,
     SyntaxKind::DeleteP,
     SyntaxKind::Create,
+    SyntaxKind::Alter,
 ];
 
-pub(crate) fn at_statement_start(kind: SyntaxKind) -> Option<SyntaxKind> {
-    STATEMENT_START_TOKENS.iter().find(|&x| x == &kind).cloned()
+pub(crate) fn at_statement_start(kind: SyntaxKind, exclude: &[SyntaxKind]) -> Option<&SyntaxKind> {
+    STATEMENT_START_TOKENS
+        .iter()
+        .filter(|&x| !exclude.contains(x))
+        .find(|&x| x == &kind)
 }