Universal quantification semantics for Length and Angle

Signed-off-by: Nick Cameron <[email protected]>

Author: nrc

docs/kcl-std/functions/std-vector-cross.md

@@ -11,7 +11,7 @@ Find the cross product of two 3D points or vectors.
 vector::cross(
   @u: Point3d,
   v: Point3d,
-)
+): Point3d
 ```
 
 
@@ -23,6 +23,10 @@ vector::cross(
 | `u` | [`Point3d`](/docs/kcl-std/types/std-types-Point3d) | A point in three dimensional space. | Yes |
 | `v` | [`Point3d`](/docs/kcl-std/types/std-types-Point3d) | A point in three dimensional space. | Yes |
 
+### Returns
+
+[`Point3d`](/docs/kcl-std/types/std-types-Point3d) - A point in three dimensional space.
+
 
 ### Examples
 

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

@@ -10,7 +10,7 @@ use crate::{
         cad_op::{Group, OpArg, OpKclValue, Operation},
         kcl_value::FunctionSource,
         memory,
-        types::RuntimeType,
+        types::{DisplayType, RuntimeType, UnitSubsts},
     },
     parsing::ast::types::{CallExpressionKw, DefaultParamVal, FunctionExpression, Node, Program, Type},
     std::StdFn,
@@ -215,7 +215,7 @@ impl Node<CallExpressionKw> {
 
         // Clone the function so that we can use a mutable reference to
         // exec_state.
-        let func: KclValue = fn_name.get_result(exec_state, ctx).await?.clone();
+        let func = fn_name.get_result(exec_state, ctx).await?.clone();
 
         let Some(fn_src) = func.as_function() else {
             return Err(KclError::new_semantic(KclErrorDetails::new(
@@ -261,7 +261,7 @@ impl Node<CallExpressionKw> {
         let args = Args::new(fn_args, unlabeled, callsite, exec_state, ctx.clone());
 
         let return_value = fn_src
-            .call_kw(Some(fn_name.to_string()), exec_state, ctx, args, callsite)
+            .call(Some(fn_name.to_string()), exec_state, ctx, args, callsite)
             .await
             .map_err(|e| {
                 // Add the call expression to the source ranges.
@@ -291,7 +291,7 @@ impl Node<CallExpressionKw> {
 }
 
 impl FunctionDefinition<'_> {
-    pub async fn call_kw(
+    pub async fn call(
         &self,
         fn_name: Option<String>,
         exec_state: &mut ExecState,
@@ -324,7 +324,7 @@ impl FunctionDefinition<'_> {
             );
         }
 
-        let args = type_check_params_kw(fn_name.as_deref(), self, args, exec_state)?;
+        let (args, unit_substs) = type_check_params(fn_name.as_deref(), self, args, exec_state)?;
 
         // Don't early return until the stack frame is popped!
         self.body.prep_mem(exec_state);
@@ -367,11 +367,13 @@ impl FunctionDefinition<'_> {
             None
         };
 
+        exec_state.mut_unit_stack().push(unit_substs);
         let mut result = match &self.body {
             FunctionBody::Rust(f) => f(exec_state, args).await.map(Some),
             FunctionBody::Kcl(f, _) => {
-                if let Err(e) = assign_args_to_params_kw(self, args, exec_state) {
+                if let Err(e) = assign_args_to_params(self, args, exec_state) {
                     exec_state.mut_stack().pop_env();
+                    exec_state.mut_unit_stack().pop();
                     return Err(e);
                 }
 
@@ -386,6 +388,7 @@ impl FunctionDefinition<'_> {
         };
 
         exec_state.mut_stack().pop_env();
+        exec_state.mut_unit_stack().pop();
 
         if let Some(mut op) = op {
             op.set_std_lib_call_is_error(result.is_err());
@@ -405,7 +408,21 @@ impl FunctionDefinition<'_> {
             update_memory_for_tags_of_geometry(result, exec_state)?;
         }
 
-        coerce_result_type(result, self, exec_state)
+        let ret_ty = self
+            .return_type
+            .as_ref()
+            .map(|ret_ty| {
+                let mut ty = RuntimeType::from_parsed(ret_ty.inner.clone(), exec_state, ret_ty.as_source_range())
+                    .map_err(|e| KclError::new_semantic(e.into()))?;
+                ty.subst_units(unit_substs);
+                Ok::<_, KclError>(ty)
+            })
+            .transpose()?;
+        if let Some(ret_ty) = &ret_ty {
+            coerce_result_type(result, ret_ty, exec_state)
+        } else {
+            result
+        }
     }
 
     // Postcondition: result.is_some() if function is not in the standard library.
@@ -427,7 +444,7 @@ impl FunctionBody<'_> {
 }
 
 impl FunctionSource {
-    pub async fn call_kw(
+    pub async fn call(
         &self,
         fn_name: Option<String>,
         exec_state: &mut ExecState,
@@ -436,7 +453,7 @@ impl FunctionSource {
         callsite: SourceRange,
     ) -> Result<Option<KclValue>, KclError> {
         let def: FunctionDefinition = self.into();
-        def.call_kw(fn_name, exec_state, ctx, args, callsite).await
+        def.call(fn_name, exec_state, ctx, args, callsite).await
     }
 }
 
@@ -551,7 +568,12 @@ fn update_memory_for_tags_of_geometry(result: &mut KclValue, exec_state: &mut Ex
     Ok(())
 }
 
-fn type_err_str(expected: &Type, found: &KclValue, source_range: &SourceRange, exec_state: &mut ExecState) -> String {
+fn type_err_str(
+    expected: &impl DisplayType,
+    found: &KclValue,
+    source_range: &SourceRange,
+    exec_state: &mut ExecState,
+) -> String {
     fn strip_backticks(s: &str) -> &str {
         let mut result = s;
         if s.starts_with('`') {
@@ -563,8 +585,8 @@ fn type_err_str(expected: &Type, found: &KclValue, source_range: &SourceRange, e
         result
     }
 
-    let expected_human = expected.human_friendly_type();
-    let expected_ty = expected.to_string();
+    let expected_human = expected.human_friendly_string();
+    let expected_ty = expected.src_string();
     let expected_str =
         if expected_human == expected_ty || expected_human == format!("a value with type `{expected_ty}`") {
             format!("a value with type `{expected_ty}`")
@@ -581,20 +603,20 @@ fn type_err_str(expected: &Type, found: &KclValue, source_range: &SourceRange, e
 
     let mut result = format!("{expected_str}, but found {found_str}.");
 
-    if found.is_unknown_number() {
+    if found.is_unknown_number(exec_state) {
         exec_state.clear_units_warnings(source_range);
         result.push_str("\nThe found value is a number but has incomplete units information. You can probably fix this error by specifying the units using type ascription, e.g., `len: mm` or `(a * b): deg`.");
     }
 
     result
 }
 
-fn type_check_params_kw(
+fn type_check_params(
     fn_name: Option<&str>,
     fn_def: &FunctionDefinition<'_>,
     mut args: Args<Sugary>,
     exec_state: &mut ExecState,
-) -> Result<Args<Desugared>, KclError> {
+) -> Result<(Args<Desugared>, UnitSubsts), KclError> {
     let mut result = Args::new_no_args(args.source_range, args.ctx);
 
     // If it's possible the input arg was meant to be labelled and we probably don't want to use
@@ -609,6 +631,10 @@ fn type_check_params_kw(
         args.labeled.insert(label.unwrap(), arg);
     }
 
+    // Collect substitutions for `number(Length)` or `number(Angle)`. See docs on execution::types::UnitSubsts
+    // for details.
+    let mut unit_substs = UnitSubsts::default();
+
     // Apply the `a == a: a` shorthand by desugaring unlabeled args into labeled ones.
     let (labeled_unlabeled, unlabeled_unlabeled) = args.unlabeled.into_iter().partition(|(l, _)| {
         if let Some(l) = l
@@ -657,20 +683,28 @@ fn type_check_params_kw(
         } else if args.unlabeled.len() == 1 {
             let mut arg = args.unlabeled.pop().unwrap().1;
             if let Some(ty) = ty {
-                let rty = RuntimeType::from_parsed(ty.clone(), exec_state, arg.source_range)
+                let mut rty = RuntimeType::from_parsed(ty.clone(), exec_state, arg.source_range)
                     .map_err(|e| KclError::new_semantic(e.into()))?;
-                arg.value = arg.value.coerce(&rty, true, exec_state).map_err(|_| {
-                    KclError::new_argument(KclErrorDetails::new(
-                        format!(
-                            "The input argument of {} requires {}",
-                            fn_name
-                                .map(|n| format!("`{n}`"))
-                                .unwrap_or_else(|| "this function".to_owned()),
-                            type_err_str(ty, &arg.value, &arg.source_range, exec_state),
-                        ),
-                        vec![arg.source_range],
-                    ))
-                })?;
+                rty.subst_units(unit_substs);
+
+                let (value, substs) = arg
+                    .value
+                    .coerce_and_find_unit_substs(&rty, true, exec_state)
+                    .map_err(|_| {
+                        KclError::new_argument(KclErrorDetails::new(
+                            format!(
+                                "The input argument of {} requires {}",
+                                fn_name
+                                    .map(|n| format!("`{n}`"))
+                                    .unwrap_or_else(|| "this function".to_owned()),
+                                type_err_str(ty, &arg.value, &arg.source_range, exec_state),
+                            ),
+                            vec![arg.source_range],
+                        ))
+                    })?;
+
+                arg.value = value;
+                unit_substs = unit_substs.or(substs);
             }
             result.unlabeled = vec![(None, arg)]
         } else {
@@ -748,11 +782,13 @@ fn type_check_params_kw(
                 // For optional args, passing None should be the same as not passing an arg.
                 if !(def.is_some() && matches!(arg.value, KclValue::KclNone { .. })) {
                     if let Some(ty) = ty {
-                        let rty = RuntimeType::from_parsed(ty.clone(), exec_state, arg.source_range)
+                        let mut rty = RuntimeType::from_parsed(ty.clone(), exec_state, arg.source_range)
                             .map_err(|e| KclError::new_semantic(e.into()))?;
-                        arg.value = arg
+                        rty.subst_units(unit_substs);
+
+                        let (value, substs) = arg
                                 .value
-                                .coerce(
+                                .coerce_and_find_unit_substs(
                                     &rty,
                                     true,
                                     exec_state,
@@ -771,6 +807,9 @@ fn type_check_params_kw(
                                         vec![arg.source_range],
                                     ))
                                 })?;
+
+                        arg.value = value;
+                        unit_substs = unit_substs.or(substs);
                     }
                     result.labeled.insert(label, arg);
                 }
@@ -789,10 +828,10 @@ fn type_check_params_kw(
         }
     }
 
-    Ok(result)
+    Ok((result, unit_substs))
 }
 
-fn assign_args_to_params_kw(
+fn assign_args_to_params(
     fn_def: &FunctionDefinition<'_>,
     args: Args<Desugared>,
     exec_state: &mut ExecState,
@@ -848,26 +887,20 @@ fn assign_args_to_params_kw(
 
 fn coerce_result_type(
     result: Result<Option<KclValue>, KclError>,
-    fn_def: &FunctionDefinition<'_>,
+    return_ty: &RuntimeType,
     exec_state: &mut ExecState,
 ) -> Result<Option<KclValue>, KclError> {
     if let Ok(Some(val)) = result {
-        if let Some(ret_ty) = &fn_def.return_type {
-            let ty = RuntimeType::from_parsed(ret_ty.inner.clone(), exec_state, ret_ty.as_source_range())
-                .map_err(|e| KclError::new_semantic(e.into()))?;
-            let val = val.coerce(&ty, true, exec_state).map_err(|_| {
-                KclError::new_type(KclErrorDetails::new(
-                    format!(
-                        "This function requires its result to be {}",
-                        type_err_str(ret_ty, &val, &(&val).into(), exec_state)
-                    ),
-                    ret_ty.as_source_ranges(),
-                ))
-            })?;
-            Ok(Some(val))
-        } else {
-            Ok(Some(val))
-        }
+        let val = val.coerce(return_ty, true, exec_state).map_err(|_| {
+            KclError::new_type(KclErrorDetails::new(
+                format!(
+                    "This function requires its result to be {}",
+                    type_err_str(return_ty, &val, &(&val).into(), exec_state)
+                ),
+                val.into(),
+            ))
+        })?;
+        Ok(Some(val))
     } else {
         result
     }
@@ -1016,9 +1049,8 @@ mod test {
                 pipe_value: None,
                 _status: std::marker::PhantomData,
             };
-
-            let actual = assign_args_to_params_kw(&(&func_src).into(), args, &mut exec_state)
-                .map(|_| exec_state.mod_local.stack);
+            let actual =
+                assign_args_to_params(&(&func_src).into(), args, &mut exec_state).map(|_| exec_state.mod_local.stack);
             assert_eq!(
                 actual, expected,
                 "failed test '{test_name}':\ngot {actual:?}\nbut expected\n{expected:?}"

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

@@ -660,9 +660,9 @@ impl KclValue {
         })
     }
 
-    pub fn is_unknown_number(&self) -> bool {
+    pub fn is_unknown_number(&self, exec_state: &ExecState) -> bool {
         match self {
-            KclValue::Number { ty, .. } => !ty.is_fully_specified(),
+            KclValue::Number { ty, .. } => !ty.is_fully_specified(exec_state),
             _ => false,
         }
     }

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

@@ -2571,4 +2571,17 @@ startSketchOn(XY)
 "#;
         parse_execute(code).await.unwrap_err();
     }
+
+    #[tokio::test(flavor = "multi_thread")]
+    async fn vector_module() {
+        let ast = r#"
+cylinder = startSketchOn(XY)
+  |> circle(center = [0.67, 0.47], radius = 1.06)
+  |> extrude(length = 5)
+p = planeOf(cylinder, face = END)
+q = vector::cross(p.xAxis, v = p.yAxis)
+r = vector::cross(p.xAxis, v = q)
+"#;
+        parse_execute(ast).await.unwrap();
+    }
 }

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

@@ -16,7 +16,7 @@ use crate::{
         cad_op::Operation,
         id_generator::IdGenerator,
         memory::{ProgramMemory, Stack},
-        types::{self, NumericType},
+        types::{self, NumericType, UnitSubsts},
     },
     modules::{ModuleId, ModuleInfo, ModuleLoader, ModulePath, ModuleRepr, ModuleSource},
     parsing::ast::types::{Annotation, NodeRef},
@@ -103,6 +103,10 @@ pub(super) struct ModuleState {
     pub settings: MetaSettings,
     pub(super) explicit_length_units: bool,
     pub(super) path: ModulePath,
+    /// The unit substitutions for the current stack frame (see docs on execution::types::UnitSubsts
+    /// for more detail). Each stack frame has a possible substitution, and the stack is never empty. The
+    /// root stack frame always has `(None, None)`.
+    pub(super) unit_stack: Vec<UnitSubsts>,
     /// Artifacts for only this module.
     pub artifacts: ModuleArtifactState,
 
@@ -205,6 +209,19 @@ impl ExecState {
         &mut self.mod_local.stack
     }
 
+    pub(crate) fn mut_unit_stack(&mut self) -> &mut Vec<UnitSubsts> {
+        &mut self.mod_local.unit_stack
+    }
+
+    pub(crate) fn current_unit_substs(&self) -> UnitSubsts {
+        debug_assert!(self.mod_local.unit_stack.last().is_some());
+        self.mod_local
+            .unit_stack
+            .last()
+            .cloned()
+            .unwrap_or(UnitSubsts::default())
+    }
+
     pub fn next_uuid(&mut self) -> Uuid {
         self.mod_local.id_generator.next_uuid()
     }
@@ -512,6 +529,7 @@ impl ModuleState {
             being_declared: Default::default(),
             module_exports: Default::default(),
             explicit_length_units: false,
+            unit_stack: vec![UnitSubsts::default()],
             path,
             settings: Default::default(),
             artifacts: Default::default(),