fix(scaffold): error when a duplicated condition is found

src/scaffold/modifiers.rs

@@ -16,8 +16,7 @@ use crate::utils::{lower_first_letter, to_pascal_case};
 /// `when only owner` is converted to the `whenOnlyOwner` modifier.
 ///
 /// For ease of retrieval, the discovered modifiers are stored in a `IndexMap`
-/// for the later phases of the compiler. Note that this means that
-/// we assume that duplicate titles translate to the same modifier.
+/// for the later phases of the compiler.
 /// `IndexMap` was chosen since preserving the order of insertion
 /// to match the order of the modifiers in the source tree is helpful
 /// and the performance trade-off is negligible.

src/syntax/semantics.rs

@@ -1,10 +1,12 @@
 //! Implementation of the semantic analysis of a bulloak tree.
 
+use std::collections::HashMap;
 use std::{fmt, result};
 
 use super::ast::{self, Ast};
 use crate::span::Span;
 use crate::syntax::visitor::Visitor;
+use crate::utils::{lower_first_letter, to_pascal_case};
 
 type Result<T> = result::Result<T, Vec<Error>>;
 
@@ -51,13 +53,15 @@ impl std::error::Error for Error {}
 /// The type of an error that occurred while building an AST.
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub enum ErrorKind {
+    /// Found two conditions with the same title.
+    ConditionDuplicated(Vec<Span>),
+    /// Found a condition with no children.
+    ConditionEmpty,
     /// Found an unexpected node. This is most probably a bug in the
     /// parser implementation.
     NodeUnexpected,
     /// Found no rules to emit.
     TreeEmpty,
-    /// Found a condition with no children.
-    ConditionEmpty,
     /// This enum may grow additional variants, so this makes sure clients
     /// don't count on exhaustive matching. (Otherwise, adding a new variant
     /// could break existing code.)
@@ -73,11 +77,19 @@ impl fmt::Display for Error {
 
 impl fmt::Display for ErrorKind {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        use self::ErrorKind::{ConditionEmpty, NodeUnexpected, TreeEmpty};
+        use self::ErrorKind::{ConditionDuplicated, ConditionEmpty, NodeUnexpected, TreeEmpty};
         match *self {
+            ConditionDuplicated(ref spans) => {
+                let lines = spans
+                    .iter()
+                    .map(|s| s.start.line.to_string())
+                    .collect::<Vec<String>>()
+                    .join(", ");
+                write!(f, "found a condition more than once in lines: {}", lines)
+            }
+            ConditionEmpty => write!(f, "found a condition with no children"),
             NodeUnexpected => write!(f, "unexpected child node"),
             TreeEmpty => write!(f, "no rules where defined"),
-            ConditionEmpty => write!(f, "found a condition with no children"),
             _ => unreachable!(),
         }
     }
@@ -90,15 +102,18 @@ pub struct SemanticAnalyzer<'t> {
     /// The original text that the visitor generated the errors from. Every
     /// span in an error is a valid range into this string.
     text: &'t str,
+    /// A map from modifier name to it's locations in the input.
+    modifiers: HashMap<String, Vec<Span>>,
 }
 
 impl<'t> SemanticAnalyzer<'t> {
     /// Create a new semantic analyzer.
     #[must_use]
     pub fn new(text: &'t str) -> SemanticAnalyzer {
         SemanticAnalyzer {
-            errors: Vec::new(),
             text,
+            errors: Vec::new(),
+            modifiers: HashMap::new(),
         }
     }
 
@@ -126,6 +141,19 @@ impl<'t> SemanticAnalyzer<'t> {
         // be stored in `self.errors`.
         .unwrap();
 
+        // Check for duplicate conditions.
+        for spans in self.modifiers.clone().into_values() {
+            if spans.len() > 1 {
+                self.error(
+                    // FIXME: This is a patch until we start storing locations for
+                    // parts of an AST node. In this case, we need the location of
+                    // the condition's title.
+                    spans[0].with_end(spans[0].start),
+                    ErrorKind::ConditionDuplicated(spans),
+                )
+            }
+        }
+
         if !self.errors.is_empty() {
             return Err(self.errors.clone());
         }
@@ -169,6 +197,14 @@ impl Visitor for SemanticAnalyzer<'_> {
             self.error(condition.span, ErrorKind::ConditionEmpty);
         }
 
+        let modifier = lower_first_letter(&to_pascal_case(&condition.title));
+        match self.modifiers.get_mut(&modifier) {
+            Some(spans) => spans.push(condition.span),
+            None => {
+                self.modifiers.insert(modifier, vec![condition.span]);
+            }
+        }
+
         for ast in &condition.children {
             match ast {
                 Ast::Condition(condition) => {
@@ -220,7 +256,7 @@ mod tests {
     }
 
     #[test]
-    fn test_unexpected_node() {
+    fn unexpected_node() {
         let ast = ast::Ast::Root(ast::Root {
             contract_name: "Foo_Test".to_owned(),
             children: vec![ast::Ast::Root(ast::Root {
@@ -244,7 +280,34 @@ mod tests {
     }
 
     #[test]
-    fn test_condition_empty() {
+    fn duplicated_condition() {
+        assert_eq!(
+            analyze(
+            "Foo_Test
+├── when dup
+│   └── It 1
+├── when dup
+│   └── It 2
+└── when dup
+    └── It 3",
+        )
+        .unwrap_err(),
+            vec![semantics::Error {
+                kind: ConditionDuplicated(
+                    vec![
+                        Span::new(Position::new(9, 2, 1), Position::new(47, 3, 12)),
+                        Span::new(Position::new(49, 4, 1), Position::new(87, 5, 12)),
+                        Span::new(Position::new(89, 6, 1), Position::new(125, 7, 12)),
+                    ],
+                ),
+                text: "Foo_Test\n├── when dup\n│   └── It 1\n├── when dup\n│   └── It 2\n└── when dup\n    └── It 3".to_owned(),
+                span: Span::new(Position::new(9, 2, 1), Position::new(9, 2, 1)),
+            }]
+        );
+    }
+
+    #[test]
+    fn condition_empty() {
         assert_eq!(
             analyze("Foo_Test\n└── when something").unwrap_err(),
             vec![semantics::Error {
@@ -256,7 +319,7 @@ mod tests {
     }
 
     #[test]
-    fn test_allow_action_without_conditions() {
+    fn allow_action_without_conditions() {
         assert!(analyze("Foo_Test\n└── it a something").is_ok());
     }
 }

tests/scaffold.rs

@@ -61,3 +61,19 @@ fn errors_when_tree_is_empty() {
         assert!(actual.contains("found an empty tree"));
     }
 }
+
+#[test]
+fn errors_when_condition_appears_multiple_times() {
+    let cwd = env::current_dir().unwrap();
+    let binary_path = get_binary_path();
+    let tests_path = cwd.join("tests").join("scaffold");
+    let trees = ["duplicated_condition.tree"];
+
+    for tree_name in trees {
+        let tree_path = tests_path.join(tree_name);
+        let output = cmd(&binary_path, "scaffold", &tree_path, &[]);
+        let actual = String::from_utf8(output.stderr).unwrap();
+
+        assert!(actual.contains("found a condition more than once"));
+    }
+}

tests/scaffold/basic.tree

@@ -1,6 +1,6 @@
 HashPairTest
 ├── It should never revert.
 ├── When first arg is smaller than second arg
-│       └── It should match the result of `keccak256(abi.encodePacked(a,b))`.
+│   └── It should match the result of `keccak256(abi.encodePacked(a,b))`.
 └── When first arg is bigger than second arg
-        └── It should match the result of `keccak256(abi.encodePacked(b,a))`.
+    └── It should match the result of `keccak256(abi.encodePacked(b,a))`.

tests/scaffold/duplicated_condition.tree

@@ -0,0 +1,7 @@
+HashPairTest
+├── When first arg is smaller than second arg
+│   └── It should match the result of `keccak256(abi.encodePacked(a,b))`.
+├── When first arg is smaller than second arg
+│   └── It should match the result of `keccak256(abi.encodePacked(a,b))`.
+└── When first arg is smaller than second arg
+    └── It should match the result of `keccak256(abi.encodePacked(b,a))`.