linter: remove Identifier wrapper type (#1139)

Author: sbdchd

crates/squawk_ide/src/code_actions/quote_identifier.rs

@@ -1,9 +1,6 @@
 use rowan::TextSize;
 use salsa::Database as Db;
-use squawk_syntax::{
-    ast::{self, AstNode},
-    quote::normalize_identifier,
-};
+use squawk_syntax::ast::{self, AstNode};
 
 use crate::{db::File, offsets::token_from_offset};
 
@@ -18,25 +15,27 @@ pub(super) fn quote_identifier(
     let token = token_from_offset(db, file, offset)?;
     let parent = token.parent()?;
 
-    let name_node = if let Some(name) = ast::Name::cast(parent.clone()) {
-        name.syntax().clone()
-    } else if let Some(name_ref) = ast::NameRef::cast(parent) {
-        name_ref.syntax().clone()
+    let (is_quoted, text, text_range) = if let Some(name) = ast::Name::cast(parent.clone()) {
+        (name.is_quoted(), name.text(), name.syntax().text_range())
+    } else if let Some(name_ref) = ast::NameRef::cast(parent.clone()) {
+        (
+            name_ref.is_quoted(),
+            name_ref.text(),
+            name_ref.syntax().text_range(),
+        )
     } else {
         return None;
     };
 
-    let text = name_node.text().to_string();
-
-    if text.starts_with('"') {
+    if is_quoted {
         return None;
     }
 
-    let quoted = format!(r#""{}""#, normalize_identifier(&text));
+    let quoted = format!(r#""{text}""#);
 
     actions.push(CodeAction {
         title: "Quote identifier".to_owned(),
-        edits: vec![squawk_linter::Edit::replace(name_node.text_range(), quoted)],
+        edits: vec![squawk_linter::Edit::replace(text_range, quoted)],
         kind: ActionKind::RefactorRewrite,
     });
 

crates/squawk_ide/src/code_actions/test_utils.rs

@@ -95,7 +95,7 @@ fn code_action_not_applicable_(
     allow_errors: bool,
 ) -> bool {
     let fixture = Fixture::new(sql);
-    let offset = fixture.marker().offset();
+    let offset = fixture.marker().offset_before();
     let sql = fixture.sql();
     let db = Database::default();
     let file = File::new(&db, sql.into());

crates/squawk_ide/src/collect.rs

@@ -1,7 +1,6 @@
 use crate::ast_nav;
-use crate::builtins::builtins_file;
 use crate::column_name::ColumnName;
-use crate::db::{File, parse};
+use crate::db::{File, list_files, parse};
 use crate::goto_definition::goto_definition;
 use crate::infer::{Type, infer_type_from_expr, infer_type_from_ty};
 use crate::location::{Location, LocationKind};
@@ -216,7 +215,7 @@ pub(crate) fn create_table_as_columns_with_types(
     file: File,
     create_table_as: &ast::CreateTableAs,
 ) -> Vec<(Name, Option<Type>)> {
-    for file in [file, builtins_file(db)] {
+    for file in list_files(db, file) {
         let columns = select_columns_with_types(db, file, &create_table_as.query());
         if !columns.is_empty() {
             return columns;
@@ -426,7 +425,7 @@ pub(crate) fn view_like_columns_with_types(
         .collect();
 
     let mut base_columns = vec![];
-    for file in [file, builtins_file(db)] {
+    for file in list_files(db, file) {
         base_columns = select_columns_with_types(db, file, &create_view.query());
         if !base_columns.is_empty() {
             break;
@@ -464,7 +463,7 @@ pub(crate) fn with_table_columns_with_types(
         .collect();
 
     let mut base_columns = vec![];
-    for file in [file, builtins_file(db)] {
+    for file in list_files(db, file) {
         base_columns = with_table_query_columns_with_types(db, file, with_table.clone());
         if !base_columns.is_empty() {
             break;
@@ -686,7 +685,7 @@ fn columns_for_star_from_table_ptr(
             columns_for_star_from_alias(db, file, &from_item, &alias)
         }
         Some(ast_nav::ParentSouce::WithTable(with_table)) => {
-            for f in [file, builtins_file(db)] {
+            for f in list_files(db, file) {
                 let columns = with_table_columns_with_types(db, f, with_table.clone());
                 if !columns.is_empty() {
                     return columns;
@@ -826,7 +825,7 @@ pub(crate) fn star_column_names(db: &dyn Db, file: File, table_ptr: &SyntaxNodeP
             .filter_map(|column| column.name().map(|name| Name::from_node(&name)))
             .collect(),
         Some(ast_nav::ParentSouce::WithTable(with_table)) => {
-            for file in [file, builtins_file(db)] {
+            for file in list_files(db, file) {
                 let columns: Vec<_> = with_table_columns_with_types(db, file, with_table.clone())
                     .into_iter()
                     .map(|(name, _)| name)

crates/squawk_ide/src/column_name.rs

@@ -3,8 +3,6 @@ use squawk_syntax::{
     ast::{self, AstNode},
 };
 
-use squawk_syntax::quote::normalize_identifier;
-
 #[derive(Clone, Debug, PartialEq)]
 pub(crate) enum ColumnName {
     Column(String),
@@ -29,9 +27,10 @@ impl ColumnName {
         if let Some(as_name) = target.as_name()
             && let Some(name_node) = as_name.name()
         {
-            let text = name_node.text();
-            let normalized = normalize_identifier(&text);
-            return Some((ColumnName::Column(normalized), name_node.syntax().clone()));
+            return Some((
+                ColumnName::Column(name_node.text()),
+                name_node.syntax().clone(),
+            ));
         }
         Self::inferred_from_target(target)
     }
@@ -133,7 +132,7 @@ fn name_from_type(ty: ast::Type, unknown_column: bool) -> Option<(ColumnName, Sy
                 name.push_str("tz");
             };
             return Some((
-                ColumnName::new(name.to_string(), unknown_column),
+                ColumnName::new(name, unknown_column),
                 time_type.syntax().clone(),
             ));
         }
@@ -228,9 +227,10 @@ fn name_from_name_ref(
             }
         }
     }
-    let text = name_ref.text();
-    let normalized = normalize_identifier(&text);
-    return Some((ColumnName::Column(normalized), name_ref.syntax().clone()));
+    return Some((
+        ColumnName::Column(name_ref.text()),
+        name_ref.syntax().clone(),
+    ));
 }
 
 /*

crates/squawk_ide/src/db.rs

@@ -6,6 +6,7 @@ use std::sync::Arc;
 
 use crate::binder;
 use crate::binder::Binder;
+use crate::builtins::builtins_file;
 
 #[salsa::input]
 pub struct File {
@@ -23,6 +24,11 @@ pub fn line_index(db: &dyn Db, file: File) -> LineIndex {
     LineIndex::new(file.content(db))
 }
 
+#[inline]
+pub(crate) fn list_files(db: &dyn Db, file: File) -> impl Iterator<Item = File> {
+    [file, builtins_file(db)].into_iter()
+}
+
 #[salsa::tracked]
 pub fn bind(db: &dyn Db, file: File) -> Binder {
     let result = parse(db, file);

crates/squawk_ide/src/goto_definition.rs

@@ -1,5 +1,4 @@
-use crate::builtins::builtins_file;
-use crate::db::{File, parse};
+use crate::db::{File, list_files, parse};
 use crate::location::{Location, LocationKind};
 use crate::offsets::token_from_offset;
 use crate::resolve;
@@ -66,7 +65,7 @@ pub fn goto_definition(db: &dyn Db, file: File, offset: TextSize) -> SmallVec<[L
     }
 
     if let Some(name_ref) = ast::NameRef::cast(parent.clone()) {
-        for definition_file in [file, builtins_file(db)] {
+        for definition_file in list_files(db, file) {
             if let Some(locations) = resolve::resolve_name_ref(db, definition_file, &name_ref) {
                 return locations;
             }
@@ -82,7 +81,7 @@ pub fn goto_definition(db: &dyn Db, file: File, offset: TextSize) -> SmallVec<[L
         }
     });
     if let Some(ty) = type_node {
-        for definition_file in [file, builtins_file(db)] {
+        for definition_file in list_files(db, file) {
             let position = token.text_range().start();
             if let Some(ptr) =
                 resolve::resolve_type_ptr_from_type(db, definition_file, &ty, position)

crates/squawk_ide/src/hover.rs

@@ -1,9 +1,8 @@
 use crate::ast_nav;
-use crate::builtins::builtins_file;
 use crate::collect;
 use crate::column_name::ColumnName;
 use crate::comments::preceding_comment;
-use crate::db::{File, bind, parse};
+use crate::db::{File, bind, list_files, parse};
 use crate::infer::infer_type_from_expr;
 use crate::location::{Location, LocationKind};
 use crate::name;
@@ -548,7 +547,7 @@ fn hover_qualified_star(db: &dyn Db, file: File, field_expr: ast::FieldExpr) ->
 
 fn hover_unqualified_star(db: &dyn Db, file: File, target: ast::Target) -> Option<Hover> {
     let mut results = vec![];
-    for file in [file, builtins_file(db)] {
+    for file in list_files(db, file) {
         results = hover_unqualified_star_with_binder(db, file, &target);
         if results.is_empty() && target_has_schema_qualified_from_item(&target) {
             continue;

crates/squawk_ide/src/name.rs

@@ -2,8 +2,6 @@ use smol_str::SmolStr;
 use squawk_syntax::ast::{self, AstNode};
 use std::fmt;
 
-use squawk_syntax::quote::normalize_identifier;
-
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub(crate) struct Name(pub(crate) SmolStr);
 
@@ -23,15 +21,20 @@ impl fmt::Display for Schema {
 }
 
 impl Name {
+    // TODO: we should get rid of this and update the ast methods to return
+    // normalized idents.
     pub(crate) fn from_string(text: impl Into<SmolStr>) -> Self {
         let text = text.into();
-        let normalized = normalize_identifier(&text);
-        Name(normalized.into())
+        let text = text
+            .strip_prefix('"')
+            .and_then(|t| t.strip_suffix('"'))
+            .map(|x| x.replace(r#""""#, "\""))
+            .unwrap_or(text.to_ascii_lowercase());
+        Name(text.into())
     }
     pub(crate) fn from_node(node: &impl ast::NameLike) -> Self {
-        let text = node.syntax().text().to_string();
-        let normalized = normalize_identifier(&text);
-        Name(normalized.into())
+        let text = node.text();
+        Name(text.into())
     }
 }
 
@@ -198,7 +201,7 @@ mod test {
     use super::*;
     #[test]
     fn name_case_insensitive_compare() {
-        assert_eq!(Name::from_string("foo"), Name::from_string("FOO"));
+        assert_eq!(Name::from_string("foo"), Name::from_string(r#""foo""#));
     }
 
     #[test]

crates/squawk_ide/src/resolve.rs

@@ -1,12 +1,11 @@
-use crate::ast_nav;
+use crate::{ast_nav, db::list_files};
 use rowan::TextSize;
 use smallvec::{SmallVec, smallvec};
 use squawk_syntax::{
     SyntaxKind, SyntaxNode, SyntaxNodePtr,
     ast::{self, AstNode},
 };
 
-use crate::builtins::builtins_file;
 use crate::column_name::ColumnName;
 use crate::db::File;
 use crate::location::{Location, LocationKind};
@@ -1379,7 +1378,7 @@ pub(crate) fn resolve_table_name(
     position: TextSize,
 ) -> Option<(File, ResolvedTableName)> {
     use ResolvedTableName::*;
-    for file in [file, builtins_file(db)] {
+    for file in list_files(db, file) {
         let Some((ptr, kind)) = resolve_table_like(db, file, None, table_name, schema, position)
         else {
             continue;

crates/squawk_linter/src/rules/adding_field_with_default.rs

@@ -2,20 +2,19 @@ use std::sync::OnceLock;
 
 use rustc_hash::FxHashSet;
 
+use squawk_syntax::ast;
 use squawk_syntax::ast::AstNode;
 use squawk_syntax::{Parse, SourceFile, SyntaxKind};
-use squawk_syntax::{ast, identifier::Identifier};
 
 use crate::{Linter, Rule, Version, Violation};
 
-fn non_volatile_funcs() -> &'static FxHashSet<Identifier> {
-    static NON_VOLATILE_FUNCS: OnceLock<FxHashSet<Identifier>> = OnceLock::new();
+fn non_volatile_funcs() -> &'static FxHashSet<&'static str> {
+    static NON_VOLATILE_FUNCS: OnceLock<FxHashSet<&'static str>> = OnceLock::new();
     NON_VOLATILE_FUNCS.get_or_init(|| {
         NON_VOLATILE_BUILT_IN_FUNCTIONS
             .split('\n')
             .map(|x| x.trim())
             .filter(|x| !x.is_empty())
-            .map(Identifier::new)
             .collect()
     })
 }
@@ -41,8 +40,7 @@ fn is_non_volatile_or_const(expr: &ast::Expr) -> bool {
                     return false;
                 };
 
-                let non_volatile_name =
-                    non_volatile_funcs().contains(&Identifier::new(name_ref.text().as_str()));
+                let non_volatile_name = non_volatile_funcs().contains(name_ref.text().as_str());
 
                 no_args && non_volatile_name
             } else {