Sketch of a visitor pattern

This visitor pattern is designed for doing normalizations on executable
documents, so it assumes you will mutate the tree as it is walked.
(Perhaps a better design would be to make each visitor return an object
of the type it is visiting (which could be the same object it received),
constructing a new tree (possibly sharing most of its data) immutably.)

This highlights some places where it would be helpful if the AST
contained more explicit types where it currently only has variants, type
aliases, or vecs. By having more little structs in the type system, it's
easier to write a visitor like this that can easily target (say) every
Name node. This includes:

- It would be helpful if all enums (which actually contain data, so not
  OperationType) had a specific type for each variant, so that visitors
  can choose to visit that variant specifically. Definition and
  Selection work this way already; Type and Value do not. This is
  especially helpful for Value, as it would be nice to be able to write
  visitors that visit just string values or int values or whatever
  without needing to match against the Value.
- Most uses of NodeStr in the AST are names or descriptions. It would be
  helpful if there were (non-alias) Name and Description types that
  could implement Walkable like the other AST types, and then you could
  implement `visit_name`, etc. (Sure, we could just call `visit_name`
  directly from each Walkable implementation for a type containing a
  name, but keeping the strict pattern of "each `walk` calls `visit_*`
  exactly once and then calls `walk` on its children" seems nice.)
- It would be nice if some more of the Vec types were a struct like
  Directives, though I suppose implementing Walkable on `Vec<Selection>`
  isn't that much more awkward than implementing it on `SelectionSet`.
  I'm thinking `SelectionSet` and `Arguments` specifically.

It's possible I'm missing something and the status quo is pretty
reasonable though.

It would be nice if apollo-compiler had some sort of built-in visitor
API; maybe this one, maybe a more immutable one, maybe something else.
Whether or not apollo-compiler adds such an API, this PR hopefully at
least points out some potential improvements in the AST data types.

Author: glasser

crates/apollo-compiler/src/ast/mod.rs

@@ -39,6 +39,7 @@ use std::sync::Arc;
 pub(crate) mod from_cst;
 pub(crate) mod impls;
 pub(crate) mod serialize;
+pub(crate) mod visitor;
 
 pub use self::serialize::Serialize;
 

crates/apollo-compiler/src/ast/visitor.rs

@@ -0,0 +1,348 @@
+use super::*;
+
+/// Visitor for an AST. Implement this trait and pass it to
+/// document.walk(visitor). This visitor is expected to mutate the AST, so each
+/// call gets `&mut` (and the AST gets fully copied-on-write as it walks it).
+/// Visitors cannot control the walk or return a value, but they can mutate
+/// their nodes. It is a preorder traversal; any changes made by a `visit_*`
+/// function affect which children are visited.
+trait Visitor {
+    fn visit_document(&mut self, _document: &mut Document) {}
+    fn visit_operation_definition(&mut self, _operation_definition: &mut OperationDefinition) {}
+    fn visit_fragment_definition(&mut self, _fragment_definition: &mut FragmentDefinition) {}
+    fn visit_directive_definition(&mut self, _directive_definition: &mut DirectiveDefinition) {}
+    fn visit_arguments(&mut self, _argument: &mut Vec<Node<Argument>>) {}
+    fn visit_argument(&mut self, _argument: &mut Argument) {}
+    fn visit_directives(&mut self, _directives: &mut Directives) {}
+    fn visit_directive(&mut self, _directive: &mut Directive) {}
+    fn visit_operation_type(&mut self, _operation_type: &mut OperationType) {}
+    fn visit_directive_location(&mut self, _directive_location: &mut DirectiveLocation) {}
+    fn visit_variable_definition(&mut self, _variable_definition: &mut VariableDefinition) {}
+    // XXX Maybe should visit the individual enum variants instead?
+    // We aren't calling this recursively right now.
+    fn visit_type(&mut self, _ty: &mut Type) {}
+    fn visit_input_value_definition(&mut self, _input_value_definition: &mut InputValueDefinition) {
+    }
+    fn visit_selection_set(&mut self, _selection_set: &mut Vec<Selection>) {}
+    fn visit_field(&mut self, _field: &mut Field) {}
+    fn visit_fragment_spread(&mut self, _fragment_spread: &mut FragmentSpread) {}
+    fn visit_inline_fragment(&mut self, _inline_fragment: &mut InlineFragment) {}
+    fn visit_value(&mut self, _value: &mut Value) {}
+}
+
+/// Trait encapsulating the traversing logic for the AST
+trait Walkable {
+    /// Visit children of the current node
+    fn walk<V: Visitor>(&mut self, visitor: &mut V);
+}
+
+impl Walkable for Document {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_document(self);
+        for definition in &mut self.definitions {
+            definition.walk(visitor);
+        }
+    }
+}
+
+impl Walkable for Definition {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        match self {
+            Definition::OperationDefinition(node) => node.make_mut().walk(visitor),
+            Definition::FragmentDefinition(node) => node.make_mut().walk(visitor),
+            Definition::DirectiveDefinition(node) => node.make_mut().walk(visitor),
+            _ => panic!("SDL visitors not implemented"),
+        }
+    }
+}
+
+impl Walkable for OperationDefinition {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_operation_definition(self);
+        self.operation_type.walk(visitor);
+        // #MoreSpecificTypes self.name.walk(visitor);
+        for variable in &mut self.variables {
+            variable.make_mut().walk(visitor);
+        }
+        self.directives.walk(visitor);
+        self.selection_set.walk(visitor);
+    }
+}
+
+impl Walkable for FragmentDefinition {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_fragment_definition(self);
+        // #MoreSpecificTypes self.name.walk(visitor);
+        // #MoreSpecificTypes self.type_condition.walk(visitor);
+        self.directives.walk(visitor);
+        self.selection_set.walk(visitor);
+    }
+}
+
+impl Walkable for DirectiveDefinition {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_directive_definition(self);
+        // #MoreSpecificTypes self.description.walk(visitor);
+        // #MoreSpecificTypes self.name.walk(visitor);
+        for argument in &mut self.arguments {
+            argument.make_mut().walk(visitor);
+        }
+        for location in &mut self.locations {
+            location.walk(visitor);
+        }
+    }
+}
+
+impl Walkable for Vec<Node<Argument>> {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_arguments(self);
+        for argument in self {
+            argument.make_mut().walk(visitor);
+        }
+    }
+}
+
+impl Walkable for Argument {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_argument(self);
+        // #MoreSpecificTypes self.name.walk(visitor);
+        self.value.make_mut().walk(visitor);
+    }
+}
+
+impl Walkable for Directives {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_directives(self);
+        for directive in &mut self.0 {
+            directive.make_mut().walk(visitor);
+        }
+    }
+}
+
+impl Walkable for Directive {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_directive(self);
+        // #MoreSpecificTypes self.name.walk(visitor);
+        self.arguments.walk(visitor);
+    }
+}
+
+impl Walkable for OperationType {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_operation_type(self);
+    }
+}
+
+impl Walkable for DirectiveLocation {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_directive_location(self);
+    }
+}
+
+impl Walkable for VariableDefinition {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_variable_definition(self);
+        // #MoreSpecificTypes self.name.walk(visitor);
+        self.ty.make_mut().walk(visitor);
+        self.default_value
+            .as_mut()
+            .map(|value| value.make_mut().walk(visitor));
+        self.directives.walk(visitor);
+    }
+}
+
+impl Walkable for Type {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_type(self);
+        // No recursive walks for the moment (including to the name). #MoreSpecificTypes
+    }
+}
+
+impl Walkable for InputValueDefinition {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_input_value_definition(self);
+        // #MoreSpecificTypes self.name.walk(visitor);
+        self.ty.make_mut().walk(visitor);
+        self.default_value
+            .as_mut()
+            .map(|value| value.make_mut().walk(visitor));
+        self.directives.walk(visitor);
+    }
+}
+
+impl Walkable for Vec<Selection> {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_selection_set(self);
+        for selection in self {
+            selection.walk(visitor);
+        }
+    }
+}
+
+impl Walkable for Selection {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        match self {
+            Selection::Field(node) => node.make_mut().walk(visitor),
+            Selection::FragmentSpread(node) => node.make_mut().walk(visitor),
+            Selection::InlineFragment(node) => node.make_mut().walk(visitor),
+        }
+    }
+}
+
+impl Walkable for Field {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_field(self);
+        // #MoreSpecificTypes self.alias.walk(visitor);
+        // #MoreSpecificTypes self.name.walk(visitor);
+        self.arguments.walk(visitor);
+        self.directives.walk(visitor);
+        self.selection_set.walk(visitor);
+    }
+}
+
+impl Walkable for FragmentSpread {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_fragment_spread(self);
+        // #MoreSpecificTypes self.fragment_name.walk(visitor);
+        self.directives.walk(visitor);
+    }
+}
+
+impl Walkable for InlineFragment {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        visitor.visit_inline_fragment(self);
+        // #MoreSpecificTypes self.type_condition.walk(visitor);
+        self.directives.walk(visitor);
+        self.selection_set.walk(visitor);
+    }
+}
+
+impl Walkable for Value {
+    fn walk<V: Visitor>(&mut self, visitor: &mut V) {
+        // #MoreSpecificTypes I would rather have visitors for StringValue,
+        // IntValue, etc, instead of a Value visitor that has to match.
+        visitor.visit_value(self);
+        match self {
+            Value::List(values) => {
+                for value in values {
+                    value.make_mut().walk(visitor);
+                }
+            }
+            Value::Object(values) => {
+                for (_, value) in values {
+                    value.make_mut().walk(visitor);
+                }
+            }
+            // Nothing else is needed for non-recursive cases.
+            _ => {}
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::Diagnostics;
+
+    use super::*;
+
+    struct HideStringAndNumericLiterals;
+
+    impl Visitor for HideStringAndNumericLiterals {
+        fn visit_value(&mut self, value: &mut Value) {
+            // This is awkward; I'd prefer to define visit_int_value, etc.
+            match value {
+                Value::Int(int_value) => int_value.0 = "0".to_string(),
+                Value::Float(float_value) => float_value.0 = "0".to_string(),
+                Value::String(node_str) => *node_str = NodeStr::new(""),
+                _ => {}
+            }
+        }
+    }
+
+    #[test]
+    fn it_hides_string_and_numeric_literals() -> Result<(), Diagnostics> {
+        let input = r#"
+          query Foo($b: Int = 5, $a: Boolean, $c: String = "asdf", $d: Float = 0.5) {
+            user(
+              name: "hello"
+              age: 5
+              pct: 0.4
+              lst: ["a", "b", "c"]
+              obj: { a: "a", b: 1 }
+            ) {
+              ...Bar
+              ... on User {
+                hello
+                bee
+              }
+              tz @someDirective(a: [500])
+              aliased: name
+              withInputs(
+                str: "hi"
+                int: 2
+                flt: 0.3
+                lst: ["", "", ""]
+                obj: { q: "", s: 0 }
+              )
+            }
+          }
+  
+          fragment Bar on User {
+            age @skip(if: $a)
+            ...Nested
+          }
+  
+          fragment Nested on User {
+            blah
+          }
+        "#;
+
+        let expected_output = r#"
+          query Foo($b: Int = 0, $a: Boolean, $c: String = "", $d: Float = 0) {
+            user(
+              name: ""
+              age: 0
+              pct: 0
+              lst: ["", "", ""]
+              obj: { a: "", b: 0 }
+            ) {
+              ...Bar
+              ... on User {
+                hello
+                bee
+              }
+              tz @someDirective(a: [0])
+              aliased: name
+              withInputs(
+                str: ""
+                int: 0
+                flt: 0
+                lst: ["", "", ""]
+                obj: { q: "", s: 0 }
+              )
+            }
+          }
+  
+          fragment Bar on User {
+            age @skip(if: $a)
+            ...Nested
+          }
+  
+          fragment Nested on User {
+            blah
+          }
+        "#;
+        let mut document = Document::parse(input, "input.graphql");
+        document.check_parse_errors()?;
+        document.walk(&mut HideStringAndNumericLiterals);
+
+        // We don't care about formatting differences, so normalize the above
+        // expected output.
+        let normalized_expected_output =
+            Document::parse(expected_output, "expected_output.graphql").to_string();
+
+        assert_eq!(document.to_string(), normalized_expected_output);
+
+        Ok(())
+    }
+}