fix: improve jinja err msg (prefix-dev#363)

src/used_variables.rs

@@ -16,7 +16,10 @@ use minijinja::machinery::{
     parse,
 };
 
-use crate::recipe::custom_yaml::Node;
+use crate::recipe::{
+    custom_yaml::{HasSpan, Node, ScalarNode},
+    ParsingError,
+};
 
 /// Extract all variables from a jinja statement
 fn extract_variables(node: &Stmt, variables: &mut HashSet<String>) {
@@ -83,7 +86,7 @@ fn extract_variable_from_expression(expr: &Expr, variables: &mut HashSet<String>
 }
 
 /// This recursively finds all `if/then/else` expressions in a YAML node
-fn find_all_selectors(node: &Node, selectors: &mut HashSet<String>) {
+fn find_all_selectors<'a>(node: &'a Node, selectors: &mut HashSet<&'a ScalarNode>) {
     use crate::recipe::custom_yaml::SequenceNodeInternal;
 
     match node {
@@ -97,7 +100,7 @@ fn find_all_selectors(node: &Node, selectors: &mut HashSet<String>) {
                 match item {
                     SequenceNodeInternal::Simple(node) => find_all_selectors(node, selectors),
                     SequenceNodeInternal::Conditional(if_sel) => {
-                        selectors.insert(if_sel.cond().as_str().to_owned());
+                        selectors.insert(if_sel.cond());
                         find_all_selectors(if_sel.then(), selectors);
                         if let Some(otherwise) = if_sel.otherwise() {
                             find_all_selectors(otherwise, selectors);
@@ -111,36 +114,54 @@ fn find_all_selectors(node: &Node, selectors: &mut HashSet<String>) {
 }
 
 // find all scalar nodes and Jinja expressions
-fn find_jinja(node: &Node, variables: &mut HashSet<String>) -> Result<(), minijinja::Error> {
+fn find_jinja(node: &Node, src: &str, variables: &mut HashSet<String>) -> Result<(), ParsingError> {
     use crate::recipe::custom_yaml::SequenceNodeInternal;
 
     match node {
         Node::Mapping(map) => {
             for (_, value) in map.iter() {
-                find_jinja(value, variables)?;
+                find_jinja(value, src, variables)?;
             }
         }
         Node::Sequence(seq) => {
             for item in seq.iter() {
                 match item {
-                    SequenceNodeInternal::Simple(node) => find_jinja(node, variables)?,
+                    SequenceNodeInternal::Simple(node) => find_jinja(node, src, variables)?,
                     SequenceNodeInternal::Conditional(if_sel) => {
                         // we need to convert the if condition to a Jinja expression to parse it
                         let as_jinja_expr = format!("${{{{ {} }}}}", if_sel.cond().as_str());
-                        let ast = parse(&as_jinja_expr, "jinja.yaml")?;
+                        let ast = parse(&as_jinja_expr, "jinja.yaml").map_err(|e| {
+                            crate::recipe::ParsingError::from_partial(
+                                src,
+                                crate::_partialerror!(
+                                    *if_sel.span(),
+                                    crate::recipe::error::ErrorKind::from(e),
+                                    label = "failed to parse as jinja expression"
+                                ),
+                            )
+                        })?;
                         extract_variables(&ast, variables);
 
-                        find_jinja(if_sel.then(), variables)?;
+                        find_jinja(if_sel.then(), src, variables)?;
                         if let Some(otherwise) = if_sel.otherwise() {
-                            find_jinja(otherwise, variables)?;
+                            find_jinja(otherwise, src, variables)?;
                         }
                     }
                 }
             }
         }
         Node::Scalar(scalar) => {
             if scalar.contains("${{") {
-                let ast = parse(scalar, "jinja.yaml")?;
+                let ast = parse(scalar, "jinja.yaml").map_err(|e| {
+                    crate::recipe::ParsingError::from_partial(
+                        src,
+                        crate::_partialerror!(
+                            *scalar.span(),
+                            crate::recipe::error::ErrorKind::from(e),
+                            label = "failed to parse as jinja expression"
+                        ),
+                    )
+                })?;
                 extract_variables(&ast, variables);
             }
         }
@@ -153,21 +174,31 @@ fn find_jinja(node: &Node, variables: &mut HashSet<String>) -> Result<(), miniji
 /// This finds all variables used in jinja or `if/then/else` expressions
 pub(crate) fn used_vars_from_expressions(
     yaml_node: &Node,
-) -> Result<HashSet<String>, minijinja::Error> {
+    src: &str,
+) -> Result<HashSet<String>, ParsingError> {
     let mut selectors = HashSet::new();
 
     find_all_selectors(yaml_node, &mut selectors);
 
     let mut variables = HashSet::new();
 
     for selector in selectors {
-        let selector_tmpl = format!("{{{{ {} }}}}", selector);
-        let ast = parse(&selector_tmpl, "selector.yaml")?;
+        let selector_tmpl = format!("{{{{ {} }}}}", selector.as_str());
+        let ast = parse(&selector_tmpl, "selector.yaml").map_err(|e| -> ParsingError {
+            crate::recipe::ParsingError::from_partial(
+                src,
+                crate::_partialerror!(
+                    *selector.span(),
+                    crate::recipe::error::ErrorKind::from(e),
+                    label = "failed to parse as jinja expression"
+                ),
+            )
+        })?;
         extract_variables(&ast, &mut variables);
     }
 
     // parse recipe into AST
-    find_jinja(yaml_node, &mut variables)?;
+    find_jinja(yaml_node, src, &mut variables)?;
 
     Ok(variables)
 }
@@ -189,8 +220,8 @@ mod test {
             - ${{ pin_subpackage('abcdef') }}
         "#;
 
-        let recipe = crate::recipe::custom_yaml::Node::parse_yaml(0, recipe).unwrap();
-        let used_vars = used_vars_from_expressions(&recipe).unwrap();
+        let recipe_node = crate::recipe::custom_yaml::Node::parse_yaml(0, recipe).unwrap();
+        let used_vars = used_vars_from_expressions(&recipe_node, recipe).unwrap();
         assert!(used_vars.contains("llvm_variant"));
         assert!(used_vars.contains("linux"));
         assert!(used_vars.contains("osx"));

src/variant_config.rs

@@ -380,7 +380,7 @@ impl VariantConfig {
         for output in outputs.iter() {
             // for the topological sort we only take into account `pin_subpackage` expressions
             // in the recipe which are captured by the `used vars`
-            let used_vars = used_vars_from_expressions(output)?;
+            let used_vars = used_vars_from_expressions(output, recipe)?;
             let parsed_recipe = Recipe::from_node(output, selector_config.clone())
                 .map_err(|err| ParsingError::from_partial(recipe, err))?;
             let noarch_type = parsed_recipe.build().noarch();
@@ -755,9 +755,6 @@ pub enum VariantError {
 
     #[error("Found a cycle in the recipe outputs: {0}")]
     CycleInRecipeOutputs(String),
-
-    #[error("Could not parse a Jinja expression: {0}")]
-    JinjaParseError(#[from] minijinja::Error),
 }
 
 fn find_combinations(