Add coercing default units to known units in KCL array range (#9219)

* Add failing test

* Change array range to be more forgiving in terms of units

* Update test output to success

* Update existing test output

Author: jtran

rust/kcl-lib/src/execution/exec_ast.rs

@@ -19,7 +19,6 @@ use crate::{
         state::{ModuleState, SketchBlockState},
         types::{NumericType, PrimitiveType, RuntimeType},
     },
-    fmt,
     modules::{ModuleExecutionOutcome, ModuleId, ModulePath, ModuleRepr},
     parsing::ast::types::{
         Annotation, ArrayExpression, ArrayRangeExpression, AscribedExpression, BinaryExpression, BinaryOperator,
@@ -2072,33 +2071,40 @@ impl Node<ArrayRangeExpression> {
                 StatementKind::Expression,
             )
             .await?;
-        let (start, start_ty) = start_val
-            .as_int_with_ty()
+        let start = start_val
+            .as_ty_f64()
             .ok_or(KclError::new_semantic(KclErrorDetails::new(
-                format!("Expected int but found {}", start_val.human_friendly_type()),
+                format!(
+                    "Expected number for range start but found {}",
+                    start_val.human_friendly_type()
+                ),
                 vec![self.into()],
             )))?;
         let metadata = Metadata::from(&self.end_element);
         let end_val = ctx
             .execute_expr(&self.end_element, exec_state, &metadata, &[], StatementKind::Expression)
             .await?;
-        let (end, end_ty) = end_val
-            .as_int_with_ty()
-            .ok_or(KclError::new_semantic(KclErrorDetails::new(
-                format!("Expected int but found {}", end_val.human_friendly_type()),
-                vec![self.into()],
-            )))?;
+        let end = end_val.as_ty_f64().ok_or(KclError::new_semantic(KclErrorDetails::new(
+            format!(
+                "Expected number for range end but found {}",
+                end_val.human_friendly_type()
+            ),
+            vec![self.into()],
+        )))?;
 
-        if start_ty != end_ty {
-            let start = start_val.as_ty_f64().unwrap_or(TyF64 { n: 0.0, ty: start_ty });
-            let start = fmt::human_display_number(start.n, start.ty);
-            let end = end_val.as_ty_f64().unwrap_or(TyF64 { n: 0.0, ty: end_ty });
-            let end = fmt::human_display_number(end.n, end.ty);
+        let (start, end, ty) = NumericType::combine_range(start, end, exec_state, self.as_source_range())?;
+        let Some(start) = crate::try_f64_to_i64(start) else {
             return Err(KclError::new_semantic(KclErrorDetails::new(
-                format!("Range start and end must be of the same type, but found {start} and {end}"),
+                format!("Range start must be an integer, but found {start}"),
                 vec![self.into()],
             )));
-        }
+        };
+        let Some(end) = crate::try_f64_to_i64(end) else {
+            return Err(KclError::new_semantic(KclErrorDetails::new(
+                format!("Range end must be an integer, but found {end}"),
+                vec![self.into()],
+            )));
+        };
 
         if end < start {
             return Err(KclError::new_semantic(KclErrorDetails::new(
@@ -2122,11 +2128,11 @@ impl Node<ArrayRangeExpression> {
                 .into_iter()
                 .map(|num| KclValue::Number {
                     value: num as f64,
-                    ty: start_ty,
+                    ty,
                     meta: meta.clone(),
                 })
                 .collect(),
-            ty: RuntimeType::Primitive(PrimitiveType::Number(start_ty)),
+            ty: RuntimeType::Primitive(PrimitiveType::Number(ty)),
         })
     }
 }

rust/kcl-lib/src/execution/types.rs

@@ -1,4 +1,4 @@
-use std::{collections::HashMap, fmt, str::FromStr};
+use std::{collections::HashMap, str::FromStr};
 
 use anyhow::Result;
 use kittycad_modeling_cmds::units::{UnitAngle, UnitLength};
@@ -12,6 +12,7 @@ use crate::{
         kcl_value::{KclValue, TypeDef},
         memory::{self},
     },
+    fmt,
     parsing::{
         ast::types::{PrimitiveType as AstPrimitiveType, Type},
         token::NumericSuffix,
@@ -323,8 +324,8 @@ impl RuntimeType {
     }
 }
 
-impl fmt::Display for RuntimeType {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+impl std::fmt::Display for RuntimeType {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             RuntimeType::Primitive(t) => t.fmt(f),
             RuntimeType::Array(t, l) => match l {
@@ -455,8 +456,8 @@ impl PrimitiveType {
     }
 }
 
-impl fmt::Display for PrimitiveType {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+impl std::fmt::Display for PrimitiveType {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             PrimitiveType::Any => write!(f, "any"),
             PrimitiveType::None => write!(f, "none"),
@@ -750,6 +751,78 @@ impl NumericType {
         }
     }
 
+    /// Combine two types for range operations.
+    ///
+    /// This combinator function is suitable for ranges where uncertainty should
+    /// be handled by the user, and it doesn't make sense to convert units. So
+    /// this is one of th most conservative ways to combine types.
+    pub fn combine_range(
+        a: TyF64,
+        b: TyF64,
+        exec_state: &mut ExecState,
+        source_range: SourceRange,
+    ) -> Result<(f64, f64, NumericType), KclError> {
+        use NumericType::*;
+        match (a.ty, b.ty) {
+            (at, bt) if at == bt => Ok((a.n, b.n, at)),
+            (at, Any) => Ok((a.n, b.n, at)),
+            (Any, bt) => Ok((a.n, b.n, bt)),
+
+            (Known(UnitType::Length(l1)), Known(UnitType::Length(l2))) => {
+                Err(KclError::new_semantic(KclErrorDetails::new(
+                    format!("Range start and range end have incompatible units: {l1} and {l2}"),
+                    vec![source_range],
+                )))
+            }
+            (Known(UnitType::Angle(a1)), Known(UnitType::Angle(a2))) => {
+                Err(KclError::new_semantic(KclErrorDetails::new(
+                    format!("Range start and range end have incompatible units: {a1} and {a2}"),
+                    vec![source_range],
+                )))
+            }
+
+            (t @ Known(UnitType::Length(_)), Known(UnitType::GenericLength)) => Ok((a.n, b.n, t)),
+            (Known(UnitType::GenericLength), t @ Known(UnitType::Length(_))) => Ok((a.n, b.n, t)),
+            (t @ Known(UnitType::Angle(_)), Known(UnitType::GenericAngle)) => Ok((a.n, b.n, t)),
+            (Known(UnitType::GenericAngle), t @ Known(UnitType::Angle(_))) => Ok((a.n, b.n, t)),
+
+            (Known(UnitType::Count), Default { .. }) | (Default { .. }, Known(UnitType::Count)) => {
+                Ok((a.n, b.n, Known(UnitType::Count)))
+            }
+            (t @ Known(UnitType::Length(l1)), Default { len: l2, .. }) if l1 == l2 => Ok((a.n, b.n, t)),
+            (Default { len: l1, .. }, t @ Known(UnitType::Length(l2))) if l1 == l2 => Ok((a.n, b.n, t)),
+            (t @ Known(UnitType::Angle(a1)), Default { angle: a2, .. }) if a1 == a2 => {
+                if b.n != 0.0 {
+                    exec_state.warn(
+                        CompilationError::err(source_range, "Prefer to use explicit units for angles"),
+                        annotations::WARN_ANGLE_UNITS,
+                    );
+                }
+                Ok((a.n, b.n, t))
+            }
+            (Default { angle: a1, .. }, t @ Known(UnitType::Angle(a2))) if a1 == a2 => {
+                if a.n != 0.0 {
+                    exec_state.warn(
+                        CompilationError::err(source_range, "Prefer to use explicit units for angles"),
+                        annotations::WARN_ANGLE_UNITS,
+                    );
+                }
+                Ok((a.n, b.n, t))
+            }
+
+            _ => {
+                let a = fmt::human_display_number(a.n, a.ty);
+                let b = fmt::human_display_number(b.n, b.ty);
+                Err(KclError::new_semantic(KclErrorDetails::new(
+                    format!(
+                        "Range start and range end must be of the same type and have compatible units, but found {a} and {b}",
+                    ),
+                    vec![source_range],
+                )))
+            }
+        }
+    }
+
     pub fn from_parsed(suffix: NumericSuffix, settings: &super::MetaSettings) -> Self {
         match suffix {
             NumericSuffix::None => NumericType::Default {

rust/kcl-lib/src/simulation_tests.rs

@@ -782,6 +782,28 @@ mod array_range_mismatch_units {
         super::execute(TEST_NAME, false).await
     }
 }
+mod array_range_units_default_count {
+    const TEST_NAME: &str = "array_range_units_default_count";
+
+    /// Test parsing KCL.
+    #[test]
+    fn parse() {
+        super::parse(TEST_NAME)
+    }
+
+    /// Test that parsing and unparsing KCL produces the original KCL input.
+    #[tokio::test(flavor = "multi_thread")]
+    async fn unparse() {
+        super::unparse(TEST_NAME).await
+    }
+
+    /// Test that KCL is executed correctly.
+    #[tokio::test(flavor = "multi_thread")]
+    async fn kcl_test_execute() {
+        super::execute(TEST_NAME, false).await
+    }
+}
+
 mod sketch_in_object {
     const TEST_NAME: &str = "sketch_in_object";
 

rust/kcl-lib/tests/array_range_mismatch_units/execution_error.snap

@@ -4,8 +4,7 @@ description: Error from executing array_range_mismatch_units.kcl
 ---
 KCL Semantic error
 
-  × semantic: Range start and end must be of the same type, but found 1:
-  │ number(mm) and 3: number(cm)
+  × semantic: Range start and range end have incompatible units: mm and cm
    ╭─[1:5]
  1 │ a = [1mm..3cm]
    ·     ─────┬────

rust/kcl-lib/tests/array_range_units_default_count/artifact_commands.snap

@@ -0,0 +1,5 @@
+---
+source: kcl-lib/src/simulation_tests.rs
+description: Artifact commands array_range_units_default_count.kcl
+---
+{}

rust/kcl-lib/tests/array_range_units_default_count/artifact_graph_flowchart.snap

@@ -0,0 +1,6 @@
+---
+source: kcl-lib/src/simulation_tests.rs
+description: Artifact graph flowchart array_range_units_default_count.kcl
+extension: md
+snapshot_kind: binary
+---

rust/kcl-lib/tests/array_range_units_default_count/artifact_graph_flowchart.snap.md

@@ -0,0 +1,3 @@
+```mermaid
+flowchart LR
+```

rust/kcl-lib/tests/array_range_units_default_count/ast.snap

@@ -0,0 +1,73 @@
+---
+source: kcl-lib/src/simulation_tests.rs
+description: Result of parsing array_range_units_default_count.kcl
+---
+{
+  "Ok": {
+    "body": [
+      {
+        "commentStart": 0,
+        "declaration": {
+          "commentStart": 0,
+          "end": 0,
+          "id": {
+            "commentStart": 0,
+            "end": 0,
+            "moduleId": 0,
+            "name": "indices",
+            "start": 0,
+            "type": "Identifier"
+          },
+          "init": {
+            "commentStart": 0,
+            "end": 0,
+            "endElement": {
+              "commentStart": 0,
+              "end": 0,
+              "moduleId": 0,
+              "raw": "3_",
+              "start": 0,
+              "type": "Literal",
+              "type": "Literal",
+              "value": {
+                "value": 3.0,
+                "suffix": "Count"
+              }
+            },
+            "endInclusive": false,
+            "moduleId": 0,
+            "start": 0,
+            "startElement": {
+              "commentStart": 0,
+              "end": 0,
+              "moduleId": 0,
+              "raw": "0",
+              "start": 0,
+              "type": "Literal",
+              "type": "Literal",
+              "value": {
+                "value": 0.0,
+                "suffix": "None"
+              }
+            },
+            "type": "ArrayRangeExpression",
+            "type": "ArrayRangeExpression"
+          },
+          "moduleId": 0,
+          "start": 0,
+          "type": "VariableDeclarator"
+        },
+        "end": 0,
+        "kind": "const",
+        "moduleId": 0,
+        "start": 0,
+        "type": "VariableDeclaration",
+        "type": "VariableDeclaration"
+      }
+    ],
+    "commentStart": 0,
+    "end": 0,
+    "moduleId": 0,
+    "start": 0
+  }
+}

rust/kcl-lib/tests/array_range_units_default_count/execution_success.snap

@@ -0,0 +1,5 @@
+---
+source: kcl-lib/src/simulation_tests.rs
+description: Execution success array_range_units_default_count.kcl
+---
+null

rust/kcl-lib/tests/array_range_units_default_count/input.kcl

@@ -0,0 +1 @@
+indices = [0 ..< 3_]