From bfaec5c04e6cbb9768781e55f5a24f11e815737d Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Tue, 17 Jun 2025 09:03:13 +1200 Subject: [PATCH] Warn on inferred angle units Signed-off-by: Nick Cameron --- rust/kcl-lib/src/execution/annotations.rs | 4 +- rust/kcl-lib/src/execution/exec_ast.rs | 29 +++++++--- rust/kcl-lib/src/execution/state.rs | 6 +- rust/kcl-lib/src/execution/types.rs | 67 ++++++++++++++++++++--- rust/kcl-lib/src/std/args.rs | 27 +++++++-- rust/kcl-lib/src/std/math.rs | 14 ++--- rust/kcl-lib/src/std/sketch.rs | 20 ++++--- rust/kcl-lib/src/std/utils.rs | 2 +- 8 files changed, 126 insertions(+), 43 deletions(-) diff --git a/rust/kcl-lib/src/execution/annotations.rs b/rust/kcl-lib/src/execution/annotations.rs index c79be2808..c7c92fd2b 100644 --- a/rust/kcl-lib/src/execution/annotations.rs +++ b/rust/kcl-lib/src/execution/annotations.rs @@ -37,12 +37,14 @@ pub(crate) const WARNINGS: &str = "warnings"; pub(crate) const WARN_ALLOW: &str = "allow"; pub(crate) const WARN_DENY: &str = "deny"; pub(crate) const WARN_UNKNOWN_UNITS: &str = "unknownUnits"; +pub(crate) const WARN_ANGLE_UNITS: &str = "angleUnits"; pub(crate) const WARN_UNKNOWN_ATTR: &str = "unknownAttribute"; pub(crate) const WARN_MOD_RETURN_VALUE: &str = "moduleReturnValue"; pub(crate) const WARN_DEPRECATED: &str = "deprecated"; pub(crate) const WARN_IGNORED_Z_AXIS: &str = "ignoredZAxis"; -pub(super) const WARN_VALUES: [&str; 5] = [ +pub(super) const WARN_VALUES: [&str; 6] = [ WARN_UNKNOWN_UNITS, + WARN_ANGLE_UNITS, WARN_UNKNOWN_ATTR, WARN_MOD_RETURN_VALUE, WARN_DEPRECATED, diff --git a/rust/kcl-lib/src/execution/exec_ast.rs b/rust/kcl-lib/src/execution/exec_ast.rs index 6773f5533..5dcce9b81 100644 --- a/rust/kcl-lib/src/execution/exec_ast.rs +++ b/rust/kcl-lib/src/execution/exec_ast.rs @@ -49,9 +49,20 @@ impl ExecutorContext { for annotation in annotations { if annotation.name() == Some(annotations::SETTINGS) { if matches!(body_type, BodyType::Root) { - if exec_state.mod_local.settings.update_from_annotation(annotation)? { + let (updated_len, updated_angle) = + exec_state.mod_local.settings.update_from_annotation(annotation)?; + if updated_len { exec_state.mod_local.explicit_length_units = true; } + if updated_angle { + exec_state.warn( + CompilationError::err( + annotation.as_source_range(), + "Prefer to use explicit units for angles", + ), + annotations::WARN_ANGLE_UNITS, + ); + } } else { exec_state.err(CompilationError::err( annotation.as_source_range(), @@ -1204,12 +1215,12 @@ impl Node { let value = match self.operator { BinaryOperator::Add => { - let (l, r, ty) = NumericType::combine_eq_coerce(left, right); + let (l, r, ty) = NumericType::combine_eq_coerce(left, right, None); self.warn_on_unknown(&ty, "Adding", exec_state); KclValue::Number { value: l + r, meta, ty } } BinaryOperator::Sub => { - let (l, r, ty) = NumericType::combine_eq_coerce(left, right); + let (l, r, ty) = NumericType::combine_eq_coerce(left, right, None); self.warn_on_unknown(&ty, "Subtracting", exec_state); KclValue::Number { value: l - r, meta, ty } } @@ -1234,32 +1245,32 @@ impl Node { ty: exec_state.current_default_units(), }, BinaryOperator::Neq => { - let (l, r, ty) = NumericType::combine_eq(left, right); + let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range()); self.warn_on_unknown(&ty, "Comparing", exec_state); KclValue::Bool { value: l != r, meta } } BinaryOperator::Gt => { - let (l, r, ty) = NumericType::combine_eq(left, right); + let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range()); self.warn_on_unknown(&ty, "Comparing", exec_state); KclValue::Bool { value: l > r, meta } } BinaryOperator::Gte => { - let (l, r, ty) = NumericType::combine_eq(left, right); + let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range()); self.warn_on_unknown(&ty, "Comparing", exec_state); KclValue::Bool { value: l >= r, meta } } BinaryOperator::Lt => { - let (l, r, ty) = NumericType::combine_eq(left, right); + let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range()); self.warn_on_unknown(&ty, "Comparing", exec_state); KclValue::Bool { value: l < r, meta } } BinaryOperator::Lte => { - let (l, r, ty) = NumericType::combine_eq(left, right); + let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range()); self.warn_on_unknown(&ty, "Comparing", exec_state); KclValue::Bool { value: l <= r, meta } } BinaryOperator::Eq => { - let (l, r, ty) = NumericType::combine_eq(left, right); + let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range()); self.warn_on_unknown(&ty, "Comparing", exec_state); KclValue::Bool { value: l == r, meta } } diff --git a/rust/kcl-lib/src/execution/state.rs b/rust/kcl-lib/src/execution/state.rs index fce6fd4f5..faaaaa624 100644 --- a/rust/kcl-lib/src/execution/state.rs +++ b/rust/kcl-lib/src/execution/state.rs @@ -542,10 +542,11 @@ impl MetaSettings { pub(crate) fn update_from_annotation( &mut self, annotation: &crate::parsing::ast::types::Node, - ) -> Result { + ) -> Result<(bool, bool), KclError> { let properties = annotations::expect_properties(annotations::SETTINGS, annotation)?; let mut updated_len = false; + let mut updated_angle = false; for p in properties { match &*p.inner.key.name { annotations::SETTINGS_UNIT_LENGTH => { @@ -558,6 +559,7 @@ impl MetaSettings { let value = annotations::expect_ident(&p.inner.value)?; let value = types::UnitAngle::from_str(value, annotation.as_source_range())?; self.default_angle_units = value; + updated_angle = true; } annotations::SETTINGS_VERSION => { let value = annotations::expect_number(&p.inner.value)?; @@ -576,6 +578,6 @@ impl MetaSettings { } } - Ok(updated_len) + Ok((updated_len, updated_angle)) } } diff --git a/rust/kcl-lib/src/execution/types.rs b/rust/kcl-lib/src/execution/types.rs index 89784be56..4bde96ec7 100644 --- a/rust/kcl-lib/src/execution/types.rs +++ b/rust/kcl-lib/src/execution/types.rs @@ -506,7 +506,12 @@ impl NumericType { /// /// This combinator function is suitable for comparisons where uncertainty should /// be handled by the user. - pub fn combine_eq(a: TyF64, b: TyF64) -> (f64, f64, NumericType) { + pub fn combine_eq( + a: TyF64, + b: TyF64, + exec_state: &mut ExecState, + source_range: SourceRange, + ) -> (f64, f64, NumericType) { use NumericType::*; match (a.ty, b.ty) { (at, bt) if at == bt => (a.n, b.n, at), @@ -521,8 +526,24 @@ impl NumericType { } (t @ Known(UnitType::Length(l1)), Default { len: l2, .. }) if l1 == l2 => (a.n, b.n, t), (Default { len: l1, .. }, t @ Known(UnitType::Length(l2))) if l1 == l2 => (a.n, b.n, t), - (t @ Known(UnitType::Angle(a1)), Default { angle: a2, .. }) if a1 == a2 => (a.n, b.n, t), - (Default { angle: a1, .. }, t @ Known(UnitType::Angle(a2))) if a1 == a2 => (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, + ); + } + (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, + ); + } + (a.n, b.n, t) + } _ => (a.n, b.n, Unknown), } @@ -535,7 +556,11 @@ impl NumericType { /// coerced together, for example two arguments to the same function or two numbers in an array being used as a point. /// /// Prefer to use `combine_eq` if possible since using that prioritises correctness over ergonomics. - pub fn combine_eq_coerce(a: TyF64, b: TyF64) -> (f64, f64, NumericType) { + pub fn combine_eq_coerce( + a: TyF64, + b: TyF64, + for_errs: Option<(&mut ExecState, SourceRange)>, + ) -> (f64, f64, NumericType) { use NumericType::*; match (a.ty, b.ty) { (at, bt) if at == bt => (a.n, b.n, at), @@ -553,8 +578,28 @@ impl NumericType { (t @ Known(UnitType::Length(l1)), Default { len: l2, .. }) => (a.n, l2.adjust_to(b.n, l1).0, t), (Default { len: l1, .. }, t @ Known(UnitType::Length(l2))) => (l1.adjust_to(a.n, l2).0, b.n, t), - (t @ Known(UnitType::Angle(a1)), Default { angle: a2, .. }) => (a.n, a2.adjust_to(b.n, a1).0, t), - (Default { angle: a1, .. }, t @ Known(UnitType::Angle(a2))) => (a1.adjust_to(a.n, a2).0, b.n, t), + (t @ Known(UnitType::Angle(a1)), Default { angle: a2, .. }) => { + if let Some((exec_state, source_range)) = for_errs { + if b.n != 0.0 { + exec_state.warn( + CompilationError::err(source_range, "Prefer to use explicit units for angles"), + annotations::WARN_ANGLE_UNITS, + ); + } + } + (a.n, a2.adjust_to(b.n, a1).0, t) + } + (Default { angle: a1, .. }, t @ Known(UnitType::Angle(a2))) => { + if let Some((exec_state, source_range)) = for_errs { + if a.n != 0.0 { + exec_state.warn( + CompilationError::err(source_range, "Prefer to use explicit units for angles"), + annotations::WARN_ANGLE_UNITS, + ); + } + } + (a1.adjust_to(a.n, a2).0, b.n, t) + } (Known(_), Known(_)) | (Default { .. }, Default { .. }) | (_, Unknown) | (Unknown, _) => { (a.n, b.n, Unknown) @@ -2350,14 +2395,18 @@ b = 180 / PI * a + 360 #[tokio::test(flavor = "multi_thread")] async fn cos_coercions() { let program = r#" -a = cos(units::toRadians(30)) +a = cos(units::toRadians(30deg)) b = 3 / a c = cos(30deg) -d = cos(30) +d = cos(1rad) "#; let result = parse_execute(program).await.unwrap(); - assert!(result.exec_state.errors().is_empty()); + assert!( + result.exec_state.errors().is_empty(), + "{:?}", + result.exec_state.errors() + ); assert_value_and_type("a", &result, 1.0, NumericType::default()); assert_value_and_type("b", &result, 3.0, NumericType::default()); diff --git a/rust/kcl-lib/src/std/args.rs b/rust/kcl-lib/src/std/args.rs index 0afe72cec..39bea4e99 100644 --- a/rust/kcl-lib/src/std/args.rs +++ b/rust/kcl-lib/src/std/args.rs @@ -9,6 +9,7 @@ pub use crate::execution::fn_call::Args; use crate::{ errors::{KclError, KclErrorDetails}, execution::{ + annotations, kcl_value::FunctionSource, types::{NumericType, PrimitiveType, RuntimeType, UnitAngle, UnitLen, UnitType}, ExecState, ExtrudeSurface, Helix, KclObjectFields, KclValue, Metadata, PlaneInfo, Sketch, SketchSurface, Solid, @@ -21,7 +22,7 @@ use crate::{ sketch::FaceTag, sweep::SweepPath, }, - ModuleId, + CompilationError, ModuleId, }; const ERROR_STRING_SKETCH_TO_SOLID_HELPER: &str = @@ -56,9 +57,17 @@ impl TyF64 { len.adjust_to(self.n, units).0 } - pub fn to_degrees(&self) -> f64 { + pub fn to_degrees(&self, exec_state: &mut ExecState, source_range: SourceRange) -> f64 { let angle = match self.ty { - NumericType::Default { angle, .. } => angle, + NumericType::Default { angle, .. } => { + if self.n != 0.0 { + exec_state.warn( + CompilationError::err(source_range, "Prefer to use explicit units for angles"), + annotations::WARN_ANGLE_UNITS, + ); + } + angle + } NumericType::Known(UnitType::Angle(angle)) => angle, _ => unreachable!(), }; @@ -68,9 +77,17 @@ impl TyF64 { angle.adjust_to(self.n, UnitAngle::Degrees).0 } - pub fn to_radians(&self) -> f64 { + pub fn to_radians(&self, exec_state: &mut ExecState, source_range: SourceRange) -> f64 { let angle = match self.ty { - NumericType::Default { angle, .. } => angle, + NumericType::Default { angle, .. } => { + if self.n != 0.0 { + exec_state.warn( + CompilationError::err(source_range, "Prefer to use explicit units for angles"), + annotations::WARN_ANGLE_UNITS, + ); + } + angle + } NumericType::Known(UnitType::Angle(angle)) => angle, _ => unreachable!(), }; diff --git a/rust/kcl-lib/src/std/math.rs b/rust/kcl-lib/src/std/math.rs index 5fc14e69e..aa50bcd10 100644 --- a/rust/kcl-lib/src/std/math.rs +++ b/rust/kcl-lib/src/std/math.rs @@ -34,21 +34,21 @@ pub async fn rem(exec_state: &mut ExecState, args: Args) -> Result Result { let num: TyF64 = args.get_unlabeled_kw_arg("input", &RuntimeType::angle(), exec_state)?; - let num = num.to_radians(); + let num = num.to_radians(exec_state, args.source_range); Ok(args.make_user_val_from_f64_with_type(TyF64::new(libm::cos(num), exec_state.current_default_units()))) } /// Compute the sine of a number (in radians). pub async fn sin(exec_state: &mut ExecState, args: Args) -> Result { let num: TyF64 = args.get_unlabeled_kw_arg("input", &RuntimeType::angle(), exec_state)?; - let num = num.to_radians(); + let num = num.to_radians(exec_state, args.source_range); Ok(args.make_user_val_from_f64_with_type(TyF64::new(libm::sin(num), exec_state.current_default_units()))) } /// Compute the tangent of a number (in radians). pub async fn tan(exec_state: &mut ExecState, args: Args) -> Result { let num: TyF64 = args.get_unlabeled_kw_arg("input", &RuntimeType::angle(), exec_state)?; - let num = num.to_radians(); + let num = num.to_radians(exec_state, args.source_range); Ok(args.make_user_val_from_f64_with_type(TyF64::new(libm::tan(num), exec_state.current_default_units()))) } @@ -190,7 +190,7 @@ pub async fn atan(exec_state: &mut ExecState, args: Args) -> Result Result { let y = args.get_kw_arg("y", &RuntimeType::length(), exec_state)?; let x = args.get_kw_arg("x", &RuntimeType::length(), exec_state)?; - let (y, x, _) = NumericType::combine_eq_coerce(y, x); + let (y, x, _) = NumericType::combine_eq_coerce(y, x, Some((exec_state, args.source_range))); let result = libm::atan2(y, x); Ok(args.make_user_val_from_f64_with_type(TyF64::new(result, NumericType::radians()))) @@ -237,7 +237,7 @@ pub async fn ln(exec_state: &mut ExecState, args: Args) -> Result Result { let hypotenuse: TyF64 = args.get_kw_arg("hypotenuse", &RuntimeType::length(), exec_state)?; let leg: TyF64 = args.get_kw_arg("leg", &RuntimeType::length(), exec_state)?; - let (hypotenuse, leg, ty) = NumericType::combine_eq_coerce(hypotenuse, leg); + let (hypotenuse, leg, ty) = NumericType::combine_eq_coerce(hypotenuse, leg, Some((exec_state, args.source_range))); let result = (hypotenuse.powi(2) - f64::min(hypotenuse.abs(), leg.abs()).powi(2)).sqrt(); Ok(KclValue::from_number_with_type(result, ty, vec![args.into()])) } @@ -246,7 +246,7 @@ pub async fn leg_length(exec_state: &mut ExecState, args: Args) -> Result Result { let hypotenuse: TyF64 = args.get_kw_arg("hypotenuse", &RuntimeType::length(), exec_state)?; let leg: TyF64 = args.get_kw_arg("leg", &RuntimeType::length(), exec_state)?; - let (hypotenuse, leg, _ty) = NumericType::combine_eq_coerce(hypotenuse, leg); + let (hypotenuse, leg, _ty) = NumericType::combine_eq_coerce(hypotenuse, leg, Some((exec_state, args.source_range))); let result = libm::acos(leg.min(hypotenuse) / hypotenuse).to_degrees(); Ok(KclValue::from_number_with_type( result, @@ -259,7 +259,7 @@ pub async fn leg_angle_x(exec_state: &mut ExecState, args: Args) -> Result Result { let hypotenuse: TyF64 = args.get_kw_arg("hypotenuse", &RuntimeType::length(), exec_state)?; let leg: TyF64 = args.get_kw_arg("leg", &RuntimeType::length(), exec_state)?; - let (hypotenuse, leg, _ty) = NumericType::combine_eq_coerce(hypotenuse, leg); + let (hypotenuse, leg, _ty) = NumericType::combine_eq_coerce(hypotenuse, leg, Some((exec_state, args.source_range))); let result = libm::asin(leg.min(hypotenuse) / hypotenuse).to_degrees(); Ok(KclValue::from_number_with_type( result, diff --git a/rust/kcl-lib/src/std/sketch.rs b/rust/kcl-lib/src/std/sketch.rs index c0c74360c..da2405342 100644 --- a/rust/kcl-lib/src/std/sketch.rs +++ b/rust/kcl-lib/src/std/sketch.rs @@ -132,6 +132,8 @@ async fn inner_involute_circular( args: Args, ) -> Result { let id = exec_state.next_uuid(); + let angle_deg = angle.to_degrees(exec_state, args.source_range); + let angle_rad = angle.to_radians(exec_state, args.source_range); exec_state .batch_modeling_cmd( @@ -141,7 +143,7 @@ async fn inner_involute_circular( segment: PathSegment::CircularInvolute { start_radius: LengthUnit(start_radius.to_mm()), end_radius: LengthUnit(end_radius.to_mm()), - angle: Angle::from_degrees(angle.to_degrees()), + angle: Angle::from_degrees(angle_deg), reverse: reverse.unwrap_or_default(), }, }), @@ -157,11 +159,11 @@ async fn inner_involute_circular( let theta = f64::sqrt(end_radius * end_radius - start_radius * start_radius) / start_radius; let (x, y) = involute_curve(start_radius, theta); - end.x = x * libm::cos(angle.to_radians()) - y * libm::sin(angle.to_radians()); - end.y = x * libm::sin(angle.to_radians()) + y * libm::cos(angle.to_radians()); + end.x = x * libm::cos(angle_rad) - y * libm::sin(angle_rad); + end.y = x * libm::sin(angle_rad) + y * libm::cos(angle_rad); - end.x -= start_radius * libm::cos(angle.to_radians()); - end.y -= start_radius * libm::sin(angle.to_radians()); + end.x -= start_radius * libm::cos(angle_rad); + end.y -= start_radius * libm::sin(angle_rad); if reverse.unwrap_or_default() { end.x = -end.x; @@ -718,7 +720,7 @@ pub async fn inner_angled_line_that_intersects( point_to_len_unit(path.get_to(), from.units), ], offset.map(|t| t.to_length_units(from.units)).unwrap_or_default(), - angle.to_degrees(), + angle.to_degrees(exec_state, args.source_range), from.ignore_units(), ); let to = [ @@ -1256,8 +1258,8 @@ pub async fn relative_arc( radius: TyF64, tag: Option, ) -> Result { - let a_start = Angle::from_degrees(angle_start.to_degrees()); - let a_end = Angle::from_degrees(angle_end.to_degrees()); + let a_start = Angle::from_degrees(angle_start.to_degrees(exec_state, args.source_range)); + let a_end = Angle::from_degrees(angle_end.to_degrees(exec_state, args.source_range)); let radius = radius.to_length_units(from.units); let (center, end) = arc_center_and_end(from.ignore_units(), a_start, a_end, radius); if a_start == a_end { @@ -1409,7 +1411,7 @@ async fn inner_tangential_arc_radius_angle( let (center, to, ccw) = match data { TangentialArcData::RadiusAndOffset { radius, offset } => { // KCL stdlib types use degrees. - let offset = Angle::from_degrees(offset.to_degrees()); + let offset = Angle::from_degrees(offset.to_degrees(exec_state, args.source_range)); // Calculate the end point from the angle and radius. // atan2 outputs radians. diff --git a/rust/kcl-lib/src/std/utils.rs b/rust/kcl-lib/src/std/utils.rs index ea81f706f..ba6d3e304 100644 --- a/rust/kcl-lib/src/std/utils.rs +++ b/rust/kcl-lib/src/std/utils.rs @@ -6,7 +6,7 @@ use super::args::TyF64; use crate::execution::types::{NumericType, UnitLen}; pub(crate) fn untype_point(p: [TyF64; 2]) -> ([f64; 2], NumericType) { - let (x, y, ty) = NumericType::combine_eq_coerce(p[0].clone(), p[1].clone()); + let (x, y, ty) = NumericType::combine_eq_coerce(p[0].clone(), p[1].clone(), None); ([x, y], ty) }