Some error improvements (#659)

crates/nu-engine/src/documentation.rs

@@ -1,7 +1,5 @@
 use itertools::Itertools;
-use nu_protocol::{
+use nu_protocol::{engine::EngineState, Example, Signature, Span, Value};
-    engine::EngineState, Example, PositionalArg, Signature, Span, SyntaxShape, Value,
-};
 use std::collections::HashMap;
 
 const COMMANDS_DOCS_DIR: &str = "docs/commands";
@@ -168,30 +166,7 @@ pub fn get_documentation(
         }
     }
 
-    let mut one_liner = String::new();
+    long_desc.push_str(&format!("Usage:\n  > {}\n", sig.call_signature()));
-    one_liner.push_str(&sig.name);
-    one_liner.push(' ');
-
-    for positional in &sig.required_positional {
-        one_liner.push_str(&get_positional_short_name(positional, true));
-    }
-    for positional in &sig.optional_positional {
-        one_liner.push_str(&get_positional_short_name(positional, false));
-    }
-
-    if sig.rest_positional.is_some() {
-        one_liner.push_str("...args ");
-    }
-
-    if !subcommands.is_empty() {
-        one_liner.push_str("<subcommand> ");
-    }
-
-    if !sig.named.is_empty() {
-        one_liner.push_str("{flags} ");
-    }
-
-    long_desc.push_str(&format!("Usage:\n  > {}\n", one_liner));
 
     if !subcommands.is_empty() {
         long_desc.push_str("\nSubcommands:\n");
@@ -246,25 +221,6 @@ pub fn get_documentation(
     long_desc
 }
 
-fn get_positional_short_name(arg: &PositionalArg, is_required: bool) -> String {
-    match &arg.shape {
-        SyntaxShape::Keyword(name, ..) => {
-            if is_required {
-                format!("{} <{}> ", String::from_utf8_lossy(name), arg.name)
-            } else {
-                format!("({} <{}>) ", String::from_utf8_lossy(name), arg.name)
-            }
-        }
-        _ => {
-            if is_required {
-                format!("<{}> ", arg.name)
-            } else {
-                format!("({}) ", arg.name)
-            }
-        }
-    }
-}
-
 fn get_flags_section(signature: &Signature) -> String {
     let mut long_desc = String::new();
     long_desc.push_str("\nFlags:\n");

crates/nu-parser/src/errors.rs

@@ -16,8 +16,8 @@ pub enum ParseError {
     ExtraTokens(#[label = "extra tokens"] Span),
 
     #[error("Extra positional argument.")]
-    #[diagnostic(code(nu::parser::extra_positional), url(docsrs))]
+    #[diagnostic(code(nu::parser::extra_positional), url(docsrs), help("Usage: {0}"))]
-    ExtraPositional(#[label = "extra positional argument"] Span),
+    ExtraPositional(String, #[label = "extra positional argument"] Span),
 
     #[error("Unexpected end of code.")]
     #[diagnostic(code(nu::parser::unexpected_eof), url(docsrs))]
@@ -106,28 +106,36 @@ pub enum ParseError {
     NonUtf8(#[label = "non-UTF8 string"] Span),
 
     #[error("The `{0}` command doesn't have flag `{1}`.")]
-    #[diagnostic(code(nu::parser::unknown_flag), url(docsrs))]
+    #[diagnostic(
+        code(nu::parser::unknown_flag),
+        url(docsrs),
+        help("use {0} --help for a list of flags")
+    )]
     UnknownFlag(String, String, #[label = "unknown flag"] Span),
 
     #[error("Unknown type.")]
     #[diagnostic(code(nu::parser::unknown_type), url(docsrs))]
     UnknownType(#[label = "unknown type"] Span),
 
-    #[error("Missing flag param.")]
+    #[error("Missing flag argument.")]
     #[diagnostic(code(nu::parser::missing_flag_param), url(docsrs))]
-    MissingFlagParam(#[label = "flag missing param"] Span),
+    MissingFlagParam(String, #[label = "flag missing {0} argument"] Span),
 
     #[error("Batches of short flags can't take arguments.")]
     #[diagnostic(code(nu::parser::short_flag_arg_cant_take_arg), url(docsrs))]
     ShortFlagBatchCantTakeArg(#[label = "short flag batches can't take args"] Span),
 
     #[error("Missing required positional argument.")]
-    #[diagnostic(code(nu::parser::missing_positional), url(docsrs))]
+    #[diagnostic(code(nu::parser::missing_positional), url(docsrs), help("Usage: {2}"))]
-    MissingPositional(String, #[label("missing {0}")] Span),
+    MissingPositional(String, #[label("missing {0}")] Span, String),
 
-    #[error("Missing argument to `{0}`.")]
+    #[error("Missing argument to `{1}`.")]
     #[diagnostic(code(nu::parser::keyword_missing_arg), url(docsrs))]
-    KeywordMissingArgument(String, #[label("missing value that follows {0}")] Span),
+    KeywordMissingArgument(
+        String,
+        String,
+        #[label("missing {0} value that follows {1}")] Span,
+    ),
 
     #[error("Missing type.")]
     #[diagnostic(code(nu::parser::missing_type), url(docsrs))]

crates/nu-parser/src/parse_keywords.rs

@@ -344,6 +344,9 @@ pub fn parse_export(
                     );
                 }
 
+                let sig = working_set.get_decl(call.decl_id);
+                let call_signature = sig.signature().call_signature();
+
                 call.head = span(&spans[0..=1]);
 
                 if let Some(name_span) = spans.get(2) {
@@ -385,7 +388,11 @@ pub fn parse_export(
                         };
 
                         error = error.or_else(|| {
-                            Some(ParseError::MissingPositional("block".into(), err_span))
+                            Some(ParseError::MissingPositional(
+                                "block".into(),
+                                err_span,
+                                call_signature,
+                            ))
                         });
 
                         None
@@ -400,6 +407,7 @@ pub fn parse_export(
                         Some(ParseError::MissingPositional(
                             "environment variable name".into(),
                             err_span,
+                            call_signature,
                         ))
                     });
 
@@ -426,6 +434,7 @@ pub fn parse_export(
                     start: export_span.end,
                     end: export_span.end,
                 },
+                "'def' or 'env' keyword.".to_string(),
             ))
         });
 
@@ -925,6 +934,9 @@ pub fn parse_let(
         }
 
         if let Some(decl_id) = working_set.find_decl(b"let") {
+            let cmd = working_set.get_decl(decl_id);
+            let call_signature = cmd.signature().call_signature();
+
             if spans.len() >= 4 {
                 // This is a bit of by-hand parsing to get around the issue where we want to parse in the reverse order
                 // so that the var-id created by the variable isn't visible in the expression that init it
@@ -943,7 +955,10 @@ pub fn parse_let(
                         error = error.or(err);
 
                         if idx < (spans.len() - 1) {
-                            error = error.or(Some(ParseError::ExtraPositional(spans[idx + 1])));
+                            error = error.or(Some(ParseError::ExtraPositional(
+                                call_signature,
+                                spans[idx + 1],
+                            )));
                         }
 
                         let mut idx = 0;

crates/nu-parser/src/parser.rs

@@ -102,15 +102,42 @@ pub fn check_call(command: Span, sig: &Signature, call: &Call) -> Option<ParseEr
                 }
             });
             if !found {
-                return Some(ParseError::MissingPositional(
+                if let Some(last) = call.positional.last() {
-                    argument.name.clone(),
+                    return Some(ParseError::MissingPositional(
-                    command,
+                        argument.name.clone(),
-                ));
+                        Span {
+                            start: last.span.end,
+                            end: last.span.end,
+                        },
+                        sig.call_signature(),
+                    ));
+                } else {
+                    return Some(ParseError::MissingPositional(
+                        argument.name.clone(),
+                        command,
+                        sig.call_signature(),
+                    ));
+                }
             }
         }
 
         let missing = &sig.required_positional[call.positional.len()];
-        Some(ParseError::MissingPositional(missing.name.clone(), command))
+        if let Some(last) = call.positional.last() {
+            Some(ParseError::MissingPositional(
+                missing.name.clone(),
+                Span {
+                    start: last.span.end,
+                    end: last.span.end,
+                },
+                sig.call_signature(),
+            ))
+        } else {
+            Some(ParseError::MissingPositional(
+                missing.name.clone(),
+                command,
+                sig.call_signature(),
+            ))
+        }
     } else {
         for req_flag in sig.named.iter().filter(|x| x.required) {
             if call.named.iter().all(|(n, _)| n.item != req_flag.long) {
@@ -228,7 +255,10 @@ fn parse_long_flag(
                         (
                             Some(long_name),
                             None,
-                            Some(ParseError::MissingFlagParam(arg_span)),
+                            Some(ParseError::MissingFlagParam(
+                                arg_shape.to_string(),
+                                arg_span,
+                            )),
                         )
                     }
                 } else {
@@ -467,8 +497,12 @@ pub fn parse_multispan_value(
             if *spans_idx >= spans.len() {
                 error = error.or_else(|| {
                     Some(ParseError::KeywordMissingArgument(
+                        arg.to_string(),
                         String::from_utf8_lossy(keyword).into(),
-                        spans[*spans_idx - 1],
+                        Span {
+                            start: spans[*spans_idx - 1].end,
+                            end: spans[*spans_idx - 1].end,
+                        },
                     ))
                 });
                 return (
@@ -584,7 +618,12 @@ pub fn parse_internal_call(
                         ));
                         spans_idx += 1;
                     } else {
-                        error = error.or(Some(ParseError::MissingFlagParam(arg_span)))
+                        error = error.or_else(|| {
+                            Some(ParseError::MissingFlagParam(
+                                arg_shape.to_string(),
+                                arg_span,
+                            ))
+                        })
                     }
                 } else {
                     call.named.push((
@@ -614,6 +653,7 @@ pub fn parse_internal_call(
                     Some(ParseError::MissingPositional(
                         positional.name.clone(),
                         spans[spans_idx],
+                        signature.call_signature(),
                     ))
                 });
                 positional_idx += 1;
@@ -646,7 +686,12 @@ pub fn parse_internal_call(
             positional_idx += 1;
         } else {
             call.positional.push(Expression::garbage(arg_span));
-            error = error.or(Some(ParseError::ExtraPositional(arg_span)))
+            error = error.or_else(|| {
+                Some(ParseError::ExtraPositional(
+                    signature.call_signature(),
+                    arg_span,
+                ))
+            })
         }
 
         error = error.or(err);

crates/nu-protocol/src/signature.rs

@@ -274,6 +274,33 @@ impl Signature {
         self
     }
 
+    pub fn call_signature(&self) -> String {
+        let mut one_liner = String::new();
+        one_liner.push_str(&self.name);
+        one_liner.push(' ');
+
+        for positional in &self.required_positional {
+            one_liner.push_str(&get_positional_short_name(positional, true));
+        }
+        for positional in &self.optional_positional {
+            one_liner.push_str(&get_positional_short_name(positional, false));
+        }
+
+        if self.rest_positional.is_some() {
+            one_liner.push_str("...args ");
+        }
+
+        // if !self.subcommands.is_empty() {
+        //     one_liner.push_str("<subcommand> ");
+        // }
+
+        if !self.named.is_empty() {
+            one_liner.push_str("{flags} ");
+        }
+
+        one_liner
+    }
+
     /// Get list of the short-hand flags
     pub fn get_shorts(&self) -> Vec<char> {
         self.named.iter().filter_map(|f| f.short).collect()
@@ -431,6 +458,25 @@ impl Command for Predeclaration {
     }
 }
 
+fn get_positional_short_name(arg: &PositionalArg, is_required: bool) -> String {
+    match &arg.shape {
+        SyntaxShape::Keyword(name, ..) => {
+            if is_required {
+                format!("{} <{}> ", String::from_utf8_lossy(name), arg.name)
+            } else {
+                format!("({} <{}>) ", String::from_utf8_lossy(name), arg.name)
+            }
+        }
+        _ => {
+            if is_required {
+                format!("<{}> ", arg.name)
+            } else {
+                format!("({}) ", arg.name)
+            }
+        }
+    }
+}
+
 #[derive(Clone)]
 struct BlockCommand {
     signature: Signature,

crates/nu-protocol/src/span.rs

@@ -46,6 +46,15 @@ impl Span {
     pub fn contains(&self, pos: usize) -> bool {
         pos >= self.start && pos < self.end
     }
+
+    /// Point to the space just past this span, useful for missing
+    /// values
+    pub fn past(&self) -> Span {
+        Span {
+            start: self.end,
+            end: self.end,
+        }
+    }
 }
 
 /// Used when you have a slice of spans of at least size 1