apache / apache/datafusion-sqlparser-rs

Question: why is the Visitor trait limited to statements, relations & expressions?

Open
#934 25 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
3.5k
Forks
772
Avg merge
4d 9h
Merged PRs (30d)
17

Description

What is the reason for that particular design decision versus providing a more general `Visitor` implementation?

Two options for a generalised Visitor trait come to mind:

1. expose pre + post trait method variants for every AST node type, or
2. expose only two trait methods (`pre_visit` + `post_visit`) with signatures like `fn pre_visit(&mut self, node: &AstNode) -> ControlFlow` - where `AstNode` is an enum with a wrapper variant for every AST node type found in `src/ast/mod.rs` and can be `match`ed against.

Would the maintainers be interested in a PR that implements one of the above two approaches?

My preference would be for option 2 because it would not break the trait when node types are added/removed.

Suggested approach:

1. Define a new `RawVisitor` trait (and `RawVisitorMut` trait) like this:

```rust
pub trait RawVisitor {
type Break;
fn pre_visit(&mut self, node: &AstNode) -> ControlFlow;
fn post_visit(&mut self, node: &AstNode) -> ControlFlow;
}
```

2. Define an adapter type (`RawVisitorAdapter` ?) that accepts a `V: Visitor` generic argument and implements `RawVisitor` & `RawVisitorMut`, which calls the appropriate method on `V` (or none at all)

```rust
struct RawVisitorAdapter(v);

impl RawVisitor for RawVisitorAdapter {
type Break = V::Break;

fn pre_visit(&mut self, node: &AstNode) -> ControlFlow {
match node {
AstNode(Statement) => self.0.pre_visit_statement(...),
// etc
}
}

fn post_visit(&mut self, node: &AstNode) -> ControlFlow;
}
```

3. Change the `Visit` derivation macros to generate code in terms of `RawVisitor` & `RawVisitorMut` instead of `Visitor`, like this:

```rust
pub trait Visit {
fn visit_raw(&self, visitor: &mut V) -> ControlFlow;

// This has an identical signature to the existing trait, but has a default implementation
fn visit(&self, visitor: &mut V) -> ControlFlow {
self.visit_raw(RawVisitorAdapter::new(visitor))
}
}
```

Contributor guide

No contributing guide indexed for this repository

Research direction

Review the existing Visitor traits and Visit derivation macros, then inspect the AST node definitions in src/ast/mod.rs. First confirm with maintainers whether the RawVisitor adapter design or per-node hooks is wanted; done means implementing the agreed generalized visitor API while preserving existing Visitor behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
compilers
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.