Config: improve duplicate property check (#560)

libs/common/src/workspace/path.rs

@@ -78,7 +78,10 @@ impl WorkspacePath {
     /// # Errors
     /// [`Error::Vfs`] if the path could not be read
     pub fn read_to_string(&self) -> Result<String, Error> {
-        self.path.read_to_string().map_err(Into::into)
+        self.path
+            .read_to_string()
+            .map(|s| s.replace('\r', ""))
+            .map_err(Into::into)
     }
 
     /// Open the file for reading

libs/config/src/analyze/class.rs

@@ -1,10 +1,8 @@
-use std::collections::HashMap;
-
 use hemtt_common::reporting::{Code, Processed};
 
-use crate::{Class, Ident, Property};
+use crate::Class;
 
-use super::{codes::ce3_duplicate_property::DuplicateProperty, Analyze};
+use super::Analyze;
 
 impl Analyze for Class {
     fn valid(&self) -> bool {
@@ -27,81 +25,10 @@ impl Analyze for Class {
     fn errors(&self, processed: &Processed) -> Vec<Box<dyn Code>> {
         match self {
             Self::External { .. } => vec![],
-            Self::Local { properties, .. } => {
-                let mut errors = properties
-                    .iter()
-                    .flat_map(|p| p.errors(processed))
-                    .collect::<Vec<_>>();
-                errors.extend(self.duplicate_properties());
-                errors
-            }
-        }
-    }
-}
-
-impl Class {
-    fn duplicate_properties(&self) -> Vec<Box<dyn Code>> {
-        match self {
-            Self::External { .. } => vec![],
-            Self::Local { properties, .. } => {
-                let mut errors: Vec<Box<dyn Code>> = Vec::new();
-                let mut seen: HashMap<Ident, &Property> = HashMap::new();
-                let mut conflicts = HashMap::new();
-                for property in properties {
-                    if matches!(
-                        property,
-                        Property::Delete(_) | Property::Class(Self::External { .. })
-                    ) {
-                        continue;
-                    }
-                    if let Some(b) = seen.iter().find(|(b, _)| b.value == property.name().value) {
-                        if let Property::Class(a) = property {
-                            if let Property::Class(ib) = b.1 {
-                                errors.extend(a.duplicate_inner(ib));
-                                continue;
-                            }
-                        }
-                        conflicts
-                            .entry(b.0.as_str().to_string())
-                            .or_insert_with(|| vec![b.0.clone()])
-                            .push(property.name().clone());
-                        continue;
-                    }
-                    seen.insert(property.name().clone(), property);
-                }
-                for (_, conflict) in conflicts {
-                    errors.push(Box::new(DuplicateProperty::new(conflict)));
-                }
-                errors
-            }
-        }
-    }
-
-    fn duplicate_inner(&self, other: &Self) -> Vec<Box<dyn Code>> {
-        let Self::Local { properties: a, .. } = self else {
-            return vec![];
-        };
-        let Self::Local { properties: b, .. } = other else {
-            return vec![];
-        };
-
-        let mut errors: Vec<Box<dyn Code>> = Vec::new();
-        for a in a {
-            if let Property::Class(a) = a {
-                if let Some(Property::Class(b)) =
-                    b.iter().find(|b| b.name().as_str() == a.name().as_str())
-                {
-                    errors.extend(a.duplicate_inner(b));
-                    continue;
-                }
-            }
-            if let Some(b) = b.iter().find(|b| b.name().as_str() == a.name().as_str()) {
-                errors.push(Box::new(DuplicateProperty::new(vec![
-                    b.name().clone(),
-                    a.name().clone(),
-                ])));
-            }
+            Self::Local { properties, .. } => properties
+                .iter()
+                .flat_map(|p| p.errors(processed))
+                .collect::<Vec<_>>(),
         }
-        errors
     }
 }

libs/config/src/analyze/config.rs

@@ -2,10 +2,13 @@ use std::collections::{HashMap, HashSet};
 
 use hemtt_common::reporting::{Code, Processed};
 
-use crate::{Class, Config, Property};
+use crate::{Class, Config, Ident, Property};
 
 use super::{
-    codes::{ce7_missing_parent::MissingParent, cw1_parent_case::ParentCase},
+    codes::{
+        ce3_duplicate_property::DuplicateProperty, ce7_missing_parent::MissingParent,
+        cw1_parent_case::ParentCase,
+    },
     Analyze,
 };
 
@@ -21,7 +24,7 @@ impl Analyze for Config {
             .flat_map(|p| p.warnings(processed))
             .collect::<Vec<_>>();
         let mut defined = HashMap::new();
-        warnings.extend(external_missing_warn(&self.0, &mut defined));
+        warnings.extend(external_parent_case_warn(&self.0, &mut defined));
         warnings
     }
 
@@ -33,6 +36,7 @@ impl Analyze for Config {
             .collect::<Vec<_>>();
         let mut defined = HashSet::new();
         errors.extend(external_missing_error(&self.0, &mut defined));
+        errors.extend(duplicate_properties(&self.0));
         errors
     }
 }
@@ -72,7 +76,7 @@ fn external_missing_error(
     errors
 }
 
-fn external_missing_warn(
+fn external_parent_case_warn(
     properties: &[Property],
     defined: &mut HashMap<String, Class>,
 ) -> Vec<Box<dyn Code>> {
@@ -106,10 +110,55 @@ fn external_missing_warn(
                         }
                     }
                     defined.insert(name_lower, c.clone());
-                    warnings.extend(external_missing_warn(properties, defined));
+                    warnings.extend(external_parent_case_warn(properties, defined));
                 }
             }
         }
     }
     warnings
 }
+
+fn duplicate_properties(properties: &[Property]) -> Vec<Box<dyn Code>> {
+    let mut seen: HashMap<String, Vec<(bool, Ident)>> = HashMap::new();
+    duplicate_properties_inner("", properties, &mut seen);
+    let mut errors: Vec<Box<dyn Code>> = Vec::new();
+    for (_, idents) in seen {
+        if idents.len() > 1 && !idents.iter().all(|(class, _)| *class) {
+            errors.push(Box::new(DuplicateProperty::new(
+                idents.into_iter().map(|(_, i)| i).collect(),
+            )));
+        }
+    }
+    errors
+}
+
+fn duplicate_properties_inner(
+    scope: &str,
+    properties: &[Property],
+    seen: &mut HashMap<String, Vec<(bool, Ident)>>,
+) {
+    for property in properties {
+        match property {
+            Property::Class(Class::Local {
+                name, properties, ..
+            }) => {
+                duplicate_properties_inner(
+                    &format!("{}.{}", scope, name.value.to_lowercase()),
+                    properties,
+                    seen,
+                );
+                let entry = seen
+                    .entry(format!("{}.{}", scope, name.value.to_lowercase()))
+                    .or_default();
+                entry.push((true, name.clone()));
+            }
+            Property::Entry { name, .. } | Property::MissingSemicolon(name, _) => {
+                let entry = seen
+                    .entry(format!("{}.{}", scope, name.value.to_lowercase()))
+                    .or_default();
+                entry.push((false, name.clone()));
+            }
+            _ => (),
+        }
+    }
+}

libs/config/tests/errors.rs

@@ -44,7 +44,7 @@ fn check(dir: &str) {
                 .unwrap();
             }
             assert_eq!(
-                errors.join("\n").replace('\r', "").replace(r"\u{1b}[38;5;201m─\u{1b}[0m\u{1b}[38;5;201m┬\u{1b}[0m  \n \u{1b}[38;5;240m  │\u{1b}[0m ", r"\u{1b}[38;5;201m┬\u{1b}[0m  \n \u{1b}[38;5;240m  │\u{1b}[0m"),
+                errors.join("\n").replace('\r', ""),
                 String::from_utf8(expected).unwrap().replace('\r', "")
             );
         }
@@ -55,6 +55,11 @@ fn check(dir: &str) {
     }
 }
 
-bootstrap!(simple);
-bootstrap!(arrays);
+bootstrap!(ce1_invalid_value);
+bootstrap!(ce2_invalid_value_macro);
+bootstrap!(ce3_duplicate_property_separate);
+bootstrap!(ce3_duplicate_property_shadow_property);
+bootstrap!(ce4_missing_semicolon);
+bootstrap!(ce5_unexpected_array);
+bootstrap!(ce6_expected_array);
 bootstrap!(ce7_missing_parent);

libs/config/tests/errors/ce1_invalid_value/source.hpp

@@ -0,0 +1,3 @@
+class Test {
+    outer = something;
+};

libs/config/tests/errors/ce1_invalid_value/stdout.ansi

@@ -0,0 +1,9 @@
+[CE1] Error: property's value could not be parsed.
+   ╭─[/source.hpp:2:13]
+   │
+ 2 │     outer = something;
+   │             ────┬────  
+   │                 ╰────── invalid value
+   │ 
+   │ Help: use quotes `"` around the value, or a QUOTE macro if it contains #define values
+───╯

libs/config/tests/errors/ce2_invalid_value_macro/source.hpp

@@ -0,0 +1,8 @@
+#define QUOTE(x) #x
+#define PATHTO(x) \some\x
+#define QPATHTO(x) QUOTE(PATHTO(x))
+
+path = PATHTO(thing);
+class Test {
+    path = PATHTO(thing);
+};

libs/config/tests/errors/ce2_invalid_value_macro/stdout.ansi

@@ -0,0 +1,23 @@
+[CE2] Error: macro's result could not be parsed
+   ╭─[/source.hpp:5:8]
+   │
+ 5 │ path = PATHTO(thing);
+   │        ───┬──  
+   │           ╰──── invalid macro result
+   │ 
+   │ Help: perhaps this macro has a `Q_` variant or you need `QUOTE(..)`
+   │ 
+   │ Note: The processed output was `\some\thing`
+───╯
+
+[CE2] Error: macro's result could not be parsed
+   ╭─[/source.hpp:7:12]
+   │
+ 7 │     path = PATHTO(thing);
+   │            ───┬──  
+   │               ╰──── invalid macro result
+   │ 
+   │ Help: perhaps this macro has a `Q_` variant or you need `QUOTE(..)`
+   │ 
+   │ Note: The processed output was `\some\thing`
+───╯

libs/config/tests/errors/ce3_duplicate_property_separate/source.hpp

@@ -0,0 +1,8 @@
+class Test {
+    class Child {
+        inner = "something";
+    };
+    class Child {
+        inner = "something";
+    };
+};

libs/config/tests/errors/ce3_duplicate_property_separate/stdout.ansi

@@ -0,0 +1,11 @@
+[CE3] Error: property was defined more than once
+   ╭─[/source.hpp:3:9]
+   │
+ 3 │         inner = "something";
+   │         ──┬──  
+   │           ╰──── first defined here
+   │ 
+ 6 │         inner = "something";
+   │         ──┬──  
+   │           ╰──── also defined here
+───╯

libs/config/tests/errors/ce3_duplicate_property_shadow_property/source.hpp

@@ -0,0 +1,6 @@
+class Test {
+    thing = 1;
+    class thing {
+        value = 1;
+    };
+};

libs/config/tests/errors/ce3_duplicate_property_shadow_property/stdout.ansi

@@ -0,0 +1,10 @@
+[CE3] Error: property was defined more than once
+   ╭─[/source.hpp:2:5]
+   │
+ 2 │     thing = 1;
+   │     ──┬──  
+   │       ╰──── first defined here
+ 3 │     class thing {
+   │           ──┬──  
+   │             ╰──── also defined here
+───╯

libs/config/tests/errors/ce4_missing_semicolon/source.hpp

@@ -0,0 +1,4 @@
+outer = "nosemi"
+class Test {
+    inner = "nosemi"
+};

libs/config/tests/errors/ce4_missing_semicolon/stdout.ansi

@@ -0,0 +1,19 @@
+[CE4] Error: property is missing a semicolon
+   ╭─[/source.hpp:1:17]
+   │
+ 1 │ outer = "nosemi"
+   │                 ┬  
+   │                 ╰── missing semicolon
+   │ 
+   │ Help: add a semicolon `;` to the end of the property
+───╯
+
+[CE4] Error: property is missing a semicolon
+   ╭─[/source.hpp:3:21]
+   │
+ 3 │     inner = "nosemi"
+   │                     ┬  
+   │                     ╰── missing semicolon
+   │ 
+   │ Help: add a semicolon `;` to the end of the property
+───╯

libs/config/tests/errors/ce5_unexpected_array/source.hpp

@@ -0,0 +1 @@
+array = {1,2,3};