Refactor error make (#10923)

- Replaced `start`/`end` with span.
- Fixed standard library.
- Add `help` option.
- Add a couple more errors for invalid record types.

Resolve #10914


# Description



# User-Facing Changes

- **BREAKING CHANGE:** `error make` now takes in `span` instead of
`start`/`end`:

  ```Nushell
  error make {
      msg: "Message"
      label: {
          text: "Label text"
          span: (metadata $var).span
      }
  }
  ```
- `error make` now has a `help` argument for custom error help.

crates/nu-cmd-lang/src/core_commands/error_make.rs

@@ -44,20 +44,15 @@ impl Command for ErrorMake {
         call: &Call,
         _input: PipelineData,
     ) -> Result<PipelineData, ShellError> {
-        let span = call.head;
         let arg: Value = call.req(engine_state, stack, 0)?;
-        let unspanned = call.has_flag("unspanned");
 
-        let throw_error = if unspanned { None } else { Some(span) };
-        Err(make_error(&arg, throw_error).unwrap_or_else(|| {
-            ShellError::GenericError(
-                "Creating error value not supported.".into(),
-                "unsupported error format".into(),
-                Some(span),
-                None,
-                Vec::new(),
-            )
-        }))
+        let throw_span = if call.has_flag("unspanned") {
+            None
+        } else {
+            Some(call.head)
+        };
+
+        Err(make_other_error(&arg, throw_span))
     }
 
     fn examples(&self) -> Vec<Example> {
@@ -82,8 +77,13 @@ impl Command for ErrorMake {
         msg: "my custom error message"
         label: {
             text: "my custom label text"  # not mandatory unless $.label exists
-            start: 123  # not mandatory unless $.label.end is set
-            end: 456  # not mandatory unless $.label.start is set
+            # optional
+            span: {
+                # if $.label.span exists, both start and end must be present
+                start: 123
+                end: 456
+            }
+            help: "A help string, suggesting a fix to the user"  # optional
         }
     }"#,
                 result: Some(Value::error(
@@ -91,7 +91,7 @@ impl Command for ErrorMake {
                         "my custom error message".to_string(),
                         "my custom label text".to_string(),
                         Some(Span::new(123, 456)),
-                        None,
+                        Some("A help string, suggesting a fix to the user".to_string()),
                         Vec::new(),
                     ),
                     Span::unknown(),
@@ -101,13 +101,11 @@ impl Command for ErrorMake {
                 description:
                     "Create a custom error for a custom command that shows the span of the argument",
                 example: r#"def foo [x] {
-        let span = (metadata $x).span;
         error make {
             msg: "this is fishy"
             label: {
                 text: "fish right here"
-                start: $span.start
-                end: $span.end
+                span: (metadata $x).span
             }
         }
     }"#,
@@ -117,100 +115,153 @@ impl Command for ErrorMake {
     }
 }
 
-fn make_error(value: &Value, throw_span: Option<Span>) -> Option<ShellError> {
-    let span = value.span();
-    if let Value::Record { .. } = &value {
-        let msg = value.get_data_by_key("msg");
-        let label = value.get_data_by_key("label");
+const UNABLE_TO_PARSE: &str = "Unable to parse error format.";
 
-        match (msg, &label) {
-            (Some(Value::String { val: message, .. }), Some(label)) => {
-                let label_start = label.get_data_by_key("start");
-                let label_end = label.get_data_by_key("end");
-                let label_text = label.get_data_by_key("text");
-
-                let label_span = Some(label.span());
-
-                match (label_start, label_end, label_text) {
-                    (
-                        Some(Value::Int { val: start, .. }),
-                        Some(Value::Int { val: end, .. }),
-                        Some(Value::String {
-                            val: label_text, ..
-                        }),
-                    ) => {
-                        if start > end {
-                            Some(ShellError::GenericError(
-                                "invalid error format.".into(),
-                                "`$.label.start` should be smaller than `$.label.end`".into(),
-                                label_span,
-                                Some(format!("{} > {}", start, end)),
-                                Vec::new(),
-                            ))
-                        } else {
-                            Some(ShellError::GenericError(
-                                message,
-                                label_text,
-                                Some(Span::new(start as usize, end as usize)),
-                                None,
-                                Vec::new(),
-                            ))
-                        }
-                    }
-                    (
-                        None,
-                        None,
-                        Some(Value::String {
-                            val: label_text, ..
-                        }),
-                    ) => Some(ShellError::GenericError(
-                        message,
-                        label_text,
-                        throw_span,
-                        None,
-                        Vec::new(),
-                    )),
-                    (_, _, None) => Some(ShellError::GenericError(
-                        "Unable to parse error format.".into(),
-                        "missing required member `$.label.text`".into(),
-                        label_span,
-                        None,
-                        Vec::new(),
-                    )),
-                    (Some(Value::Int { .. }), None, _) => Some(ShellError::GenericError(
-                        "Unable to parse error format.".into(),
-                        "missing required member `$.label.end`".into(),
-                        label_span,
-                        Some("required because `$.label.start` is set".to_string()),
-                        Vec::new(),
-                    )),
-                    (None, Some(Value::Int { .. }), _) => Some(ShellError::GenericError(
-                        "Unable to parse error format.".into(),
-                        "missing required member `$.label.start`".into(),
-                        label_span,
-                        Some("required because `$.label.end` is set".to_string()),
-                        Vec::new(),
-                    )),
-                    _ => None,
-                }
-            }
-            (Some(Value::String { val: message, .. }), None) => Some(ShellError::GenericError(
-                message,
-                "originates from here".to_string(),
+fn make_other_error(value: &Value, throw_span: Option<Span>) -> ShellError {
+    let value = match value {
+        Value::Record { .. } => value,
+        _ => {
+            return ShellError::GenericError(
+                "Creating error value not supported.".into(),
+                "unsupported error format, must be a record".into(),
                 throw_span,
                 None,
                 Vec::new(),
-            )),
-            (None, _) => Some(ShellError::GenericError(
-                "Unable to parse error format.".into(),
-                "missing required member `$.msg`".into(),
-                Some(span),
+            )
+        }
+    };
+
+    let msg = match value.get_data_by_key("msg") {
+        Some(Value::String { val, .. }) => val,
+        Some(_) => {
+            return ShellError::GenericError(
+                UNABLE_TO_PARSE.into(),
+                "`$.msg` has wrong type, must be string".into(),
+                Some(value.span()),
                 None,
                 Vec::new(),
-            )),
-            _ => None,
+            )
         }
-    } else {
-        None
+        None => {
+            return ShellError::GenericError(
+                UNABLE_TO_PARSE.into(),
+                "missing required member `$.msg`".into(),
+                Some(value.span()),
+                None,
+                Vec::new(),
+            )
+        }
+    };
+
+    let help = match value.get_data_by_key("help") {
+        Some(Value::String { val, .. }) => Some(val),
+        _ => None,
+    };
+
+    let label = match value.get_data_by_key("label") {
+        Some(value) => value,
+        None => {
+            return ShellError::GenericError(
+                msg,
+                "originates from here".to_string(),
+                throw_span,
+                help,
+                Vec::new(),
+            )
+        }
+    };
+
+    // remove after a few versions
+    if label.get_data_by_key("start").is_some() || label.get_data_by_key("end").is_some() {
+        return ShellError::GenericError(
+            UNABLE_TO_PARSE.into(),
+            "`start` and `end` are deprecated".into(),
+            Some(value.span()),
+            Some("Use `$.label.span` instead".into()),
+            Vec::new(),
+        );
+    }
+
+    let text = match label.get_data_by_key("text") {
+        Some(Value::String { val, .. }) => val,
+        Some(_) => {
+            return ShellError::GenericError(
+                UNABLE_TO_PARSE.into(),
+                "`$.label.text` has wrong type, must be string".into(),
+                Some(label.span()),
+                None,
+                Vec::new(),
+            )
+        }
+        None => {
+            return ShellError::GenericError(
+                UNABLE_TO_PARSE.into(),
+                "missing required member `$.label.text`".into(),
+                Some(label.span()),
+                None,
+                Vec::new(),
+            )
+        }
+    };
+
+    let span = match label.get_data_by_key("span") {
+        Some(val @ Value::Record { .. }) => val,
+        Some(value) => {
+            return ShellError::GenericError(
+                UNABLE_TO_PARSE.into(),
+                "`$.label.span` has wrong type, must be record".into(),
+                Some(value.span()),
+                None,
+                Vec::new(),
+            )
+        }
+        None => return ShellError::GenericError(msg, text, throw_span, None, Vec::new()),
+    };
+
+    let span_start = match get_span_sides(&span, "start") {
+        Ok(val) => val,
+        Err(err) => return err,
+    };
+    let span_end = match get_span_sides(&span, "end") {
+        Ok(val) => val,
+        Err(err) => return err,
+    };
+
+    if span_start > span_end {
+        return ShellError::GenericError(
+            "invalid error format.".into(),
+            "`$.label.start` should be smaller than `$.label.end`".into(),
+            Some(label.span()),
+            Some(format!("{} > {}", span_start, span_end)),
+            Vec::new(),
+        );
+    }
+
+    ShellError::GenericError(
+        msg,
+        text,
+        Some(Span::new(span_start as usize, span_end as usize)),
+        help,
+        Vec::new(),
+    )
+}
+
+fn get_span_sides(span: &Value, side: &str) -> Result<i64, ShellError> {
+    match span.get_data_by_key(side) {
+        Some(Value::Int { val, .. }) => Ok(val),
+        Some(_) => Err(ShellError::GenericError(
+            UNABLE_TO_PARSE.into(),
+            format!("`$.span.{side}` must be int"),
+            Some(span.span()),
+            None,
+            Vec::new(),
+        )),
+        None => Err(ShellError::GenericError(
+            UNABLE_TO_PARSE.into(),
+            format!("`$.span.{side}` must be present, if span is specified."),
+            Some(span.span()),
+            None,
+            Vec::new(),
+        )),
     }
 }

crates/nu-command/tests/commands/error_make.rs

@@ -17,10 +17,25 @@ fn no_span_if_unspanned() {
 
 #[test]
 fn error_start_bigger_than_end_should_fail() {
-    let actual = nu!("error make {msg: foo label: {text: bar start 456 end 123}}");
+    let actual = nu!("
+        error make {
+            msg: foo
+            label: {
+                text: bar
+                span: {start: 456 end: 123}
+            }
+        }
+    ");
 
     assert!(!actual.err.contains("invalid error format"));
     assert!(!actual
         .err
         .contains("`$.label.start` should be smaller than `$.label.end`"));
 }
+
+#[test]
+fn check_help_line() {
+    let actual = nu!("error make {msg:foo help: `Custom help line`}");
+
+    assert!(actual.err.contains("Custom help line"));
+}

crates/nu-protocol/tests/test_config.rs

@@ -54,13 +54,11 @@ fn filesize_format_auto_metric_false() {
 fn fancy_default_errors() {
     let actual = nu!(nu_repl_code(&[
         r#"def force_error [x] {
-        let span = (metadata $x).span;
         error make {
             msg: "oh no!"
             label: {
                 text: "here's the error"
-                start: $span.start
-                end: $span.end
+                span: (metadata $x).span
             }
         }
     }"#,
@@ -78,13 +76,11 @@ fn narratable_errors() {
     let actual = nu!(nu_repl_code(&[
         r#"$env.config = { error_style: "plain" }"#,
         r#"def force_error [x] {
-        let span = (metadata $x).span;
         error make {
             msg: "oh no!"
             label: {
                 text: "here's the error"
-                start: $span.start
-                end: $span.end
+                span: (metadata $x).span
             }
         }
     }"#,

crates/nu-std/std/assert.nu

@@ -28,9 +28,8 @@
 # ```
 # def "assert even" [number: int] {
 #     assert ($number mod 2 == 0) --error-label {
-#         start: (metadata $number).span.start,
-#         end: (metadata $number).span.end,
 #         text: $"($number) is not an even number",
+#         span: (metadata $number).span,
 #     }
 # }
 # ```
@@ -40,13 +39,11 @@ export def main [
     --error-label: record # Label for `error make` if you want to create a custom assert
 ] {
     if $condition { return }
-    let span = (metadata $condition).span
     error make {
         msg: ($message | default "Assertion failed."),
         label: ($error_label | default {
             text: "It is not true.",
-            start: $span.start,
-            end: $span.end
+            span: (metadata $condition).span,
         })
     }
 }
@@ -75,8 +72,7 @@ export def main [
 # ```
 # def "assert not even" [number: int] {
 #     assert not ($number mod 2 == 0) --error-label {
-#         start: (metadata $number).span.start,
-#         end: (metadata $number).span.end,
+#         span: (metadata $number).span,
 #         text: $"($number) is an even number",
 #     }
 # }
@@ -93,8 +89,7 @@ export def not [
             msg: ($message | default "Assertion failed."),
             label: ($error_label | default {
                 text: "It is not false.",
-                start: $span.start,
-                end: $span.end
+                span: $span,
             })
         }
     }
@@ -114,8 +109,7 @@ export def error [
 ] {
     let error_raised = (try { do $code; false } catch { true })
     main ($error_raised) $message --error-label {
-        start: (metadata $code).span.start
-        end: (metadata $code).span.end
+        span: (metadata $code).span
         text: (
             "There were no error during code execution:\n"
          + $"        (view source $code)"
@@ -134,8 +128,10 @@ export def error [
 # > assert equal 1 2 # fails
 export def equal [left: any, right: any, message?: string] {
     main ($left == $right) $message --error-label {
-        start: (metadata $left).span.start
-        end: (metadata $right).span.end
+        span: {
+            start: (metadata $left).span.start
+            end: (metadata $right).span.end
+        }
         text: (
             "These are not equal.\n"
          + $"        Left  : '($left | to nuon --raw)'\n"
@@ -155,8 +151,10 @@ export def equal [left: any, right: any, message?: string] {
 # > assert not equal 7 7 # fails
 export def "not equal" [left: any, right: any, message?: string] {
     main ($left != $right) $message --error-label {
-        start: (metadata $left).span.start
-        end: (metadata $right).span.end
+        span: {
+            start: (metadata $left).span.start
+            end: (metadata $right).span.end
+        }
         text: $"These are both '($left | to nuon --raw)'."
     }
 }
@@ -172,8 +170,10 @@ export def "not equal" [left: any, right: any, message?: string] {
 # > assert less or equal 1 0 # fails
 export def "less or equal" [left: any, right: any, message?: string] {
     main ($left <= $right) $message --error-label {
-        start: (metadata $left).span.start
-        end: (metadata $right).span.end
+        span: {
+            start: (metadata $left).span.start
+            end: (metadata $right).span.end
+        }
         text: (
             "The condition *left <= right* is not satisfied.\n"
          + $"        Left  : '($left)'\n"
@@ -192,8 +192,10 @@ export def "less or equal" [left: any, right: any, message?: string] {
 # > assert less 1 1 # fails
 export def less [left: any, right: any, message?: string] {
     main ($left < $right) $message --error-label {
-        start: (metadata $left).span.start
-        end: (metadata $right).span.end
+        span: {
+            start: (metadata $left).span.start
+            end: (metadata $right).span.end
+        }
         text: (
             "The condition *left < right* is not satisfied.\n"
          + $"        Left  : '($left)'\n"
@@ -212,8 +214,10 @@ export def less [left: any, right: any, message?: string] {
 # > assert greater 2 2 # fails
 export def greater [left: any, right: any, message?: string] {
     main ($left > $right) $message --error-label {
-        start: (metadata $left).span.start
-        end: (metadata $right).span.end
+        span: {
+            start: (metadata $left).span.start
+            end: (metadata $right).span.end
+        }
         text: (
             "The condition *left > right* is not satisfied.\n"
          + $"        Left  : '($left)'\n"
@@ -233,8 +237,10 @@ export def greater [left: any, right: any, message?: string] {
 # > assert greater or equal 1 2 # fails
 export def "greater or equal" [left: any, right: any, message?: string] {
     main ($left >= $right) $message --error-label {
-        start: (metadata $left).span.start
-        end: (metadata $right).span.end
+        span: {
+            start: (metadata $left).span.start
+            end: (metadata $right).span.end
+        }
         text: (
             "The condition *left < right* is not satisfied.\n"
          + $"        Left  : '($left)'\n"
@@ -254,8 +260,10 @@ alias "core length" = length
 # > assert length [0] 3 # fails
 export def length [left: list, right: int, message?: string] {
     main (($left | core length) == $right) $message --error-label {
-        start: (metadata $left).span.start
-        end: (metadata $right).span.end
+        span: {
+            start: (metadata $left).span.start
+            end: (metadata $right).span.end
+        }
         text: (
             "This does not have the correct length:\n"
          + $"        value    : ($left | to nuon --raw)\n"
@@ -276,8 +284,10 @@ alias "core str contains" = str contains
 # > assert str contains "arst" "k" # fails
 export def "str contains" [left: string, right: string, message?: string] {
     main ($left | core str contains $right) $message --error-label {
-        start: (metadata $left).span.start
-        end: (metadata $right).span.end
+        span: {
+            start: (metadata $left).span.start
+            end: (metadata $right).span.end
+        }
         text: (
             $"This does not contain '($right)'.\n"
           + $"        value: ($left | to nuon --raw)"

crates/nu-std/std/dirs.nu

@@ -31,7 +31,7 @@ export def --env add [
             let exp = ($p | path expand)
             if ($exp | path type) != 'dir' {
                 let span = (metadata $p).span
-                error make {msg: "not a directory", label: {text: "not a directory", start: $span.start, end: $span.end } }
+                error make {msg: "not a directory", label: {text: "not a directory", span: $span } }
             }
             $abspaths = ($abspaths | append $exp)
         }
@@ -106,8 +106,7 @@ export def --env goto [shell?: int] {
             msg: $"(ansi red_bold)invalid_shell_index(ansi reset)"
             label: {
                 text: $"`shell` should be between 0 and (($env.DIRS_LIST | length) - 1)"
-                start: $span.start
-                end: $span.end
+                span: $span
             }
         }
     }

crates/nu-std/std/dt.nu

@@ -109,7 +109,16 @@ export def datetime-diff [
     if $earlier > $later {
         let start = (metadata $later).span.start
         let end = (metadata $earlier).span.end
-        error make {msg: "Incompatible arguments", label: {start:$start, end:$end, text:$"First datetime must be >= second, but was actually ($later - $earlier) less than it."}}
+        error make {
+            msg: "Incompatible arguments",
+            label: {
+                span: {
+                    start: $start
+                    end: $end
+                }
+                text: $"First datetime must be >= second, but was actually ($later - $earlier) less than it."
+            }
+        }
     }
     let from_expanded = ($later | date to-timezone utc | date to-record)
     let to_expanded = ($earlier | date to-timezone utc | date to-record)

crates/nu-std/std/help.nu

@@ -7,8 +7,7 @@ def throw-error [error: string, msg: string, span: record] {
         msg: ($error | error-fmt)
         label: {
             text: $msg
-            start: $span.start
-            end: $span.end
+            span: $span
         }
     }
 }
@@ -762,11 +761,10 @@ You can also learn more at (ansi default_italic)(ansi light_cyan_underline)https
 
     let span = (metadata $item | get span)
     error make {
-        msg: ("std::help::item_not_found"  | error-fmt)
+        msg: ("std::help::item_not_found" | error-fmt)
         label: {
             text: "item not found"
-            start: $span.start
-            end: $span.end
+            span: $span
         }
     }
 }

crates/nu-std/std/log.nu

@@ -237,8 +237,7 @@ def log-level-deduction-error [
                 $"        Available log levels in $env.LOG_LEVEL:"
                  ($env.LOG_LEVEL | to text | lines | each {|it| $"            ($it)" } | to text)
             ] | str join "\n")
-            start: $span.start
-            end: $span.end
+            span: $span
         }
     }
 }

crates/nu-std/std/mod.nu

@@ -45,8 +45,7 @@ export def --env "path add" [
     if ($paths | is-empty) or ($paths | length) == 0 {
         error make {msg: "Empty input", label: {
             text: "Provide at least one string or a record",
-            start: $span.start,
-            end: $span.end
+            span: $span
         }}
     }
 
@@ -64,8 +63,7 @@ export def --env "path add" [
     if null in $paths or ($paths | is-empty) {
         error make {msg: "Empty input", label: {
             text: $"Received a record, that does not contain a ($nu.os-info.name) key",
-            start: $span.start,
-            end: $span.end
+            span: $span
         }}
     }
 
@@ -326,8 +324,7 @@ export def repeat [
             msg: $"(ansi red_bold)invalid_argument(ansi reset)"
             label: {
                 text: $"n should be a positive integer, found ($n)"
-                start: $span.start
-                end: $span.end
+            	span: $span
             }
         }
     }

crates/nu-std/std/testing.nu

@@ -94,8 +94,7 @@ def throw-error [error: record] {
         msg: $"(ansi red)($error.msg)(ansi reset)"
         label: {
             text: ($error.label)
-            start: $error.span.start
-            end: $error.span.end
+            span: $error.span
         }
     }
 }

crates/nu-std/std/xml.nu

@@ -12,9 +12,13 @@ export def xaccess [
     let input = $in
     if ($path | is-empty) {
         let path_span = (metadata $path).span
-        error make {msg: 'Empty path provided'
-                    label: {text: 'Use a non-empty  list of path steps'
-                            start: $path_span.start end: $path_span.end}}
+        error make {
+            msg: 'Empty path provided'
+            label: {
+                text: 'Use a non-empty  list of path steps'
+                span: $path_span
+            }
+        }
     }
     # In xpath first element in path is applied to root element
     # this way it is possible to apply first step to root element
@@ -38,9 +42,13 @@ export def xaccess [
             },
             $type => {
                 let step_span = (metadata $step).span
-                error make {msg: $'Incorrect path step type ($type)'
-                        label: {text: 'Use a string or int as a step'
-                                start: $step_span.start end: $step_span.end}}
+                error make {
+                    msg: $'Incorrect path step type ($type)'
+                    label: {
+                        text: 'Use a string or int as a step'
+                        span: $step_span
+                    }
+                }
             }
         }
 
@@ -131,9 +139,13 @@ def xupdate-internal [ path: list updater: closure ] {
             },
             $type => {
                 let step_span = (metadata $step).span
-                error make {msg: $'Incorrect path step type ($type)'
-                        label: {text: 'Use a string or int as a step'
-                                start: $step_span.start end: $step_span.end}}
+                error make {
+                    msg: $'Incorrect path step type ($type)'
+                    label: {
+                        text: 'Use a string or int as a step'
+                        span: $step_span
+                    }
+                }
             }
         }
     }