diff --git a/docs/kcl-lang/numeric.md b/docs/kcl-lang/numeric.md index 728c9dfa1..ac0258eef 100644 --- a/docs/kcl-lang/numeric.md +++ b/docs/kcl-lang/numeric.md @@ -46,3 +46,7 @@ KCL has no support for area, volume, or other higher dimension units. When inter ## Explicit conversions You might sometimes need to convert from one unit to another for some calculation. You can do this implicitly when calling a function (see above), but if you can't or don't want to, then you can use the explicit conversion functions in the [`std::units`](/docs/kcl-std/modules/std-units) module. + +KCL cannot know about changes to units caused by arithmetic. For example, you may intend for `10in * 25.4` to be the value `254mm` (i.e., `10in` in mm), however, the result of that computation in KCL is `254in`. It is always better to rely on automatic conversion or to use the explicit conversion functions, where possible. + +Converting between degrees and radians using π ([`PI`](/docs/kcl-std/consts/std-math-PI) in KCL) is especially prone to this error and so the `PI` constant always requires specifying units of any computation it is used with. E.g., `radius = (circumference / (2 * PI)): number(mm)`. diff --git a/docs/kcl-std/consts/std-math-PI.md b/docs/kcl-std/consts/std-math-PI.md index 8eb4c3206..5e98cbb41 100644 --- a/docs/kcl-std/consts/std-math-PI.md +++ b/docs/kcl-std/consts/std-math-PI.md @@ -16,8 +16,8 @@ However, `PI` is nearly always used for converting between different units - usu from radians. Therefore, `PI` is treated a bit specially by KCL and always has unknown units. This means that if you use `PI`, you will need to give KCL some extra information about the units of numbers. Usually you should use type ascription on the result of calculations, e.g., `(2 * PI): number(rad)`. -You might prefer to use `units::toRadians` or `units::toDegrees` to convert between angles with -different units. +It is better to use `units::toRadians` or `units::toDegrees` to convert between angles with +different units where possible. ### Examples diff --git a/e2e/playwright/projects.spec.ts b/e2e/playwright/projects.spec.ts index 5974d9671..0eea463a4 100644 --- a/e2e/playwright/projects.spec.ts +++ b/e2e/playwright/projects.spec.ts @@ -170,7 +170,7 @@ test( // error text on hover await page.hover('.cm-lint-marker-error') const crypticErrorText = - 'tag requires a value with type `tag`, but found string' + 'tag requires a value with type `tag`, but found a value with type `string`.' await expect(page.getByText(crypticErrorText).first()).toBeVisible() // black pixel means the scene has been cleared. @@ -369,7 +369,7 @@ test( // error text on hover await page.hover('.cm-lint-marker-error') const crypticErrorText = - 'tag requires a value with type `tag`, but found string' + 'tag requires a value with type `tag`, but found a value with type `string`.' await expect(page.getByText(crypticErrorText).first()).toBeVisible() // black pixel means the scene has been cleared. @@ -408,7 +408,7 @@ test( // error text on hover await page.hover('.cm-lint-marker-error') const crypticErrorText = - 'tag requires a value with type `tag`, but found string' + 'tag requires a value with type `tag`, but found a value with type `string`.' await expect(page.getByText(crypticErrorText).first()).toBeVisible() } ) diff --git a/rust/kcl-lib/e2e/executor/main.rs b/rust/kcl-lib/e2e/executor/main.rs index 9b4aca2ef..f1f84a86f 100644 --- a/rust/kcl-lib/e2e/executor/main.rs +++ b/rust/kcl-lib/e2e/executor/main.rs @@ -1230,7 +1230,10 @@ secondSketch = startSketchOn(part001, face = '') let result = execute_and_snapshot(code, None).await; let err = result.unwrap_err(); let err = err.as_kcl_error().unwrap(); - assert_eq!(err.message(), "face requires a value with type `tag`, but found string"); + assert_eq!( + err.message(), + "face requires a value with type `tag`, but found a value with type `string`." + ); } #[tokio::test(flavor = "multi_thread")] @@ -1962,7 +1965,7 @@ someFunction('INVALID') let err = err.as_kcl_error().unwrap(); assert_eq!( err.message(), - "The input argument of `startSketchOn` requires a value with type `Solid | Plane`, but found string" + "The input argument of `startSketchOn` requires a value with type `Solid` or a value with type `Plane` (`Solid | Plane`), but found a value with type `string`." ); assert_eq!( err.source_ranges(), @@ -2087,7 +2090,7 @@ async fn kcl_test_better_type_names() { }, None => todo!(), }; - assert_eq!(err, "This function expected the input argument to be one or more Solids or imported geometry but it's actually of type Sketch. You can convert a sketch (2D) into a Solid (3D) by calling a function like `extrude` or `revolve`"); + assert_eq!(err, "This function expected the input argument to be one or more Solids or ImportedGeometry but it's actually of type Sketch. You can convert a sketch (2D) into a Solid (3D) by calling a function like `extrude` or `revolve`"); } #[tokio::test(flavor = "multi_thread")] diff --git a/rust/kcl-lib/src/errors.rs b/rust/kcl-lib/src/errors.rs index 956eb40ac..9561fa91e 100644 --- a/rust/kcl-lib/src/errors.rs +++ b/rust/kcl-lib/src/errors.rs @@ -783,6 +783,7 @@ impl Severity { pub enum Tag { Deprecated, Unnecessary, + UnknownNumericUnits, None, } diff --git a/rust/kcl-lib/src/execution/exec_ast.rs b/rust/kcl-lib/src/execution/exec_ast.rs index f44cb6f74..388eea246 100644 --- a/rust/kcl-lib/src/execution/exec_ast.rs +++ b/rust/kcl-lib/src/execution/exec_ast.rs @@ -801,6 +801,10 @@ fn apply_ascription( let ty = RuntimeType::from_parsed(ty.inner.clone(), exec_state, value.into()) .map_err(|e| KclError::new_semantic(e.into()))?; + if matches!(&ty, &RuntimeType::Primitive(PrimitiveType::Number(..))) { + exec_state.clear_units_warnings(&source_range); + } + value.coerce(&ty, false, exec_state).map_err(|_| { let suggestion = if ty == RuntimeType::length() { ", you might try coercing to a fully specified numeric type such as `number(mm)`" @@ -809,9 +813,14 @@ fn apply_ascription( } else { "" }; + let ty_str = if let Some(ty) = value.principal_type() { + format!("(with type `{ty}`) ") + } else { + String::new() + }; KclError::new_semantic(KclErrorDetails::new( format!( - "could not coerce value of type {} to type {ty}{suggestion}", + "could not coerce {} {ty_str}to type `{ty}`{suggestion}", value.human_friendly_type() ), vec![source_range], @@ -1021,14 +1030,13 @@ impl Node { .map(|(k, tag)| (k.to_owned(), KclValue::TagIdentifier(Box::new(tag.to_owned())))) .collect(), }), - (being_indexed, _, _) => { - let t = being_indexed.human_friendly_type(); - let article = article_for(&t); - Err(KclError::new_semantic(KclErrorDetails::new( - format!("Only arrays can be indexed, but you're trying to index {article} {t}"), - vec![self.clone().into()], - ))) - } + (being_indexed, _, _) => Err(KclError::new_semantic(KclErrorDetails::new( + format!( + "Only arrays can be indexed, but you're trying to index {}", + being_indexed.human_friendly_type() + ), + vec![self.clone().into()], + ))), } } } @@ -1203,11 +1211,14 @@ impl Node { fn warn_on_unknown(&self, ty: &NumericType, verb: &str, exec_state: &mut ExecState) { if ty == &NumericType::Unknown { - // TODO suggest how to fix this - exec_state.warn(CompilationError::err( - self.as_source_range(), - format!("{} numbers which have unknown or incompatible units.", verb), - )); + let sr = self.as_source_range(); + exec_state.clear_units_warnings(&sr); + let mut err = CompilationError::err( + sr, + format!("{} numbers which have unknown or incompatible units.\nYou can probably fix this error by specifying the units using type ascription, e.g., `len: number(mm)` or `(a * b): number(deg)`.", verb), + ); + err.tag = crate::errors::Tag::UnknownNumericUnits; + exec_state.warn(err); } } } @@ -1756,7 +1767,7 @@ a = 42: string let err = result.unwrap_err(); assert!( err.to_string() - .contains("could not coerce value of type number(default units) to type string"), + .contains("could not coerce a number (with type `number`) to type `string`"), "Expected error but found {err:?}" ); @@ -1767,7 +1778,7 @@ a = 42: Plane let err = result.unwrap_err(); assert!( err.to_string() - .contains("could not coerce value of type number(default units) to type Plane"), + .contains("could not coerce a number (with type `number`) to type `Plane`"), "Expected error but found {err:?}" ); @@ -1778,7 +1789,7 @@ arr = [0]: [string] let err = result.unwrap_err(); assert!( err.to_string().contains( - "could not coerce value of type array of number(default units) with 1 value to type [string]" + "could not coerce an array of `number` with 1 value (with type `[any; 1]`) to type `[string]`" ), "Expected error but found {err:?}" ); @@ -1789,8 +1800,9 @@ mixedArr = [0, "a"]: [number(mm)] let result = parse_execute(program).await; let err = result.unwrap_err(); assert!( - err.to_string() - .contains("could not coerce value of type array of number(default units), string with 2 values to type [number(mm)]"), + err.to_string().contains( + "could not coerce an array of `number`, `string` (with type `[any; 2]`) to type `[number(mm)]`" + ), "Expected error but found {err:?}" ); } @@ -2095,4 +2107,19 @@ y = x: number(Length)"#; assert_eq!(num.n, 2.0); assert_eq!(num.ty, NumericType::mm()); } + + #[tokio::test(flavor = "multi_thread")] + async fn one_warning_unknown() { + let ast = r#" +// Should warn once +a = PI * 2 +// Should warn once +b = (PI * 2) / 3 +// Should not warn +c = ((PI * 2) / 3): number(deg) +"#; + + let result = parse_execute(ast).await.unwrap(); + assert_eq!(result.exec_state.errors().len(), 2); + } } diff --git a/rust/kcl-lib/src/execution/fn_call.rs b/rust/kcl-lib/src/execution/fn_call.rs index 62c370ca8..5188597f3 100644 --- a/rust/kcl-lib/src/execution/fn_call.rs +++ b/rust/kcl-lib/src/execution/fn_call.rs @@ -532,6 +532,44 @@ 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 strip_backticks(s: &str) -> &str { + let mut result = s; + if s.starts_with('`') { + result = &result[1..] + } + if s.ends_with('`') { + result = &result[..result.len() - 1] + } + result + } + + let expected_human = expected.human_friendly_type(); + let expected_ty = expected.to_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}`") + } else { + format!("{expected_human} (`{expected_ty}`)") + }; + let found_human = found.human_friendly_type(); + let found_ty = found.principal_type_string(); + let found_str = if found_human == found_ty || found_human == format!("a {}", strip_backticks(&found_ty)) { + format!("a value with type {}", found_ty) + } else { + format!("{found_human} (with type {})", found_ty) + }; + + let mut result = format!("{expected_str}, but found {found_str}."); + + if found.is_unknown_number() { + 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: number(mm)` or `(a * b): number(deg)`."); + } + + result +} + fn type_check_params_kw( fn_name: Option<&str>, fn_def: &FunctionDefinition<'_>, @@ -556,18 +594,19 @@ 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) + .map_err(|e| KclError::new_semantic(e.into()))?; arg.value = arg .value .coerce( - &RuntimeType::from_parsed(ty.clone(), exec_state, arg.source_range).map_err(|e| KclError::new_semantic(e.into()))?, + &rty, true, exec_state, ) .map_err(|e| { let mut message = format!( - "{label} requires a value with type `{}`, but found {}", - ty, - arg.value.human_friendly_type(), + "{label} requires {}", + type_err_str(ty, &arg.value, &arg.source_range, exec_state), ); if let Some(ty) = e.explicit_coercion { // TODO if we have access to the AST for the argument we could choose which example to suggest. @@ -630,28 +669,20 @@ fn type_check_params_kw( if let Some(arg) = &mut args.unlabeled { if let Some((_, Some(ty))) = &fn_def.input_arg { - arg.1.value = arg - .1 - .value - .coerce( - &RuntimeType::from_parsed(ty.clone(), exec_state, arg.1.source_range) - .map_err(|e| KclError::new_semantic(e.into()))?, - true, - exec_state, - ) - .map_err(|_| { - KclError::new_semantic(KclErrorDetails::new( - format!( - "The input argument of {} requires a value with type `{}`, but found {}", - fn_name - .map(|n| format!("`{}`", n)) - .unwrap_or_else(|| "this function".to_owned()), - ty, - arg.1.value.human_friendly_type() - ), - vec![arg.1.source_range], - )) - })?; + let rty = RuntimeType::from_parsed(ty.clone(), exec_state, arg.1.source_range) + .map_err(|e| KclError::new_semantic(e.into()))?; + arg.1.value = arg.1.value.coerce(&rty, true, exec_state).map_err(|_| { + KclError::new_semantic(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.1.value, &arg.1.source_range, exec_state), + ), + vec![arg.1.source_range], + )) + })?; } } else if let Some((name, _)) = &fn_def.input_arg { if let Some(arg) = args.labeled.get(name) { @@ -747,9 +778,8 @@ fn coerce_result_type( let val = val.coerce(&ty, true, exec_state).map_err(|_| { KclError::new_semantic(KclErrorDetails::new( format!( - "This function requires its result to be of type `{}`, but found {}", - ty.human_friendly_type(), - val.human_friendly_type(), + "This function requires its result to be {}", + type_err_str(ret_ty, &val, &(&val).into(), exec_state) ), ret_ty.as_source_ranges(), )) @@ -928,7 +958,7 @@ msg2 = makeMessage(prefix = 1, suffix = 3)"#; let err = parse_execute(program).await.unwrap_err(); assert_eq!( err.message(), - "prefix requires a value with type `string`, but found number(default units)" + "prefix requires a value with type `string`, but found a value with type `number`.\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: number(mm)` or `(a * b): number(deg)`." ) } } diff --git a/rust/kcl-lib/src/execution/kcl_value.rs b/rust/kcl-lib/src/execution/kcl_value.rs index b839a5a6e..7537a6781 100644 --- a/rust/kcl-lib/src/execution/kcl_value.rs +++ b/rust/kcl-lib/src/execution/kcl_value.rs @@ -4,7 +4,6 @@ use anyhow::Result; use schemars::JsonSchema; use serde::Serialize; -use super::types::UnitType; use crate::{ errors::KclErrorDetails, execution::{ @@ -281,69 +280,57 @@ impl KclValue { /// Human readable type name used in error messages. Should not be relied /// on for program logic. pub(crate) fn human_friendly_type(&self) -> String { - self.inner_human_friendly_type(1) - } - - fn inner_human_friendly_type(&self, max_depth: usize) -> String { - if let Some(pt) = self.principal_type() { - if max_depth > 0 { - // The principal type of an array uses the array's element type, - // which is oftentimes `any`, and that's not a helpful message. So - // we show the actual elements. - if let KclValue::Tuple { value, .. } | KclValue::HomArray { value, .. } = self { - // If it's empty, we want to show the type of the array. - if !value.is_empty() { - // A max of 3 is good because it's common to use 3D points. - let max = 3; - let len = value.len(); - let ellipsis = if len > max { ", ..." } else { "" }; - let element_label = if len == 1 { "value" } else { "values" }; - let element_tys = value - .iter() - .take(max) - .map(|elem| elem.inner_human_friendly_type(max_depth - 1)) - .collect::>() - .join(", "); - return format!("array of {element_tys}{ellipsis} with {len} {element_label}"); - } - } - } - return pt.to_string(); - } match self { - KclValue::Uuid { .. } => "Unique ID (uuid)", - KclValue::TagDeclarator(_) => "TagDeclarator", - KclValue::TagIdentifier(_) => "TagIdentifier", - KclValue::Solid { .. } => "Solid", - KclValue::Sketch { .. } => "Sketch", - KclValue::Helix { .. } => "Helix", - KclValue::ImportedGeometry(_) => "ImportedGeometry", - KclValue::Function { .. } => "Function", - KclValue::Plane { .. } => "Plane", - KclValue::Face { .. } => "Face", - KclValue::Bool { .. } => "boolean (true/false value)", + KclValue::Uuid { .. } => "a unique ID (uuid)".to_owned(), + KclValue::TagDeclarator(_) => "a tag declarator".to_owned(), + KclValue::TagIdentifier(_) => "a tag identifier".to_owned(), + KclValue::Solid { .. } => "a solid".to_owned(), + KclValue::Sketch { .. } => "a sketch".to_owned(), + KclValue::Helix { .. } => "a helix".to_owned(), + KclValue::ImportedGeometry(_) => "an imported geometry".to_owned(), + KclValue::Function { .. } => "a function".to_owned(), + KclValue::Plane { .. } => "a plane".to_owned(), + KclValue::Face { .. } => "a face".to_owned(), + KclValue::Bool { .. } => "a boolean (`true` or `false`)".to_owned(), KclValue::Number { ty: NumericType::Unknown, .. - } => "number(unknown units)", + } => "a number with unknown units".to_owned(), KclValue::Number { - ty: NumericType::Known(UnitType::Length(_)), + ty: NumericType::Known(units), .. - } => "number(Length)", - KclValue::Number { - ty: NumericType::Known(UnitType::Angle(_)), - .. - } => "number(Angle)", - KclValue::Number { .. } => "number", - KclValue::String { .. } => "string (text)", - KclValue::Tuple { .. } => "tuple (list)", - KclValue::HomArray { .. } => "array (list)", - KclValue::Object { .. } => "object", - KclValue::Module { .. } => "module", - KclValue::Type { .. } => "type", - KclValue::KclNone { .. } => "None", + } => format!("a number ({units})"), + KclValue::Number { .. } => "a number".to_owned(), + KclValue::String { .. } => "a string".to_owned(), + KclValue::Object { .. } => "an object".to_owned(), + KclValue::Module { .. } => "a module".to_owned(), + KclValue::Type { .. } => "a type".to_owned(), + KclValue::KclNone { .. } => "none".to_owned(), + KclValue::Tuple { value, .. } | KclValue::HomArray { value, .. } => { + if value.is_empty() { + "an empty array".to_owned() + } else { + // A max of 3 is good because it's common to use 3D points. + const MAX: usize = 3; + + let len = value.len(); + let element_tys = value + .iter() + .take(MAX) + .map(|elem| elem.principal_type_string()) + .collect::>() + .join(", "); + let mut result = format!("an array of {element_tys}"); + if len > MAX { + result.push_str(&format!(", ... with {len} values")); + } + if len == 1 { + result.push_str(" with 1 value"); + } + result + } + } } - .to_owned() } pub(crate) fn from_literal(literal: Node, exec_state: &mut ExecState) -> Self { @@ -602,6 +589,13 @@ impl KclValue { }) } + pub fn is_unknown_number(&self) -> bool { + match self { + KclValue::Number { ty, .. } => !ty.is_fully_specified(), + _ => false, + } + } + pub fn value_str(&self) -> Option { match self { KclValue::Bool { value, .. } => Some(format!("{value}")), @@ -649,6 +643,8 @@ impl From for KclValue { #[cfg(test)] mod tests { + use crate::exec::UnitType; + use super::*; #[test] @@ -658,21 +654,21 @@ mod tests { ty: NumericType::Known(UnitType::Length(UnitLen::Unknown)), meta: vec![], }; - assert_eq!(len.human_friendly_type(), "number(Length)".to_string()); + assert_eq!(len.human_friendly_type(), "a number (Length)".to_string()); let unknown = KclValue::Number { value: 1.0, ty: NumericType::Unknown, meta: vec![], }; - assert_eq!(unknown.human_friendly_type(), "number(unknown units)".to_string()); + assert_eq!(unknown.human_friendly_type(), "a number with unknown units".to_string()); let mm = KclValue::Number { value: 1.0, ty: NumericType::Known(UnitType::Length(UnitLen::Mm)), meta: vec![], }; - assert_eq!(mm.human_friendly_type(), "number(mm)".to_string()); + assert_eq!(mm.human_friendly_type(), "a number (mm)".to_string()); let array1_mm = KclValue::HomArray { value: vec![mm.clone()], @@ -680,7 +676,7 @@ mod tests { }; assert_eq!( array1_mm.human_friendly_type(), - "array of number(mm) with 1 value".to_string() + "an array of `number(mm)` with 1 value".to_string() ); let array2_mm = KclValue::HomArray { @@ -689,7 +685,7 @@ mod tests { }; assert_eq!( array2_mm.human_friendly_type(), - "array of number(mm), number(mm) with 2 values".to_string() + "an array of `number(mm)`, `number(mm)`".to_string() ); let array3_mm = KclValue::HomArray { @@ -698,7 +694,7 @@ mod tests { }; assert_eq!( array3_mm.human_friendly_type(), - "array of number(mm), number(mm), number(mm) with 3 values".to_string() + "an array of `number(mm)`, `number(mm)`, `number(mm)`".to_string() ); let inches = KclValue::Number { @@ -712,14 +708,14 @@ mod tests { }; assert_eq!( array4.human_friendly_type(), - "array of number(mm), number(mm), number(in), ... with 4 values".to_string() + "an array of `number(mm)`, `number(mm)`, `number(in)`, ... with 4 values".to_string() ); let empty_array = KclValue::HomArray { value: vec![], ty: RuntimeType::any(), }; - assert_eq!(empty_array.human_friendly_type(), "[any; 0]".to_string()); + assert_eq!(empty_array.human_friendly_type(), "an empty array".to_string()); let array_nested = KclValue::HomArray { value: vec![array2_mm.clone()], @@ -727,7 +723,7 @@ mod tests { }; assert_eq!( array_nested.human_friendly_type(), - "array of [any; 2] with 1 value".to_string() + "an array of `[any; 2]` with 1 value".to_string() ); } } diff --git a/rust/kcl-lib/src/execution/mod.rs b/rust/kcl-lib/src/execution/mod.rs index 4d87f7154..fee8996b5 100644 --- a/rust/kcl-lib/src/execution/mod.rs +++ b/rust/kcl-lib/src/execution/mod.rs @@ -1931,13 +1931,13 @@ notNull = !myNull "#; assert_eq!( parse_execute(code1).await.unwrap_err().message(), - "Cannot apply unary operator ! to non-boolean value: number(default units)", + "Cannot apply unary operator ! to non-boolean value: a number", ); let code2 = "notZero = !0"; assert_eq!( parse_execute(code2).await.unwrap_err().message(), - "Cannot apply unary operator ! to non-boolean value: number(default units)", + "Cannot apply unary operator ! to non-boolean value: a number", ); let code3 = r#" @@ -1945,7 +1945,7 @@ notEmptyString = !"" "#; assert_eq!( parse_execute(code3).await.unwrap_err().message(), - "Cannot apply unary operator ! to non-boolean value: string", + "Cannot apply unary operator ! to non-boolean value: a string", ); let code4 = r#" @@ -1954,7 +1954,7 @@ notMember = !obj.a "#; assert_eq!( parse_execute(code4).await.unwrap_err().message(), - "Cannot apply unary operator ! to non-boolean value: number(default units)", + "Cannot apply unary operator ! to non-boolean value: a number", ); let code5 = " @@ -1962,7 +1962,7 @@ a = [] notArray = !a"; assert_eq!( parse_execute(code5).await.unwrap_err().message(), - "Cannot apply unary operator ! to non-boolean value: [any; 0]", + "Cannot apply unary operator ! to non-boolean value: an empty array", ); let code6 = " @@ -1970,7 +1970,7 @@ x = {} notObject = !x"; assert_eq!( parse_execute(code6).await.unwrap_err().message(), - "Cannot apply unary operator ! to non-boolean value: { }", + "Cannot apply unary operator ! to non-boolean value: an object", ); let code7 = " @@ -1996,7 +1996,7 @@ notTagDeclarator = !myTagDeclarator"; assert!( tag_declarator_err .message() - .starts_with("Cannot apply unary operator ! to non-boolean value: tag"), + .starts_with("Cannot apply unary operator ! to non-boolean value: a tag declarator"), "Actual error: {:?}", tag_declarator_err ); @@ -2010,7 +2010,7 @@ notTagIdentifier = !myTag"; assert!( tag_identifier_err .message() - .starts_with("Cannot apply unary operator ! to non-boolean value: tag"), + .starts_with("Cannot apply unary operator ! to non-boolean value: a tag identifier"), "Actual error: {:?}", tag_identifier_err ); diff --git a/rust/kcl-lib/src/execution/state.rs b/rust/kcl-lib/src/execution/state.rs index 989059456..e799f5488 100644 --- a/rust/kcl-lib/src/execution/state.rs +++ b/rust/kcl-lib/src/execution/state.rs @@ -145,6 +145,17 @@ impl ExecState { self.global.errors.push(e); } + pub fn clear_units_warnings(&mut self, source_range: &SourceRange) { + self.global.errors = std::mem::take(&mut self.global.errors) + .into_iter() + .filter(|e| { + e.severity != Severity::Warning + || !source_range.contains_range(&e.source_range) + || e.tag != crate::errors::Tag::UnknownNumericUnits + }) + .collect(); + } + pub fn errors(&self) -> &[CompilationError] { &self.global.errors } diff --git a/rust/kcl-lib/src/execution/types.rs b/rust/kcl-lib/src/execution/types.rs index c4f4d911f..b0684f36b 100644 --- a/rust/kcl-lib/src/execution/types.rs +++ b/rust/kcl-lib/src/execution/types.rs @@ -438,7 +438,7 @@ impl fmt::Display for PrimitiveType { PrimitiveType::Any => write!(f, "any"), PrimitiveType::Number(NumericType::Known(unit)) => write!(f, "number({unit})"), PrimitiveType::Number(NumericType::Unknown) => write!(f, "number(unknown units)"), - PrimitiveType::Number(NumericType::Default { .. }) => write!(f, "number(default units)"), + PrimitiveType::Number(NumericType::Default { .. }) => write!(f, "number"), PrimitiveType::Number(NumericType::Any) => write!(f, "number(any units)"), PrimitiveType::String => write!(f, "string"), PrimitiveType::Boolean => write!(f, "bool"), @@ -453,8 +453,8 @@ impl fmt::Display for PrimitiveType { PrimitiveType::Axis2d => write!(f, "Axis2d"), PrimitiveType::Axis3d => write!(f, "Axis3d"), PrimitiveType::Helix => write!(f, "Helix"), - PrimitiveType::ImportedGeometry => write!(f, "imported geometry"), - PrimitiveType::Function => write!(f, "function"), + PrimitiveType::ImportedGeometry => write!(f, "ImportedGeometry"), + PrimitiveType::Function => write!(f, "fn"), } } } @@ -1508,6 +1508,23 @@ impl KclValue { KclValue::Module { .. } | KclValue::KclNone { .. } | KclValue::Type { .. } => None, } } + + pub fn principal_type_string(&self) -> String { + if let Some(ty) = self.principal_type() { + return format!("`{ty}`"); + } + + match self { + KclValue::Module { .. } => "module", + KclValue::KclNone { .. } => "none", + KclValue::Type { .. } => "type", + _ => { + debug_assert!(false); + "" + } + } + .to_owned() + } } #[cfg(test)] diff --git a/rust/kcl-lib/src/lsp/mod.rs b/rust/kcl-lib/src/lsp/mod.rs index 2058305a7..d66af6a99 100644 --- a/rust/kcl-lib/src/lsp/mod.rs +++ b/rust/kcl-lib/src/lsp/mod.rs @@ -47,7 +47,7 @@ impl Tag { match self { Tag::Deprecated => Some(vec![DiagnosticTag::DEPRECATED]), Tag::Unnecessary => Some(vec![DiagnosticTag::UNNECESSARY]), - Tag::None => None, + Tag::UnknownNumericUnits | Tag::None => None, } } } diff --git a/rust/kcl-lib/src/lsp/tests.rs b/rust/kcl-lib/src/lsp/tests.rs index 607efa9b2..0ff6de881 100644 --- a/rust/kcl-lib/src/lsp/tests.rs +++ b/rust/kcl-lib/src/lsp/tests.rs @@ -950,7 +950,7 @@ startSketchOn(XY) match hover.unwrap().contents { tower_lsp::lsp_types::HoverContents::Markup(tower_lsp::lsp_types::MarkupContent { value, .. }) => { - assert!(value.contains("foo: number(default units) = 42")); + assert!(value.contains("foo: number = 42")); } _ => unreachable!(), } @@ -3900,7 +3900,7 @@ startSketchOn(XY) match hover.unwrap().contents { tower_lsp::lsp_types::HoverContents::Markup(tower_lsp::lsp_types::MarkupContent { value, .. }) => { - assert!(value.contains("foo: number(default units) = 42")); + assert!(value.contains("foo: number = 42")); } _ => unreachable!(), } diff --git a/rust/kcl-lib/src/parsing/ast/types/mod.rs b/rust/kcl-lib/src/parsing/ast/types/mod.rs index bce830e32..a03d19766 100644 --- a/rust/kcl-lib/src/parsing/ast/types/mod.rs +++ b/rust/kcl-lib/src/parsing/ast/types/mod.rs @@ -3174,6 +3174,19 @@ impl PrimitiveType { _ => None, } } + + fn display_multiple(&self) -> String { + match self { + PrimitiveType::Any => "values".to_owned(), + PrimitiveType::Number(_) => "numbers".to_owned(), + PrimitiveType::String => "strings".to_owned(), + PrimitiveType::Boolean => "bools".to_owned(), + PrimitiveType::ImportedGeometry => "imported geometries".to_owned(), + PrimitiveType::Function(_) => "functions".to_owned(), + PrimitiveType::Named { id } => format!("`{}`s", id.name), + PrimitiveType::Tag => "tags".to_owned(), + } + } } impl fmt::Display for PrimitiveType { @@ -3264,6 +3277,53 @@ pub enum Type { }, } +impl Type { + pub fn human_friendly_type(&self) -> String { + match self { + Type::Primitive(ty) => format!("a value with type `{ty}`"), + Type::Array { + ty, + len: ArrayLen::None | ArrayLen::Minimum(0), + } => { + format!("an array of {}", ty.display_multiple()) + } + Type::Array { + ty, + len: ArrayLen::Minimum(1), + } => format!("one or more {}", ty.display_multiple()), + Type::Array { + ty, + len: ArrayLen::Minimum(n), + } => { + format!("an array of {n} or more {}", ty.display_multiple()) + } + Type::Array { + ty, + len: ArrayLen::Known(n), + } => format!("an array of {n} {}", ty.display_multiple()), + Type::Union { tys } => tys + .iter() + .map(|t| t.human_friendly_type()) + .collect::>() + .join(" or "), + Type::Object { .. } => format!("an object with fields `{}`", self), + } + } + + fn display_multiple(&self) -> String { + match self { + Type::Primitive(ty) => ty.display_multiple(), + Type::Array { .. } => "arrays".to_owned(), + Type::Union { tys } => tys + .iter() + .map(|t| t.display_multiple()) + .collect::>() + .join(" or "), + Type::Object { .. } => format!("objects with fields `{self}`"), + } + } +} + impl fmt::Display for Type { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { diff --git a/rust/kcl-lib/src/std/args.rs b/rust/kcl-lib/src/std/args.rs index e72f5c72b..e8b05a130 100644 --- a/rust/kcl-lib/src/std/args.rs +++ b/rust/kcl-lib/src/std/args.rs @@ -160,7 +160,7 @@ impl Args { None => msg_base, Some(sugg) => format!("{msg_base}. {sugg}"), }; - if message.contains("one or more Solids or imported geometry but it's actually of type Sketch") { + if message.contains("one or more Solids or ImportedGeometry but it's actually of type Sketch") { message = format!("{message}. {ERROR_STRING_SKETCH_TO_SOLID_HELPER}"); } KclError::new_semantic(KclErrorDetails::new(message, arg.source_ranges())) @@ -257,7 +257,7 @@ impl Args { Some(sugg) => format!("{msg_base}. {sugg}"), }; - if message.contains("one or more Solids or imported geometry but it's actually of type Sketch") { + if message.contains("one or more Solids or ImportedGeometry but it's actually of type Sketch") { message = format!("{message}. {ERROR_STRING_SKETCH_TO_SOLID_HELPER}"); } KclError::new_semantic(KclErrorDetails::new(message, arg.source_ranges())) @@ -448,107 +448,12 @@ impl Args { } } -/// Types which impl this trait can be read out of the `Args` passed into a KCL function. -pub trait FromArgs<'a>: Sized { - /// Get this type from the args passed into a KCL function, at the given index in the argument list. - fn from_args(args: &'a Args, index: usize) -> Result; -} - /// Types which impl this trait can be extracted from a `KclValue`. pub trait FromKclValue<'a>: Sized { /// Try to convert a KclValue into this type. fn from_kcl_val(arg: &'a KclValue) -> Option; } -impl<'a, T> FromArgs<'a> for T -where - T: FromKclValue<'a> + Sized, -{ - fn from_args(args: &'a Args, i: usize) -> Result { - let Some(arg) = args.args.get(i) else { - return Err(KclError::new_semantic(KclErrorDetails::new( - format!("Expected an argument at index {i}"), - vec![args.source_range], - ))); - }; - let Some(val) = T::from_kcl_val(&arg.value) else { - return Err(KclError::new_semantic(KclErrorDetails::new( - format!( - "Argument at index {i} was supposed to be type {} but found {}", - tynm::type_name::(), - arg.value.human_friendly_type(), - ), - arg.source_ranges(), - ))); - }; - Ok(val) - } -} - -impl<'a, T> FromArgs<'a> for Option -where - T: FromKclValue<'a> + Sized, -{ - fn from_args(args: &'a Args, i: usize) -> Result { - let Some(arg) = args.args.get(i) else { return Ok(None) }; - if crate::parsing::ast::types::KclNone::from_kcl_val(&arg.value).is_some() { - return Ok(None); - } - let Some(val) = T::from_kcl_val(&arg.value) else { - return Err(KclError::new_semantic(KclErrorDetails::new( - format!( - "Argument at index {i} was supposed to be type Option<{}> but found {}", - tynm::type_name::(), - arg.value.human_friendly_type() - ), - arg.source_ranges(), - ))); - }; - Ok(Some(val)) - } -} - -impl<'a, A, B> FromArgs<'a> for (A, B) -where - A: FromArgs<'a>, - B: FromArgs<'a>, -{ - fn from_args(args: &'a Args, i: usize) -> Result { - let a = A::from_args(args, i)?; - let b = B::from_args(args, i + 1)?; - Ok((a, b)) - } -} - -impl<'a, A, B, C> FromArgs<'a> for (A, B, C) -where - A: FromArgs<'a>, - B: FromArgs<'a>, - C: FromArgs<'a>, -{ - fn from_args(args: &'a Args, i: usize) -> Result { - let a = A::from_args(args, i)?; - let b = B::from_args(args, i + 1)?; - let c = C::from_args(args, i + 2)?; - Ok((a, b, c)) - } -} -impl<'a, A, B, C, D> FromArgs<'a> for (A, B, C, D) -where - A: FromArgs<'a>, - B: FromArgs<'a>, - C: FromArgs<'a>, - D: FromArgs<'a>, -{ - fn from_args(args: &'a Args, i: usize) -> Result { - let a = A::from_args(args, i)?; - let b = B::from_args(args, i + 1)?; - let c = C::from_args(args, i + 2)?; - let d = D::from_args(args, i + 3)?; - Ok((a, b, c, d)) - } -} - impl<'a> FromKclValue<'a> for TagNode { fn from_kcl_val(arg: &'a KclValue) -> Option { arg.get_tag_declarator().ok() diff --git a/rust/kcl-lib/std/math.kcl b/rust/kcl-lib/std/math.kcl index 327815012..25a28a243 100644 --- a/rust/kcl-lib/std/math.kcl +++ b/rust/kcl-lib/std/math.kcl @@ -12,8 +12,8 @@ import Point2d from "std::types" /// from radians. Therefore, `PI` is treated a bit specially by KCL and always has unknown units. This /// means that if you use `PI`, you will need to give KCL some extra information about the units of numbers. /// Usually you should use type ascription on the result of calculations, e.g., `(2 * PI): number(rad)`. -/// You might prefer to use `units::toRadians` or `units::toDegrees` to convert between angles with -/// different units. +/// It is better to use `units::toRadians` or `units::toDegrees` to convert between angles with +/// different units where possible. /// /// ``` /// circumference = 70 diff --git a/rust/kcl-lib/tests/argument_error/execution_error.snap b/rust/kcl-lib/tests/argument_error/execution_error.snap index 0815769c6..ff9df8636 100644 --- a/rust/kcl-lib/tests/argument_error/execution_error.snap +++ b/rust/kcl-lib/tests/argument_error/execution_error.snap @@ -4,8 +4,8 @@ description: Error from executing argument_error.kcl --- KCL Semantic error - × semantic: f requires a value with type `fn(any): any`, but found array of - │ number(default units), number(default units) with 2 values + × semantic: f requires a value with type `fn(any): any`, but found an array + │ of `number`, `number` (with type `[any; 2]`). ╭─[5:1] 4 │ 5 │ map(f, f = [0, 1]) @@ -15,8 +15,8 @@ KCL Semantic error ╰──── ╰─▶ KCL Semantic error - × semantic: f requires a value with type `fn(any): any`, but found - │ array of number(default units), number(default units) with 2 values + × semantic: f requires a value with type `fn(any): any`, but found an + │ array of `number`, `number` (with type `[any; 2]`). ╭─[5:12] 4 │ 5 │ map(f, f = [0, 1]) diff --git a/rust/kcl-lib/tests/array_elem_pop_empty_fail/execution_error.snap b/rust/kcl-lib/tests/array_elem_pop_empty_fail/execution_error.snap index de1f7061c..affdb79cf 100644 --- a/rust/kcl-lib/tests/array_elem_pop_empty_fail/execution_error.snap +++ b/rust/kcl-lib/tests/array_elem_pop_empty_fail/execution_error.snap @@ -4,8 +4,8 @@ description: Error from executing array_elem_pop_empty_fail.kcl --- KCL Semantic error - × semantic: The input argument of `pop` requires a value with type `[any; - │ 1+]`, but found [any; 0] + × semantic: The input argument of `pop` requires one or more values (`[any; + │ 1+]`), but found an empty array (with type `[any; 0]`). ╭─[2:8] 1 │ arr = [] 2 │ fail = pop(arr) @@ -15,8 +15,8 @@ KCL Semantic error ╰──── ╰─▶ KCL Semantic error - × semantic: The input argument of `pop` requires a value with type - │ `[any; 1+]`, but found [any; 0] + × semantic: The input argument of `pop` requires one or more values + │ (`[any; 1+]`), but found an empty array (with type `[any; 0]`). ╭─[2:12] 1 │ arr = [] 2 │ fail = pop(arr) diff --git a/rust/kcl-lib/tests/comparisons_multiple/execution_error.snap b/rust/kcl-lib/tests/comparisons_multiple/execution_error.snap index add75ca2b..967ff5038 100644 --- a/rust/kcl-lib/tests/comparisons_multiple/execution_error.snap +++ b/rust/kcl-lib/tests/comparisons_multiple/execution_error.snap @@ -4,7 +4,7 @@ description: Error from executing comparisons_multiple.kcl --- KCL Semantic error - × semantic: Expected a number, but found bool + × semantic: Expected a number, but found a boolean (`true` or `false`) ╭──── 1 │ assert(3 == 3 == 3, error = "this should not compile") · ───┬── diff --git a/rust/kcl-lib/tests/error_inside_fn_also_has_source_range_of_call_site_recursive/execution_error.snap b/rust/kcl-lib/tests/error_inside_fn_also_has_source_range_of_call_site_recursive/execution_error.snap index d3def6268..680162b0a 100644 --- a/rust/kcl-lib/tests/error_inside_fn_also_has_source_range_of_call_site_recursive/execution_error.snap +++ b/rust/kcl-lib/tests/error_inside_fn_also_has_source_range_of_call_site_recursive/execution_error.snap @@ -5,7 +5,8 @@ description: Error from executing error_inside_fn_also_has_source_range_of_call_ KCL Semantic error × semantic: The input argument of `startSketchOn` requires a value with type - │ `Solid | Plane`, but found string + │ `Solid` or a value with type `Plane` (`Solid | Plane`), but found a value + │ with type `string`. ╭─[3:9] 2 │ fn someNestedFunction(@something2) { 3 │ startSketchOn(something2) @@ -26,7 +27,8 @@ KCL Semantic error ├─▶ KCL Semantic error │ │ × semantic: The input argument of `startSketchOn` requires a value - │ │ with type `Solid | Plane`, but found string + │ │ with type `Solid` or a value with type `Plane` (`Solid | Plane`), + │ │ but found a value with type `string`. │ ╭─[3:23] │ 2 │ fn someNestedFunction(@something2) { │ 3 │ startSketchOn(something2) @@ -38,7 +40,8 @@ KCL Semantic error ├─▶ KCL Semantic error │ │ × semantic: The input argument of `startSketchOn` requires a value - │ │ with type `Solid | Plane`, but found string + │ │ with type `Solid` or a value with type `Plane` (`Solid | Plane`), + │ │ but found a value with type `string`. │ ╭─[3:9] │ 2 │ fn someNestedFunction(@something2) { │ 3 │ startSketchOn(something2) @@ -50,7 +53,8 @@ KCL Semantic error ╰─▶ KCL Semantic error × semantic: The input argument of `startSketchOn` requires a value - │ with type `Solid | Plane`, but found string + │ with type `Solid` or a value with type `Plane` (`Solid | Plane`), + │ but found a value with type `string`. ╭─[6:5] 5 │ 6 │ someNestedFunction(something) diff --git a/rust/kcl-lib/tests/invalid_member_object/execution_error.snap b/rust/kcl-lib/tests/invalid_member_object/execution_error.snap index 101f312c9..789c594cb 100644 --- a/rust/kcl-lib/tests/invalid_member_object/execution_error.snap +++ b/rust/kcl-lib/tests/invalid_member_object/execution_error.snap @@ -4,8 +4,7 @@ description: Error from executing invalid_member_object.kcl --- KCL Semantic error - × semantic: Only arrays can be indexed, but you're trying to index a - │ number(default units) + × semantic: Only arrays can be indexed, but you're trying to index a number ╭─[2:5] 1 │ num = 999 2 │ x = num[3]