pr feedback

Author: Nyrox

README.md

@@ -102,7 +102,7 @@ issues and distinguish different syntaxes in the AST.
 
 ## WIP: Extracting source locations from AST nodes
 
-This crate allows recovering source locations from AST nodes via the [Spanned](https://docs.rs/sqlparser/latest/sqlparser/ast/trait.Spanned.html) trait, which can be used for advanced diagnostics tooling. Node that this feature is a work in progress and many nodes report missing or inaccurate spans. Please see [this document](./docs/source_spans.md#source-span-contributing-guidelines) for information on how to contribute missing improvements.
+This crate allows recovering source locations from AST nodes via the [Spanned](https://docs.rs/sqlparser/latest/sqlparser/ast/trait.Spanned.html) trait, which can be used for advanced diagnostics tooling. Note that this feature is a work in progress and many nodes report missing or inaccurate spans. Please see [this document](./docs/source_spans.md#source-span-contributing-guidelines) for information on how to contribute missing improvements.
 
 ```rust
 use sqlparser::ast::Spanned;

docs/source_spans.md

@@ -8,7 +8,7 @@ These are the current breaking changes introduced by the source spans feature:
 - `Select`, `With`, `Cte`, `WildcardAdditionalOptions` now store a `TokenWithLocation` 
 
 #### Misc.
-- `TokenWithLocation` stores a full `Span`, rathern than just a source location. Users relying on `token.location` should use `token.location.start` instead.
+- `TokenWithLocation` stores a full `Span`, rather than just a source location. Users relying on `token.location` should use `token.location.start` instead.
 ## Source Span Contributing Guidelines
 
 For contributing source spans improvement in addition to the general [contribution guidelines](../README.md#contributing), please make sure to pay attention to the following:
@@ -25,7 +25,7 @@ The primary reason for missing and inaccurate source spans at this time is missi
 
 When considering adding support for source spans on a type, consider the impact to consumers of that type and whether your change would require a consumer to do non-trivial changes to their code.
 
-f.e. of a trivial change
+Example of a trivial change
 ```rust
 match node {  
   ast::Query { 
@@ -39,11 +39,11 @@ If adding source spans to a type would require a significant change like wrappin
 
 ### AST Node Equality and Hashes
 
-When adding tokens to AST nodes, make sure to wrap them in the [IgnoreField](https://docs.rs/sqlparser/latest/sqlparser/ast/helpers/struct.IgnoreField.html) helper to ensure that semantically equivalent AST nodes always compare as equal and hash to the same value. F.e. `select 5` and `SELECT 5` would compare as different `Select` nodes, if the select token was stored directly. f.e.
+When adding tokens to AST nodes, make sure to store them using the [AttachedToken](https://docs.rs/sqlparser/latest/sqlparser/ast/helpers/struct.AttachedToken.html) helper to ensure that semantically equivalent AST nodes always compare as equal and hash to the same value. F.e. `select 5` and `SELECT 5` would compare as different `Select` nodes, if the select token was stored directly. f.e.
 
 ```rust
 struct Select {
-    select_token: IgnoreField<TokenWithLocation>, // only used for spans
+    select_token: AttachedToken, // only used for spans
     /// remaining fields
     field1,
     field2,

src/ast/helpers/attached_token.rs

@@ -21,10 +21,18 @@ use core::hash::{Hash, Hasher};
 
 use crate::tokenizer::TokenWithLocation;
 
+#[cfg(feature = "serde")]
+use serde::{Deserialize, Serialize};
+
+#[cfg(feature = "visitor")]
+use sqlparser_derive::{Visit, VisitMut};
+
 /// A wrapper type for attaching tokens to AST nodes that should be ignored in comparisons and hashing.
 /// This should be used when a token is not relevant for semantics, but is still needed for
 /// accurate source location tracking.
 #[derive(Clone)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
 pub struct AttachedToken(pub TokenWithLocation);
 
 // Conditional Implementations

src/ast/mod.rs

@@ -23,6 +23,7 @@ use alloc::{
     string::{String, ToString},
     vec::Vec,
 };
+use helpers::attached_token::AttachedToken;
 
 use core::ops::Deref;
 use core::{
@@ -36,7 +37,7 @@ use serde::{Deserialize, Serialize};
 #[cfg(feature = "visitor")]
 use sqlparser_derive::{Visit, VisitMut};
 
-use crate::tokenizer::{Span, TokenWithLocation};
+use crate::tokenizer::Span;
 
 pub use self::data_type::{
     ArrayElemTypeDef, CharLengthUnits, CharacterLength, DataType, ExactNumberInfo,
@@ -967,10 +968,10 @@ pub enum Expr {
         /// `<search modifier>`
         opt_search_modifier: Option<SearchModifier>,
     },
-    Wildcard(TokenWithLocation),
+    Wildcard(AttachedToken),
     /// Qualified wildcard, e.g. `alias.*` or `schema.table.*`.
     /// (Same caveats apply to `QualifiedWildcard` as to `Wildcard`.)
-    QualifiedWildcard(ObjectName, TokenWithLocation),
+    QualifiedWildcard(ObjectName, AttachedToken),
     /// Some dialects support an older syntax for outer joins where columns are
     /// marked with the `(+)` operator in the WHERE clause, for example:
     ///

src/ast/spans.rs

@@ -1372,13 +1372,13 @@ impl Spanned for Expr {
             Expr::Map(_) => Span::empty(),
             Expr::Subscript { expr, subscript } => expr.span().union(&subscript.span()),
             Expr::Interval(interval) => interval.value.span(),
-            Expr::Wildcard(token) => token.span,
+            Expr::Wildcard(token) => token.0.span,
             Expr::QualifiedWildcard(object_name, token) => union_spans(
                 object_name
                     .0
                     .iter()
                     .map(|i| i.span)
-                    .chain(iter::once(token.span)),
+                    .chain(iter::once(token.0.span)),
             ),
             Expr::OuterJoin(expr) => expr.span(),
             Expr::Prior(expr) => expr.span(),

src/parser/mod.rs

@@ -886,7 +886,7 @@ impl<'a> Parser<'a> {
                             Token::Mul => {
                                 return Ok(Expr::QualifiedWildcard(
                                     ObjectName(id_parts),
-                                    next_token,
+                                    AttachedToken(next_token),
                                 ));
                             }
                             _ => {
@@ -898,7 +898,7 @@ impl<'a> Parser<'a> {
                 }
             }
             Token::Mul => {
-                return Ok(Expr::Wildcard(next_token));
+                return Ok(Expr::Wildcard(AttachedToken(next_token)));
             }
             _ => (),
         };
@@ -1143,7 +1143,7 @@ impl<'a> Parser<'a> {
                         }
 
                         if let Some(ending_wildcard_token) = ending_wildcard {
-                            Ok(Expr::QualifiedWildcard(ObjectName(id_parts), ending_wildcard_token))
+                            Ok(Expr::QualifiedWildcard(ObjectName(id_parts), AttachedToken(ending_wildcard_token)))
                         } else if self.consume_token(&Token::LParen) {
                             if dialect_of!(self is SnowflakeDialect | MsSqlDialect)
                                 && self.consume_tokens(&[Token::Plus, Token::RParen])
@@ -9203,8 +9203,7 @@ impl<'a> Parser<'a> {
         }
     }
 
-    /// Parse a restricted `SELECT` statement (no CTEs / `UNION` / `ORDER BY`),
-    /// assuming the initial `SELECT` was not yet consumed
+    /// Parse a restricted `SELECT` statement (no CTEs / `UNION` / `ORDER BY`)
     pub fn parse_select(&mut self) -> Result<Select, ParserError> {
         let select_token = self.expect_keyword(Keyword::SELECT)?;
         let value_table_mode =
@@ -11290,10 +11289,10 @@ impl<'a> Parser<'a> {
         match self.parse_wildcard_expr()? {
             Expr::QualifiedWildcard(prefix, token) => Ok(SelectItem::QualifiedWildcard(
                 prefix,
-                self.parse_wildcard_additional_options(token)?,
+                self.parse_wildcard_additional_options(token.0)?,
             )),
             Expr::Wildcard(token) => Ok(SelectItem::Wildcard(
-                self.parse_wildcard_additional_options(token)?,
+                self.parse_wildcard_additional_options(token.0)?,
             )),
             Expr::Identifier(v) if v.value.to_lowercase() == "from" && v.quote_style.is_none() => {
                 parser_err!(

src/tokenizer.rs

@@ -29,10 +29,10 @@ use alloc::{
     vec,
     vec::Vec,
 };
+use core::iter::Peekable;
 use core::num::NonZeroU8;
 use core::str::Chars;
 use core::{cmp, fmt};
-use core::{hash, iter::Peekable};
 
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Serialize};
@@ -522,7 +522,7 @@ impl Span {
 }
 
 /// A [Token] with [Location] attached to it
-#[derive(Debug, Eq, Clone)]
+#[derive(Debug, Clone, Hash, Ord, PartialOrd, Eq, PartialEq)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
 pub struct TokenWithLocation {
@@ -544,35 +544,6 @@ impl TokenWithLocation {
     }
 }
 
-impl core::hash::Hash for TokenWithLocation {
-    fn hash<H: hash::Hasher>(&self, state: &mut H) {
-        let TokenWithLocation { token, span: _ } = self;
-
-        token.hash(state);
-    }
-}
-
-impl PartialEq<TokenWithLocation> for TokenWithLocation {
-    fn eq(&self, other: &TokenWithLocation) -> bool {
-        let TokenWithLocation { token, span: _ } = self;
-
-        token == &other.token
-    }
-}
-
-impl PartialOrd<TokenWithLocation> for TokenWithLocation {
-    fn partial_cmp(&self, other: &TokenWithLocation) -> Option<cmp::Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
-impl Ord for TokenWithLocation {
-    fn cmp(&self, other: &Self) -> cmp::Ordering {
-        let TokenWithLocation { token, span: _ } = self;
-        token.cmp(&other.token)
-    }
-}
-
 impl PartialEq<Token> for TokenWithLocation {
     fn eq(&self, other: &Token) -> bool {
         &self.token == other