Repository: sharkdp/numbat (rust)

Issue:
Incompatible dimension errors: better suggestions

- [x] Only show suggestions if the X factor in "multiply the rhs/lhs by X" is not too complicated. For example: only if X=D^alpha with a single dimension `D` (base or derived). Something like *"multiply left hand side by Current × Temperature × Time / Length"* is not very helpful.
- [ ] In the above spirit, something like *"multiply left hand side by Length² × Mass / Time²"* would be okay if we abbreviated it properly: *"multiply left hand side by Energy"*.
- [x] For some types of errors, only one thing can be changed by the user. For example: if a function has been defined already, you can only modify the argument type, not the parameter type. In those cases, never show suggestions to change the parameter type. And similar for other types of `IncompatibleDimensionsError`s.
- [x] Think of more sophisticated heuristics for providing even better suggestions. For example: in the `X` vs `1/X` case, do not show a suggestion to multiply the rhs by `X²`, but rather suggest that one side should be inverted?

Pull request:
Better "suggested fix" messages
- [x] Only show suggestions if the X factor in "multiply the rhs/lhs by X" is not too complicated. For example: only if X=D^alpha with a single dimension `D` (base or derived). Something like *"multiply left hand side by Current × Temperature × Time / Length"* is not very helpful.
- [x] For some types of errors, only one thing can be changed by the user. For example: if a function has been defined already, you can only modify the argument type, not the parameter type. In those cases, never show suggestions to change the parameter type. And similar for other types of `IncompatibleDimensionsError`s.
- [x] Think of more sophisticated heuristics for providing even better suggestions. For example: in the `X` vs `1/X` case, do not show a suggestion to multiply the rhs by `X²`, but rather suggest that one side should be inverted?

closes #138

Patch:
diff --git a/numbat/src/typechecker.rs b/numbat/src/typechecker.rs
index 16bfa90..e000976 100644
--- a/numbat/src/typechecker.rs
+++ b/numbat/src/typechecker.rs
@@ -35,6 +35,7 @@ pub struct IncompatibleDimensionsError {
     pub expected_dimensions: Vec<String>,
     pub span_actual: Span,
     pub actual_name: &'static str,
+    pub actual_name_for_fix: &'static str,
     pub actual_type: BaseRepresentation,
     pub actual_dimensions: Vec<String>,
 }
@@ -48,6 +49,40 @@ fn pad(a: &str, b: &str) -> (String, String) {
     )
 }
 
+fn suggested_fix(
+    expected_type: &BaseRepresentation,
+    actual_type: &BaseRepresentation,
+    expression_to_change: &str,
+) -> Option<String> {
+    // Heuristic 1: if actual_type == 1 / expected_type, suggest
+    // to invert the 'actual' expression:
+    if actual_type == &expected_type.clone().invert() {
+        return Some(format!("invert the {expression_to_change}"));
+    }
+
+    // Heuristic 2: compute the "missing" factor between the expected
+    // and the actual type. Suggest to multiply / divide with the
+    // appropriate delta.
+    let delta_type = expected_type.clone() / actual_type.clone();
+
+    let num_factors = delta_type.iter().count();
+    if num_factors > 2 {
+        return None; // Do not suggest fixes with complicated dimensions
+    }
+
+    let exponent_sum: Rational = delta_type.iter().map(|a| a.1).sum();
+
+    let (action, delta_type) = if exponent_sum >= Rational::zero() {
+        ("multiply", delta_type)
+    } else {
+        ("divide", delta_type.invert())
+    };
+
+    Some(format!(
+        "{action} the {expression_to_change} by a `{delta_type}` factor"
+    ))
+}
+
 impl fmt::Display for IncompatibleDimensionsError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let have_common_factors = self
@@ -151,24 +186,15 @@ impl fmt::Display for IncompatibleDimensionsError {
             actual_result_string.trim_start_matches(" × ").trim_end(),
         )?;
 
-        let mut missing_type = self.actual_type.clone() / self.expected_type.clone();
-
-        let suggestion_name =
-            if missing_type.iter().fold(Exponent::zero(), |a, b| a + b.1) >= Exponent::zero() {
-                self.expected_name
-            } else {
-                missing_type = missing_type.invert();
-                self.actual_name
-            };
+        if let Some(fix) = suggested_fix(
+            &self.expected_type,
+            &self.actual_type,
+            self.actual_name_for_fix,
+        ) {
+            write!(f, "\n\nSuggested fix: {fix}")?;
+        }
 
-        write!(
-            f,
-            "\n\nSuggested fix: multiply {} by {}",
-            // Remove leading whitespace padding.
-            // TODO: don't pass in names with whitespace, pad them in programatically in this method instead.
-            suggestion_name.trim_start(),
-            missing_type,
-        )
+        Ok(())
     }
 }
 
@@ -487,6 +513,7 @@ impl TypeChecker {
                                 expected_type: lhs_type,
                                 span_actual: rhs.full_span(),
                                 actual_name: "right hand side",
+                                actual_name_for_fix: "expression on the right hand side",
                                 actual_dimensions: self
                                     .registry
                                     .get_derived_entry_names_for(&rhs_type),
@@ -703,6 +730,7 @@ impl TypeChecker {
                                         expected_type: parameter_type,
                                         span_actual: args[idx].full_span(),
                                         actual_name: " argument type",
+                                        actual_name_for_fix: "function argument",
                                         actual_dimensions: self
                                             .registry
                                             .get_derived_entry_names_for(&argument_type),
@@ -849,6 +877,7 @@ impl TypeChecker {
                                         expected_type: dexpr_specified,
                                         span_actual: expr.full_span(),
                                         actual_name: "   actual dimension",
+                                        actual_name_for_fix: "right hand side expression",
                                         actual_dimensions: self
                                             .registry
                                             .get_derived_entry_names_for(&dexpr_deduced),
@@ -948,6 +977,7 @@ impl TypeChecker {
                                 expected_type: type_specified,
                                 span_actual: expr.full_span(),
                                 actual_name: "   actual dimension",
+                                actual_name_for_fix: "right hand side expression",
                                 actual_dimensions: self
                                     .registry
                                     .get_derived_entry_names_for(&type_deduced),
@@ -1142,6 +1172,7 @@ impl TypeChecker {
                                                 .map(|b| b.full_span())
                                                 .unwrap(),
                                             actual_name: "   actual return type",
+                                            actual_name_for_fix: "expression in the function body",
                                             actual_dimensions: self
                                                 .registry
                                                 .get_derived_entry_names_for(&dtype_deduced),


Test patch:
diff --git a/numbat/tests/interpreter.rs b/numbat/tests/interpreter.rs
index 2bc1b59..0c45246 100644
--- a/numbat/tests/interpreter.rs
+++ b/numbat/tests/interpreter.rs
@@ -2,6 +2,7 @@ mod common;
 
 use common::get_test_context;
 
+use insta::assert_snapshot;
 use numbat::markup::{Formatter, PlainTextFormatter};
 use numbat::resolver::CodeSource;
 use numbat::{pretty_print::PrettyPrint, Context, InterpreterResult};
@@ -39,10 +40,10 @@ fn expect_failure(code: &str, msg_part: &str) {
 }
 
 #[track_caller]
-fn expect_exact_failure(code: &str, expected: &str) {
+fn get_error_message(code: &str) -> String {
     let mut ctx = get_test_context();
     if let Err(e) = ctx.interpret(code, CodeSource::Internal) {
-        assert_eq!(e.to_string(), expected);
+        e.to_string()
     } else {
         panic!();
     }
@@ -231,35 +232,92 @@ fn test_math() {
 
 #[test]
 fn test_incompatible_dimension_errors() {
-    expect_exact_failure(
-        "kg m / s^2 + kg m^2",
-        " left hand side: Length  × Mass × Time⁻²    [= Force]\n\
-         right hand side: Length² × Mass             [= MomentOfInertia]\n\n\
-         Suggested fix: multiply left hand side by Length × Time²",
+    assert_snapshot!(
+        get_error_message("kg m / s^2 + kg m^2"),
+        @r###"
+     left hand side: Length  × Mass × Time⁻²    [= Force]
+    right hand side: Length² × Mass             [= MomentOfInertia]
+
+    Suggested fix: divide the expression on the right hand side by a `Length × Time²` factor
+    "###
     );
-    expect_exact_failure(
-        "1 + m",
-        " left hand side: Scalar    [= Angle, Scalar, SolidAngle]\n\
-         right hand side: Length\n\n\
-         Suggested fix: multiply left hand side by Length",
+
+    assert_snapshot!(
+        get_error_message("1 + m"),
+        @r###"
+     left hand side: Scalar    [= Angle, Scalar, SolidAngle]
+    right hand side: Length
+
+    Suggested fix: divide the expression on the right hand side by a `Length` factor
+    "###
     );
-    expect_exact_failure(
-        "m / s + K A",
-        " left hand side: Length / Time            [= Velocity]\n\
-         right hand side: Current × Temperature\n\n\
-         Suggested fix: multiply left hand side by Current × Temperature × Time / Length",
+
+    assert_snapshot!(
+        get_error_message("m / s + K A"),
+        @r###"
+     left hand side: Length / Time            [= Velocity]
+    right hand side: Current × Temperature
+    "###
+    );
+
+    assert_snapshot!(
+        get_error_message("m + 1 / m"),
+        @r###"
+     left hand side: Length
+    right hand side: Length⁻¹    [= Wavenumber]
+
+    Suggested fix: invert the expression on the right hand side
+    "###
     );
-    expect_exact_failure(
-        "m + 1 / m",
-        " left hand side: Length\n\
-         right hand side: Length⁻¹    [= Wavenumber]\n\n\
-         Suggested fix: multiply right hand side by Length²",
+
+    assert_snapshot!(
+        get_error_message("kW -> J"),
+        @r###"
+     left hand side: Length² × Mass × Time⁻³    [= Power]
+    right hand side: Length² × Mass × Time⁻²    [= Energy, Torque]
+
+    Suggested fix: divide the expression on the right hand side by a `Time` factor
+    "###
     );
-    expect_exact_failure(
-        "kW -> J",
-        " left hand side: Length² × Mass × Time⁻³    [= Power]\n\
-         right hand side: Length² × Mass × Time⁻²    [= Energy, Torque]\n\n\
-         Suggested fix: multiply left hand side by Time",
+
+    assert_snapshot!(
+        get_error_message("sin(1 meter)"),
+        @r###"
+    parameter type: Scalar    [= Angle, Scalar, SolidAngle]
+     argument type: Length
+
+    Suggested fix: divide the function argument by a `Length` factor
+    "###
+    );
+
+    assert_snapshot!(
+        get_error_message("let x: Acceleration = 4 m / s"),
+        @r###"
+    specified dimension: Length × Time⁻²    [= Acceleration]
+       actual dimension: Length × Time⁻¹    [= Velocity]
+
+    Suggested fix: divide the right hand side expression by a `Time` factor
+    "###
+    );
+
+    assert_snapshot!(
+        get_error_message("unit x: Acceleration = 4 m / s"),
+        @r###"
+    specified dimension: Length × Time⁻²    [= Acceleration]
+       actual dimension: Length × Time⁻¹    [= Velocity]
+
+    Suggested fix: divide the right hand side expression by a `Time` factor
+    "###
+    );
+
+    assert_snapshot!(
+        get_error_message("fn acceleration(length: Length, time: Time) -> Acceleration = length / time"),
+        @r###"
+    specified return type: Length × Time⁻²    [= Acceleration]
+       actual return type: Length × Time⁻¹    [= Velocity]
+
+    Suggested fix: divide the expression in the function body by a `Time` factor
+    "###
     );
 }