Merge pull request #136 from tree-sitter/error-improvements

Error improvements and small bug fix

CHANGELOG.md

···
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## v0.10.4 -- 2023-06-02
+
+### Library
+
+#### Added
+
+- Several errors include more context in the error message: Duplicate errors report both statements using source snippets. Edge statements report which argument (the source or the sink) triggered an evluation error.
+
+#### Fixed
+
+- Ensure that edge attribute statements are executed after edges are created, to prevent non-deterministic ordering bugs in lazy execution mode.
+
 ## v0.10.3 -- 2023-06-01
 
 ### DSL

Cargo.toml

···
 [package]
 name = "tree-sitter-graph"
-version = "0.10.3"
+version = "0.10.4"
 description = "Construct graphs from parsed source code"
 homepage = "https://github.com/tree-sitter/tree-sitter-graph/"
 repository = "https://github.com/tree-sitter/tree-sitter-graph/"

src/execution/lazy.rs

···
         let mut locals = VariableMap::new();
         let mut store = LazyStore::new();
         let mut scoped_store = LazyScopedVariables::new();
-        let mut lazy_graph = Vec::new();
+        let mut lazy_graph = LazyGraph::new();
         let mut function_parameters = Vec::new();
         let mut prev_element_debug_info = HashMap::new();
 
···
             prev_element_debug_info: &mut prev_element_debug_info,
             cancellation_flag,
         };
-        for graph_stmt in &lazy_graph {
-            graph_stmt.evaluate(&mut exec)?;
-        }
+        lazy_graph.evaluate(&mut exec)?;
         // make sure any unforced values are now forced, to surface any problems
         // hidden by the fact that the values were unused
         store.evaluate_all(&mut exec)?;
···
     mat: &'a QueryMatch<'a, 'tree>,
     store: &'a mut LazyStore,
     scoped_store: &'a mut LazyScopedVariables,
-    lazy_graph: &'a mut Vec<LazyStatement>,
+    lazy_graph: &'a mut LazyGraph,
     function_parameters: &'a mut Vec<graph::Value>, // re-usable buffer to reduce memory allocations
     prev_element_debug_info: &'a mut HashMap<GraphElementKey, DebugInfo>,
     error_context: StatementContext,
···
         locals: &mut VariableMap<'l, LazyValue>,
         store: &mut LazyStore,
         scoped_store: &mut LazyScopedVariables,
-        lazy_graph: &mut Vec<LazyStatement>,
+        lazy_graph: &mut LazyGraph,
         function_parameters: &mut Vec<graph::Value>,
         prev_element_debug_info: &mut HashMap<GraphElementKey, DebugInfo>,
         inherited_variables: &HashSet<Identifier>,

src/execution/lazy/statements.rs

···
 use super::EvaluationContext;
 use super::GraphElementKey;
 
+/// Lazy graph
+#[derive(Debug)]
+pub(super) struct LazyGraph {
+    edge_statements: Vec<LazyStatement>,
+    attr_statements: Vec<LazyStatement>,
+    print_statements: Vec<LazyStatement>,
+}
+
+impl LazyGraph {
+    pub(super) fn new() -> Self {
+        LazyGraph {
+            edge_statements: Vec::new(),
+            attr_statements: Vec::new(),
+            print_statements: Vec::new(),
+        }
+    }
+
+    pub(super) fn push(&mut self, stmt: LazyStatement) {
+        match stmt {
+            LazyStatement::AddGraphNodeAttribute(_) => self.attr_statements.push(stmt),
+            LazyStatement::CreateEdge(_) => self.edge_statements.push(stmt),
+            LazyStatement::AddEdgeAttribute(_) => self.attr_statements.push(stmt),
+            LazyStatement::Print(_) => self.print_statements.push(stmt),
+        }
+    }
+
+    pub(super) fn evaluate(&self, exec: &mut EvaluationContext) -> Result<(), ExecutionError> {
+        for stmt in &self.edge_statements {
+            stmt.evaluate(exec)?;
+        }
+        for stmt in &self.attr_statements {
+            stmt.evaluate(exec)?;
+        }
+        for stmt in &self.print_statements {
+            stmt.evaluate(exec)?;
+        }
+        Ok(())
+    }
+}
+
 /// Lazy graph statements
 #[derive(Debug)]
 pub(super) enum LazyStatement {
···
     }
 
     pub(super) fn evaluate(&self, exec: &mut EvaluationContext) -> Result<(), ExecutionError> {
-        let node = self.node.evaluate_as_graph_node(exec)?;
+        let node = self
+            .node
+            .evaluate_as_graph_node(exec)
+            .with_context(|| "Evaluating target node".to_string().into())?;
         for attribute in &self.attributes {
             let value = attribute.value.evaluate(exec)?;
             let prev_debug_info = exec.prev_element_debug_info.insert(
                 GraphElementKey::NodeAttribute(node, attribute.name.clone()),
                 self.debug_info.clone(),
             );
-            exec.graph[node]
+            if let Err(_) = exec.graph[node]
                 .attributes
                 .add(attribute.name.clone(), value)
-                .map_err(|_| {
-                    ExecutionError::DuplicateAttribute(format!(
-                        "{} on {} at {} and {}",
-                        attribute.name,
-                        node,
-                        prev_debug_info.unwrap(),
-                        self.debug_info,
-                    ))
-                })?;
+            {
+                return Err(ExecutionError::DuplicateAttribute(format!(
+                    "{} on {}",
+                    attribute.name, node,
+                )))
+                .with_context(|| {
+                    (
+                        prev_debug_info.unwrap().into(),
+                        self.debug_info.clone().into(),
+                    )
+                        .into()
+                });
+            };
         }
         Ok(())
     }
···
     }
 
     pub(super) fn evaluate(&self, exec: &mut EvaluationContext) -> Result<(), ExecutionError> {
-        let source = self.source.evaluate_as_graph_node(exec)?;
-        let sink = self.sink.evaluate_as_graph_node(exec)?;
+        let source = self
+            .source
+            .evaluate_as_graph_node(exec)
+            .with_context(|| "Evaluating edge source".to_string().into())?;
+        let sink = self
+            .sink
+            .evaluate_as_graph_node(exec)
+            .with_context(|| "Evaluating edge sink".to_string().into())?;
         let prev_debug_info = exec
             .prev_element_debug_info
             .insert(GraphElementKey::Edge(source, sink), self.debug_info.clone());
···
             Ok(edge) => edge,
             Err(_) => {
                 return Err(ExecutionError::DuplicateEdge(format!(
-                    "({} -> {}) at {} and {}",
-                    source,
-                    sink,
-                    prev_debug_info.unwrap(),
-                    self.debug_info,
-                )))?
+                    "({} -> {})",
+                    source, sink,
+                )))
+                .with_context(|| {
+                    (
+                        prev_debug_info.unwrap().into(),
+                        self.debug_info.clone().into(),
+                    )
+                        .into()
+                });
             }
         };
         edge.attributes = self.attributes.clone();
···
     }
 
     pub(super) fn evaluate(&self, exec: &mut EvaluationContext) -> Result<(), ExecutionError> {
-        let source = self.source.evaluate_as_graph_node(exec)?;
-        let sink = self.sink.evaluate_as_graph_node(exec)?;
+        let source = self
+            .source
+            .evaluate_as_graph_node(exec)
+            .with_context(|| "Evaluating edge source".to_string().into())?;
+        let sink = self
+            .sink
+            .evaluate_as_graph_node(exec)
+            .with_context(|| "Evaluating edge sink".to_string().into())?;
         for attribute in &self.attributes {
             let value = attribute.value.evaluate(exec)?;
             let edge = match exec.graph[source].get_edge_mut(sink) {
···
                 GraphElementKey::EdgeAttribute(source, sink, attribute.name.clone()),
                 self.debug_info.clone(),
             );
-            edge.attributes
-                .add(attribute.name.clone(), value)
-                .map_err(|_| {
-                    ExecutionError::DuplicateAttribute(format!(
-                        "{} on edge ({} -> {}) at {} and {}",
-                        attribute.name,
-                        source,
-                        sink,
-                        prev_debug_info.unwrap(),
-                        self.debug_info,
-                    ))
-                })?;
+            if let Err(_) = edge.attributes.add(attribute.name.clone(), value) {
+                return Err(ExecutionError::DuplicateAttribute(format!(
+                    "{} on edge ({} -> {})",
+                    attribute.name, source, sink,
+                )))
+                .with_context(|| {
+                    (
+                        prev_debug_info.unwrap().into(),
+                        self.debug_info.clone().into(),
+                    )
+                        .into()
+                });
+            }
         }
         Ok(())
     }

src/execution/lazy/values.rs

···
 use std::fmt;
 
 use crate::execution::error::ExecutionError;
+use crate::execution::error::ResultWithExecutionError;
 use crate::graph::GraphNodeRef;
 use crate::graph::SyntaxNodeRef;
 use crate::graph::Value;
···
     }
 
     fn resolve<'a>(&self, exec: &'a mut EvaluationContext) -> Result<LazyValue, ExecutionError> {
-        let scope = self.scope.as_ref().evaluate_as_syntax_node(exec)?;
+        let scope = self
+            .scope
+            .as_ref()
+            .evaluate_as_syntax_node(exec)
+            .with_context(|| format!("Evaluating scope of variable _.{}", self.name).into())?;
         let scoped_store = &exec.scoped_store;
         scoped_store.evaluate(&scope, &self.name, exec)
     }

tests/it/lazy_execution.rs

···
         "#},
         indoc! {r#"
           node 0
-            name: "alpha"
-          edge 0 -> 2
+          edge 0 -> 1
           node 1
-          edge 1 -> 0
+            name: "alpha"
+          edge 1 -> 2
           node 2
             name: "beta"
           edge 2 -> 3