Skip to content

Commit a0ed289

Browse files
nrcnrc
committed
Warn on inferred angle units
Signed-off-by: Nick Cameron <nrc@ncameron.org>
1 parent 38402c7 commit a0ed289

File tree

8 files changed

+126
-43
lines changed

8 files changed

+126
-43
lines changed

‎rust/kcl-lib/src/execution/annotations.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,14 @@ pub(crate) const WARNINGS: &str = "warnings";
3737
pub(crate) const WARN_ALLOW: &str = "allow";
3838
pub(crate) const WARN_DENY: &str = "deny";
3939
pub(crate) const WARN_UNKNOWN_UNITS: &str = "unknownUnits";
40+
pub(crate) const WARN_ANGLE_UNITS: &str = "angleUnits";
4041
pub(crate) const WARN_UNKNOWN_ATTR: &str = "unknownAttribute";
4142
pub(crate) const WARN_MOD_RETURN_VALUE: &str = "moduleReturnValue";
4243
pub(crate) const WARN_DEPRECATED: &str = "deprecated";
4344
pub(crate) const WARN_IGNORED_Z_AXIS: &str = "ignoredZAxis";
44-
pub(super) const WARN_VALUES: [&str; 5] = [
45+
pub(super) const WARN_VALUES: [&str; 6] = [
4546
WARN_UNKNOWN_UNITS,
47+
WARN_ANGLE_UNITS,
4648
WARN_UNKNOWN_ATTR,
4749
WARN_MOD_RETURN_VALUE,
4850
WARN_DEPRECATED,

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

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,20 @@ impl ExecutorContext {
4949
for annotation in annotations {
5050
if annotation.name() == Some(annotations::SETTINGS) {
5151
if matches!(body_type, BodyType::Root) {
52-
if exec_state.mod_local.settings.update_from_annotation(annotation)? {
52+
let (updated_len, updated_angle) =
53+
exec_state.mod_local.settings.update_from_annotation(annotation)?;
54+
if updated_len {
5355
exec_state.mod_local.explicit_length_units = true;
5456
}
57+
if updated_angle {
58+
exec_state.warn(
59+
CompilationError::err(
60+
annotation.as_source_range(),
61+
"Prefer to use explicit units for angles",
62+
),
63+
annotations::WARN_ANGLE_UNITS,
64+
);
65+
}
5566
} else {
5667
exec_state.err(CompilationError::err(
5768
annotation.as_source_range(),
@@ -1204,12 +1215,12 @@ impl Node<BinaryExpression> {
12041215

12051216
let value = match self.operator {
12061217
BinaryOperator::Add => {
1207-
let (l, r, ty) = NumericType::combine_eq_coerce(left, right);
1218+
let (l, r, ty) = NumericType::combine_eq_coerce(left, right, None);
12081219
self.warn_on_unknown(&ty, "Adding", exec_state);
12091220
KclValue::Number { value: l + r, meta, ty }
12101221
}
12111222
BinaryOperator::Sub => {
1212-
let (l, r, ty) = NumericType::combine_eq_coerce(left, right);
1223+
let (l, r, ty) = NumericType::combine_eq_coerce(left, right, None);
12131224
self.warn_on_unknown(&ty, "Subtracting", exec_state);
12141225
KclValue::Number { value: l - r, meta, ty }
12151226
}
@@ -1234,32 +1245,32 @@ impl Node<BinaryExpression> {
12341245
ty: exec_state.current_default_units(),
12351246
},
12361247
BinaryOperator::Neq => {
1237-
let (l, r, ty) = NumericType::combine_eq(left, right);
1248+
let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range());
12381249
self.warn_on_unknown(&ty, "Comparing", exec_state);
12391250
KclValue::Bool { value: l != r, meta }
12401251
}
12411252
BinaryOperator::Gt => {
1242-
let (l, r, ty) = NumericType::combine_eq(left, right);
1253+
let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range());
12431254
self.warn_on_unknown(&ty, "Comparing", exec_state);
12441255
KclValue::Bool { value: l > r, meta }
12451256
}
12461257
BinaryOperator::Gte => {
1247-
let (l, r, ty) = NumericType::combine_eq(left, right);
1258+
let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range());
12481259
self.warn_on_unknown(&ty, "Comparing", exec_state);
12491260
KclValue::Bool { value: l >= r, meta }
12501261
}
12511262
BinaryOperator::Lt => {
1252-
let (l, r, ty) = NumericType::combine_eq(left, right);
1263+
let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range());
12531264
self.warn_on_unknown(&ty, "Comparing", exec_state);
12541265
KclValue::Bool { value: l < r, meta }
12551266
}
12561267
BinaryOperator::Lte => {
1257-
let (l, r, ty) = NumericType::combine_eq(left, right);
1268+
let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range());
12581269
self.warn_on_unknown(&ty, "Comparing", exec_state);
12591270
KclValue::Bool { value: l <= r, meta }
12601271
}
12611272
BinaryOperator::Eq => {
1262-
let (l, r, ty) = NumericType::combine_eq(left, right);
1273+
let (l, r, ty) = NumericType::combine_eq(left, right, exec_state, self.as_source_range());
12631274
self.warn_on_unknown(&ty, "Comparing", exec_state);
12641275
KclValue::Bool { value: l == r, meta }
12651276
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -542,10 +542,11 @@ impl MetaSettings {
542542
pub(crate) fn update_from_annotation(
543543
&mut self,
544544
annotation: &crate::parsing::ast::types::Node<Annotation>,
545-
) -> Result<bool, KclError> {
545+
) -> Result<(bool, bool), KclError> {
546546
let properties = annotations::expect_properties(annotations::SETTINGS, annotation)?;
547547

548548
let mut updated_len = false;
549+
let mut updated_angle = false;
549550
for p in properties {
550551
match &*p.inner.key.name {
551552
annotations::SETTINGS_UNIT_LENGTH => {
@@ -558,6 +559,7 @@ impl MetaSettings {
558559
let value = annotations::expect_ident(&p.inner.value)?;
559560
let value = types::UnitAngle::from_str(value, annotation.as_source_range())?;
560561
self.default_angle_units = value;
562+
updated_angle = true;
561563
}
562564
annotations::SETTINGS_VERSION => {
563565
let value = annotations::expect_number(&p.inner.value)?;
@@ -576,6 +578,6 @@ impl MetaSettings {
576578
}
577579
}
578580

579-
Ok(updated_len)
581+
Ok((updated_len, updated_angle))
580582
}
581583
}

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

Lines changed: 58 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -506,7 +506,12 @@ impl NumericType {
506506
///
507507
/// This combinator function is suitable for comparisons where uncertainty should
508508
/// be handled by the user.
509-
pub fn combine_eq(a: TyF64, b: TyF64) -> (f64, f64, NumericType) {
509+
pub fn combine_eq(
510+
a: TyF64,
511+
b: TyF64,
512+
exec_state: &mut ExecState,
513+
source_range: SourceRange,
514+
) -> (f64, f64, NumericType) {
510515
use NumericType::*;
511516
match (a.ty, b.ty) {
512517
(at, bt) if at == bt => (a.n, b.n, at),
@@ -521,8 +526,24 @@ impl NumericType {
521526
}
522527
(t @ Known(UnitType::Length(l1)), Default { len: l2, .. }) if l1 == l2 => (a.n, b.n, t),
523528
(Default { len: l1, .. }, t @ Known(UnitType::Length(l2))) if l1 == l2 => (a.n, b.n, t),
524-
(t @ Known(UnitType::Angle(a1)), Default { angle: a2, .. }) if a1 == a2 => (a.n, b.n, t),
525-
(Default { angle: a1, .. }, t @ Known(UnitType::Angle(a2))) if a1 == a2 => (a.n, b.n, t),
529+
(t @ Known(UnitType::Angle(a1)), Default { angle: a2, .. }) if a1 == a2 => {
530+
if b.n != 0.0 {
531+
exec_state.warn(
532+
CompilationError::err(source_range, "Prefer to use explicit units for angles"),
533+
annotations::WARN_ANGLE_UNITS,
534+
);
535+
}
536+
(a.n, b.n, t)
537+
}
538+
(Default { angle: a1, .. }, t @ Known(UnitType::Angle(a2))) if a1 == a2 => {
539+
if a.n != 0.0 {
540+
exec_state.warn(
541+
CompilationError::err(source_range, "Prefer to use explicit units for angles"),
542+
annotations::WARN_ANGLE_UNITS,
543+
);
544+
}
545+
(a.n, b.n, t)
546+
}
526547

527548
_ => (a.n, b.n, Unknown),
528549
}
@@ -535,7 +556,11 @@ impl NumericType {
535556
/// coerced together, for example two arguments to the same function or two numbers in an array being used as a point.
536557
///
537558
/// Prefer to use `combine_eq` if possible since using that prioritises correctness over ergonomics.
538-
pub fn combine_eq_coerce(a: TyF64, b: TyF64) -> (f64, f64, NumericType) {
559+
pub fn combine_eq_coerce(
560+
a: TyF64,
561+
b: TyF64,
562+
for_errs: Option<(&mut ExecState, SourceRange)>,
563+
) -> (f64, f64, NumericType) {
539564
use NumericType::*;
540565
match (a.ty, b.ty) {
541566
(at, bt) if at == bt => (a.n, b.n, at),
@@ -553,8 +578,28 @@ impl NumericType {
553578

554579
(t @ Known(UnitType::Length(l1)), Default { len: l2, .. }) => (a.n, l2.adjust_to(b.n, l1).0, t),
555580
(Default { len: l1, .. }, t @ Known(UnitType::Length(l2))) => (l1.adjust_to(a.n, l2).0, b.n, t),
556-
(t @ Known(UnitType::Angle(a1)), Default { angle: a2, .. }) => (a.n, a2.adjust_to(b.n, a1).0, t),
557-
(Default { angle: a1, .. }, t @ Known(UnitType::Angle(a2))) => (a1.adjust_to(a.n, a2).0, b.n, t),
581+
(t @ Known(UnitType::Angle(a1)), Default { angle: a2, .. }) => {
582+
if let Some((exec_state, source_range)) = for_errs {
583+
if b.n != 0.0 {
584+
exec_state.warn(
585+
CompilationError::err(source_range, "Prefer to use explicit units for angles"),
586+
annotations::WARN_ANGLE_UNITS,
587+
);
588+
}
589+
}
590+
(a.n, a2.adjust_to(b.n, a1).0, t)
591+
}
592+
(Default { angle: a1, .. }, t @ Known(UnitType::Angle(a2))) => {
593+
if let Some((exec_state, source_range)) = for_errs {
594+
if a.n != 0.0 {
595+
exec_state.warn(
596+
CompilationError::err(source_range, "Prefer to use explicit units for angles"),
597+
annotations::WARN_ANGLE_UNITS,
598+
);
599+
}
600+
}
601+
(a1.adjust_to(a.n, a2).0, b.n, t)
602+
}
558603

559604
(Known(_), Known(_)) | (Default { .. }, Default { .. }) | (_, Unknown) | (Unknown, _) => {
560605
(a.n, b.n, Unknown)
@@ -2350,14 +2395,18 @@ b = 180 / PI * a + 360
23502395
#[tokio::test(flavor = "multi_thread")]
23512396
async fn cos_coercions() {
23522397
let program = r#"
2353-
a = cos(units::toRadians(30))
2398+
a = cos(units::toRadians(30deg))
23542399
b = 3 / a
23552400
c = cos(30deg)
2356-
d = cos(30)
2401+
d = cos(1rad)
23572402
"#;
23582403

23592404
let result = parse_execute(program).await.unwrap();
2360-
assert!(result.exec_state.errors().is_empty());
2405+
assert!(
2406+
result.exec_state.errors().is_empty(),
2407+
"{:?}",
2408+
result.exec_state.errors()
2409+
);
23612410

23622411
assert_value_and_type("a", &result, 1.0, NumericType::default());
23632412
assert_value_and_type("b", &result, 3.0, NumericType::default());

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

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ pub use crate::execution::fn_call::Args;
99
use crate::{
1010
errors::{KclError, KclErrorDetails},
1111
execution::{
12+
annotations,
1213
kcl_value::FunctionSource,
1314
types::{NumericType, PrimitiveType, RuntimeType, UnitAngle, UnitLen, UnitType},
1415
ExecState, ExtrudeSurface, Helix, KclObjectFields, KclValue, Metadata, PlaneInfo, Sketch, SketchSurface, Solid,
@@ -21,7 +22,7 @@ use crate::{
2122
sketch::FaceTag,
2223
sweep::SweepPath,
2324
},
24-
ModuleId,
25+
CompilationError, ModuleId,
2526
};
2627

2728
const ERROR_STRING_SKETCH_TO_SOLID_HELPER: &str =
@@ -56,9 +57,17 @@ impl TyF64 {
5657
len.adjust_to(self.n, units).0
5758
}
5859

59-
pub fn to_degrees(&self) -> f64 {
60+
pub fn to_degrees(&self, exec_state: &mut ExecState, source_range: SourceRange) -> f64 {
6061
let angle = match self.ty {
61-
NumericType::Default { angle, .. } => angle,
62+
NumericType::Default { angle, .. } => {
63+
if self.n != 0.0 {
64+
exec_state.warn(
65+
CompilationError::err(source_range, "Prefer to use explicit units for angles"),
66+
annotations::WARN_ANGLE_UNITS,
67+
);
68+
}
69+
angle
70+
}
6271
NumericType::Known(UnitType::Angle(angle)) => angle,
6372
_ => unreachable!(),
6473
};
@@ -68,9 +77,17 @@ impl TyF64 {
6877
angle.adjust_to(self.n, UnitAngle::Degrees).0
6978
}
7079

71-
pub fn to_radians(&self) -> f64 {
80+
pub fn to_radians(&self, exec_state: &mut ExecState, source_range: SourceRange) -> f64 {
7281
let angle = match self.ty {
73-
NumericType::Default { angle, .. } => angle,
82+
NumericType::Default { angle, .. } => {
83+
if self.n != 0.0 {
84+
exec_state.warn(
85+
CompilationError::err(source_range, "Prefer to use explicit units for angles"),
86+
annotations::WARN_ANGLE_UNITS,
87+
);
88+
}
89+
angle
90+
}
7491
NumericType::Known(UnitType::Angle(angle)) => angle,
7592
_ => unreachable!(),
7693
};

‎rust/kcl-lib/src/std/math.rs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,21 +34,21 @@ pub async fn rem(exec_state: &mut ExecState, args: Args) -> Result<KclValue, Kcl
3434
/// Compute the cosine of a number (in radians).
3535
pub async fn cos(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
3636
let num: TyF64 = args.get_unlabeled_kw_arg("input", &RuntimeType::angle(), exec_state)?;
37-
let num = num.to_radians();
37+
let num = num.to_radians(exec_state, args.source_range);
3838
Ok(args.make_user_val_from_f64_with_type(TyF64::new(libm::cos(num), exec_state.current_default_units())))
3939
}
4040

4141
/// Compute the sine of a number (in radians).
4242
pub async fn sin(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
4343
let num: TyF64 = args.get_unlabeled_kw_arg("input", &RuntimeType::angle(), exec_state)?;
44-
let num = num.to_radians();
44+
let num = num.to_radians(exec_state, args.source_range);
4545
Ok(args.make_user_val_from_f64_with_type(TyF64::new(libm::sin(num), exec_state.current_default_units())))
4646
}
4747

4848
/// Compute the tangent of a number (in radians).
4949
pub async fn tan(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
5050
let num: TyF64 = args.get_unlabeled_kw_arg("input", &RuntimeType::angle(), exec_state)?;
51-
let num = num.to_radians();
51+
let num = num.to_radians(exec_state, args.source_range);
5252
Ok(args.make_user_val_from_f64_with_type(TyF64::new(libm::tan(num), exec_state.current_default_units())))
5353
}
5454

@@ -190,7 +190,7 @@ pub async fn atan(exec_state: &mut ExecState, args: Args) -> Result<KclValue, Kc
190190
pub async fn atan2(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
191191
let y = args.get_kw_arg("y", &RuntimeType::length(), exec_state)?;
192192
let x = args.get_kw_arg("x", &RuntimeType::length(), exec_state)?;
193-
let (y, x, _) = NumericType::combine_eq_coerce(y, x);
193+
let (y, x, _) = NumericType::combine_eq_coerce(y, x, Some((exec_state, args.source_range)));
194194
let result = libm::atan2(y, x);
195195

196196
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<KclValue, KclE
237237
pub async fn leg_length(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
238238
let hypotenuse: TyF64 = args.get_kw_arg("hypotenuse", &RuntimeType::length(), exec_state)?;
239239
let leg: TyF64 = args.get_kw_arg("leg", &RuntimeType::length(), exec_state)?;
240-
let (hypotenuse, leg, ty) = NumericType::combine_eq_coerce(hypotenuse, leg);
240+
let (hypotenuse, leg, ty) = NumericType::combine_eq_coerce(hypotenuse, leg, Some((exec_state, args.source_range)));
241241
let result = (hypotenuse.powi(2) - f64::min(hypotenuse.abs(), leg.abs()).powi(2)).sqrt();
242242
Ok(KclValue::from_number_with_type(result, ty, vec![args.into()]))
243243
}
@@ -246,7 +246,7 @@ pub async fn leg_length(exec_state: &mut ExecState, args: Args) -> Result<KclVal
246246
pub async fn leg_angle_x(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
247247
let hypotenuse: TyF64 = args.get_kw_arg("hypotenuse", &RuntimeType::length(), exec_state)?;
248248
let leg: TyF64 = args.get_kw_arg("leg", &RuntimeType::length(), exec_state)?;
249-
let (hypotenuse, leg, _ty) = NumericType::combine_eq_coerce(hypotenuse, leg);
249+
let (hypotenuse, leg, _ty) = NumericType::combine_eq_coerce(hypotenuse, leg, Some((exec_state, args.source_range)));
250250
let result = libm::acos(leg.min(hypotenuse) / hypotenuse).to_degrees();
251251
Ok(KclValue::from_number_with_type(
252252
result,
@@ -259,7 +259,7 @@ pub async fn leg_angle_x(exec_state: &mut ExecState, args: Args) -> Result<KclVa
259259
pub async fn leg_angle_y(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
260260
let hypotenuse: TyF64 = args.get_kw_arg("hypotenuse", &RuntimeType::length(), exec_state)?;
261261
let leg: TyF64 = args.get_kw_arg("leg", &RuntimeType::length(), exec_state)?;
262-
let (hypotenuse, leg, _ty) = NumericType::combine_eq_coerce(hypotenuse, leg);
262+
let (hypotenuse, leg, _ty) = NumericType::combine_eq_coerce(hypotenuse, leg, Some((exec_state, args.source_range)));
263263
let result = libm::asin(leg.min(hypotenuse) / hypotenuse).to_degrees();
264264
Ok(KclValue::from_number_with_type(
265265
result,

0 commit comments

Comments
 (0)