KCL: Fix cryptic error when using duplicate edges in fillet call (#5755)

Fixes #4307

Now if you try to fillet the same edge twice in a single fillet command,
the error message is clearer, and the source range will highlight
the specific edges in the array which are duplicated.

Same goes for chamfer.

Note: although the Rust KCL interpreter sends back an array of SourceRange
for each KCL error, the frontend only puts the first one into CodeMirror
diagnostics. We should fix that: #5754

Author: adamchalmers

rust/kcl-lib/e2e/executor/inputs/fillet_duplicate_tags.kcl

@@ -1,8 +1,15 @@
-const part001 = startSketchOn('XY')
-    |> startProfileAt([0,0], %)
-    |> line(end = [0, 10], tag = $thing)
-    |> line(end = [10, 0])
-    |> line(end = [0, -10], tag = $thing2)
-    |> close()
-    |> extrude(length = 10)
-    |> fillet(radius = 0.5, tags = [thing, thing])
+startProfileAt([0, 0], startSketchOn("XY"))
+  |> xLine(length = 10, tag = $line000)
+  |> yLine(length = 10, tag = $line001)
+  |> xLine(endAbsolute = profileStartX(%), tag = $line002)
+  |> close(tag = $line003)
+  |> extrude(length = 10)
+  |> fillet(
+       radius = 1,
+       tags = [
+         line003,
+         getNextAdjacentEdge(line000),
+         getPreviousAdjacentEdge(line001)
+       ],
+     )
+

rust/kcl-lib/e2e/executor/main.rs

@@ -27,11 +27,14 @@ async fn kcl_test_fillet_duplicate_tags() {
     let code = kcl_input!("fillet_duplicate_tags");
 
     let result = execute_and_snapshot(code, UnitLength::Mm, None).await;
-    assert!(result.is_err());
+    let err = result.expect_err("Code should have failed due to the duplicate edges being filletted");
+
+    let err = err.as_kcl_error().unwrap();
     assert_eq!(
-        result.err().unwrap().to_string(),
-        r#"type: KclErrorDetails { source_ranges: [SourceRange([229, 272, 0])], message: "Duplicate tags are not allowed." }"#,
+        err.message(),
+        "The same edge ID is being referenced multiple times, which is not allowed. Please select a different edge"
     );
+    assert_eq!(err.source_ranges().len(), 2);
 }
 
 #[tokio::test(flavor = "multi_thread")]

rust/kcl-lib/src/errors.rs

@@ -48,6 +48,15 @@ impl ExecErrorWithState {
     }
 }
 
+impl ExecError {
+    pub fn as_kcl_error(&self) -> Option<&crate::KclError> {
+        let ExecError::Kcl(k) = &self else {
+            return None;
+        };
+        Some(&k.error)
+    }
+}
+
 impl From<ExecError> for ExecErrorWithState {
     fn from(error: ExecError) -> Self {
         Self {

rust/kcl-lib/src/std/args.rs

@@ -159,6 +159,49 @@ impl Args {
         })
     }
 
+    /// Get a labelled keyword arg, check it's an array, and return all items in the array
+    /// plus their source range.
+    pub(crate) fn kw_arg_array_and_source<'a, T>(&'a self, label: &str) -> Result<Vec<(T, SourceRange)>, KclError>
+    where
+        T: FromKclValue<'a>,
+    {
+        let Some(arg) = self.kw_args.labeled.get(label) else {
+            let err = KclError::Semantic(KclErrorDetails {
+                source_ranges: vec![self.source_range],
+                message: format!("This function requires a keyword argument '{label}'"),
+            });
+            return Err(err);
+        };
+        let Some(array) = arg.value.as_array() else {
+            let err = KclError::Semantic(KclErrorDetails {
+                source_ranges: vec![arg.source_range],
+                message: format!(
+                    "Expected an array of {} but found {}",
+                    type_name::<T>(),
+                    arg.value.human_friendly_type()
+                ),
+            });
+            return Err(err);
+        };
+        array
+            .iter()
+            .map(|item| {
+                let source = SourceRange::from(item);
+                let val = FromKclValue::from_kcl_val(item).ok_or_else(|| {
+                    KclError::Semantic(KclErrorDetails {
+                        source_ranges: arg.source_ranges(),
+                        message: format!(
+                            "Expected a {} but found {}",
+                            type_name::<T>(),
+                            arg.value.human_friendly_type()
+                        ),
+                    })
+                })?;
+                Ok((val, source))
+            })
+            .collect::<Result<Vec<_>, _>>()
+    }
+
     /// Get the unlabeled keyword argument. If not set, returns None.
     pub(crate) fn unlabeled_kw_arg_unconverted(&self) -> Option<&Arg> {
         self.kw_args

rust/kcl-lib/src/std/chamfer.rs

@@ -5,7 +5,6 @@ use kcl_derive_docs::stdlib;
 use kcmc::{each_cmd as mcmd, length_unit::LengthUnit, shared::CutType, ModelingCmd};
 use kittycad_modeling_cmds as kcmc;
 
-use super::utils::unique_count;
 use crate::{
     errors::{KclError, KclErrorDetails},
     execution::{ChamferSurface, EdgeCut, ExecState, ExtrudeSurface, GeoMeta, KclValue, Solid},
@@ -19,9 +18,11 @@ pub(crate) const DEFAULT_TOLERANCE: f64 = 0.0000001;
 pub async fn chamfer(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
     let solid = args.get_unlabeled_kw_arg("solid")?;
     let length = args.get_kw_arg("length")?;
-    let tags = args.get_kw_arg("tags")?;
+    let tags = args.kw_arg_array_and_source::<EdgeReference>("tags")?;
     let tag = args.get_kw_arg_opt("tag")?;
 
+    super::fillet::validate_unique(&tags)?;
+    let tags: Vec<EdgeReference> = tags.into_iter().map(|item| item.0).collect();
     let value = inner_chamfer(solid, length, tags, tag, exec_state, args).await?;
     Ok(KclValue::Solid { value })
 }
@@ -109,15 +110,6 @@ async fn inner_chamfer(
     exec_state: &mut ExecState,
     args: Args,
 ) -> Result<Box<Solid>, KclError> {
-    // Check if tags contains any duplicate values.
-    let unique_tags = unique_count(tags.clone());
-    if unique_tags != tags.len() {
-        return Err(KclError::Type(KclErrorDetails {
-            message: "Duplicate tags are not allowed.".to_string(),
-            source_ranges: vec![args.source_range],
-        }));
-    }
-
     // If you try and tag multiple edges with a tagged chamfer, we want to return an
     // error to the user that they can only tag one edge at a time.
     if tag.is_some() && tags.len() > 1 {

rust/kcl-lib/src/std/fillet.rs

@@ -1,6 +1,7 @@
 //! Standard library fillets.
 
 use anyhow::Result;
+use indexmap::IndexMap;
 use kcl_derive_docs::stdlib;
 use kcmc::{
     each_cmd as mcmd, length_unit::LengthUnit, ok_response::OkModelingCmdResponse, shared::CutType,
@@ -11,13 +12,13 @@ use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 use uuid::Uuid;
 
-use super::utils::unique_count;
 use crate::{
     errors::{KclError, KclErrorDetails},
     execution::{EdgeCut, ExecState, ExtrudeSurface, FilletSurface, GeoMeta, KclValue, Solid, TagIdentifier},
     parsing::ast::types::TagNode,
     settings::types::UnitLength,
     std::Args,
+    SourceRange,
 };
 
 /// A tag or a uuid of an edge.
@@ -40,13 +41,39 @@ impl EdgeReference {
     }
 }
 
+pub(super) fn validate_unique<T: Eq + std::hash::Hash>(tags: &[(T, SourceRange)]) -> Result<(), KclError> {
+    // Check if tags contains any duplicate values.
+    let mut tag_counts: IndexMap<&T, Vec<SourceRange>> = Default::default();
+    for tag in tags {
+        tag_counts.entry(&tag.0).or_insert(Vec::new()).push(tag.1);
+    }
+    let mut duplicate_tags_source = Vec::new();
+    for (_tag, count) in tag_counts {
+        if count.len() > 1 {
+            duplicate_tags_source.extend(count)
+        }
+    }
+    if !duplicate_tags_source.is_empty() {
+        return Err(KclError::Type(KclErrorDetails {
+            message: "The same edge ID is being referenced multiple times, which is not allowed. Please select a different edge".to_string(),
+            source_ranges: duplicate_tags_source,
+        }));
+    }
+    Ok(())
+}
+
 /// Create fillets on tagged paths.
 pub async fn fillet(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
+    // Get all args:
     let solid = args.get_unlabeled_kw_arg("solid")?;
     let radius = args.get_kw_arg("radius")?;
     let tolerance = args.get_kw_arg_opt("tolerance")?;
-    let tags = args.get_kw_arg("tags")?;
+    let tags = args.kw_arg_array_and_source::<EdgeReference>("tags")?;
     let tag = args.get_kw_arg_opt("tag")?;
+
+    // Run the function.
+    validate_unique(&tags)?;
+    let tags: Vec<EdgeReference> = tags.into_iter().map(|item| item.0).collect();
     let value = inner_fillet(solid, radius, tags, tolerance, tag, exec_state, args).await?;
     Ok(KclValue::Solid { value })
 }
@@ -129,15 +156,6 @@ async fn inner_fillet(
     exec_state: &mut ExecState,
     args: Args,
 ) -> Result<Box<Solid>, KclError> {
-    // Check if tags contains any duplicate values.
-    let unique_tags = unique_count(tags.clone());
-    if unique_tags != tags.len() {
-        return Err(KclError::Type(KclErrorDetails {
-            message: "Duplicate tags are not allowed.".to_string(),
-            source_ranges: vec![args.source_range],
-        }));
-    }
-
     let mut solid = solid.clone();
     for edge_tag in tags {
         let edge_id = edge_tag.get_engine_id(exec_state, &args)?;
@@ -432,3 +450,22 @@ pub(crate) fn default_tolerance(units: &UnitLength) -> f64 {
         UnitLength::M => 0.001,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_validate_unique() {
+        let dup_a = SourceRange::from([1, 3, 0]);
+        let dup_b = SourceRange::from([10, 30, 0]);
+        // Two entries are duplicates (abc) with different source ranges.
+        let tags = vec![("abc", dup_a), ("abc", dup_b), ("def", SourceRange::from([2, 4, 0]))];
+        let actual = validate_unique(&tags);
+        // Both the duplicates should show up as errors, with both of the
+        // source ranges they correspond to.
+        // But the unique source range 'def' should not.
+        let expected = vec![dup_a, dup_b];
+        assert_eq!(actual.err().unwrap().source_ranges(), expected);
+    }
+}

rust/kcl-lib/src/std/utils.rs

@@ -1,4 +1,4 @@
-use std::{collections::HashSet, f64::consts::PI};
+use std::f64::consts::PI;
 
 use kittycad_modeling_cmds::shared::Angle;
 
@@ -8,16 +8,6 @@ use crate::{
     source_range::SourceRange,
 };
 
-/// Count the number of unique items in a `Vec` in O(n) time.
-pub(crate) fn unique_count<T: Eq + std::hash::Hash>(vec: Vec<T>) -> usize {
-    // Add to a set.
-    let mut set = HashSet::with_capacity(vec.len());
-    for item in vec {
-        set.insert(item);
-    }
-    set.len()
-}
-
 /// Get the distance between two points.
 pub fn distance(a: Point2d, b: Point2d) -> f64 {
     ((b.x - a.x).powi(2) + (b.y - a.y).powi(2)).sqrt()
@@ -686,11 +676,6 @@ mod get_tangential_arc_to_info_tests {
         (num * 1000.0).round() / 1000.0
     }
 
-    #[test]
-    fn test_unique_count() {
-        assert_eq!(unique_count(vec![1, 2, 2, 3, 2]), 3);
-    }
-
     #[test]
     fn test_basic_case() {
         let result = get_tangential_arc_to_info(TangentialArcInfoInput {