Skip to content

Expose values through ValueWithSpan#2281

Open
xitep wants to merge 3 commits intoapache:mainfrom
xitep:values-with-span
Open

Expose values through ValueWithSpan#2281
xitep wants to merge 3 commits intoapache:mainfrom
xitep:values-with-span

Conversation

@xitep
Copy link
Contributor

@xitep xitep commented Mar 20, 2026

  • This exposes values in the AST as ValueWithSpan allowing clients to get hold of the location/span for 1) error reporting and 2) associating metadata with the visited values (the start location can be used to make up an identifier)
  • I'm not entirely sure it's the right approach, but it's at least consistent with other items (e.g. Ident) exposing their span in the AST
  • Relevant to [EPIC] Complete Span (source location) information / feature #1548
  • The Deref<Target = Value> for ValueWithSpan would make an upgrade to this version less hassle:
    fn pre_visit_value(&mut self, value: &ValueWithSpan) -> ControlFlow<Self::Break> {
        match **value { // <--- access `&value.value` through deref via `**`
            Value::Number(_, _) => eprintln!("number"),
            _ => eprintln!(".."),
        }
 ...

@xitep xitep force-pushed the values-with-span branch from c587976 to 6c3e018 Compare March 20, 2026 19:47
Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

Thanks @xitep! The changes look reasonable to me overall

Just sanity checking this assumption:

Tests work only because the parser in the tests sets all tokens' spans to "empty"; ie. there is not "magic" wrt to PartialEq/Eq like for the AttachedToken

Is it rather the case that the tests aren't affected due to the eq impl here?

@xitep
Copy link
Contributor Author

xitep commented Mar 26, 2026

ah, indeed! yes, you are right. it's because of that partial-eq impl.

i was mislead by .with_token(..) which gets invoked by parse_sql_statement in tests (through verified_stmt and similar). however, i just see that that does not apply to verfied_expr, though.

@xitep xitep force-pushed the values-with-span branch from 51392ae to 369f72c Compare March 26, 2026 17:12
@xitep xitep requested a review from iffyio March 26, 2026 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants