Improve def parse errors (#3681)

crates/nu-errors/src/lib.rs

@@ -32,7 +32,10 @@ pub enum ParseErrorReason {
     Unclosed { delimiter: String, span: Span },
 
     /// An unexpected internal error has occurred
-    InternalError { message: Spanned<String> },
+    GeneralError {
+        message: String,
+        label: Spanned<String>,
+    },
 
     /// The parser tried to parse an argument for a command, but it failed for
     /// some reason
@@ -83,11 +86,15 @@ impl ParseError {
         }
     }
 
-    /// Construct a [ParseErrorReason::InternalError](ParseErrorReason::InternalError)
+    /// Construct a [ParseErrorReason::GeneralError](ParseErrorReason::GeneralError)
-    pub fn internal_error(message: Spanned<impl Into<String>>) -> ParseError {
+    pub fn general_error(
+        message: impl Into<String>,
+        label: Spanned<impl Into<String>>,
+    ) -> ParseError {
         ParseError {
-            reason: ParseErrorReason::InternalError {
+            reason: ParseErrorReason::GeneralError {
-                message: message.item.into().spanned(message.span),
+                message: message.into(),
+                label: label.item.into().spanned(label.span),
             },
         }
     }
@@ -119,11 +126,9 @@ impl From<ParseError> for ShellError {
             ParseErrorReason::Mismatch { actual, expected } => {
                 ShellError::type_error(expected, actual)
             }
-            ParseErrorReason::InternalError { message } => ShellError::labeled_error(
+            ParseErrorReason::GeneralError { message, label } => {
-                format!("Internal error: {}", message.item),
+                ShellError::labeled_error(&message, &label.item, &label.span)
-                &message.item,
+            }
-                &message.span,
-            ),
             ParseErrorReason::ArgumentError { command, error } => {
                 ShellError::argument_error(command, error)
             }

crates/nu-parser/src/parse.rs

@@ -1853,13 +1853,8 @@ fn parse_call(
     // Check if it's an internal command
     if let Some(signature) = scope.get_signature(&lite_cmd.parts[0].item) {
         if lite_cmd.parts[0].item == "def" {
-            let error = parse_definition(&lite_cmd, scope);
+            let err = parse_definition(&lite_cmd, scope);
-            if error.is_some() {
+            error = error.or(err);
-                return (
-                    Some(ClassifiedCommand::Expr(Box::new(garbage(lite_cmd.span())))),
-                    error,
-                );
-            }
         }
         let (mut internal_command, err) = parse_internal_command(&lite_cmd, scope, &signature, 0);
 

crates/nu-parser/src/parse/def.rs

@@ -8,7 +8,7 @@ use crate::{
 use indexmap::IndexMap;
 use nu_errors::ParseError;
 use nu_protocol::hir::Block;
-use nu_source::{HasSpan, SpannedItem};
+use nu_source::{HasSpan, Span, SpannedItem};
 
 //use crate::errors::{ParseError, ParseResult};
 use crate::lex::lexer::{lex, parse_block};
@@ -28,59 +28,100 @@ pub(crate) fn parse_definition(call: &LiteCommand, scope: &dyn ParserScope) -> O
     // So our main goal here is to parse the block now that the names and
     // prototypes of adjacent commands are also available
 
-    if call.parts.len() == 4 {
+    match call.parts.len() {
-        if call.parts[0].item != "def" {
+        4 => {
-            return Some(ParseError::mismatch("definition", call.parts[0].clone()));
+            if call.parts[0].item != "def" {
-        }
+                return Some(ParseError::mismatch("definition", call.parts[0].clone()));
-
+            }
-        let name = trim_quotes(&call.parts[1].item);
+
-        let (mut signature, err) = parse_signature(&name, &call.parts[2]);
+            let name = trim_quotes(&call.parts[1].item);
-
+            let (mut signature, err) = parse_signature(&name, &call.parts[2]);
-        //Add commands comments to signature usage
+
-        signature.usage = call.comments_joined();
+            //Add commands comments to signature usage
-
+            signature.usage = call.comments_joined();
-        if err.is_some() {
+
-            return err;
+            if err.is_some() {
-        };
+                return err;
-
+            };
-        let mut chars = call.parts[3].chars();
+
-        match (chars.next(), chars.next_back()) {
+            let mut chars = call.parts[3].chars();
-            (Some('{'), Some('}')) => {
+            match (chars.next(), chars.next_back()) {
-                // We have a literal block
+                (Some('{'), Some('}')) => {
-                let string: String = chars.collect();
+                    // We have a literal block
-
+                    let string: String = chars.collect();
-                scope.enter_scope();
+
-
+                    scope.enter_scope();
-                let (tokens, err) = lex(&string, call.parts[3].span.start() + 1);
+
-                if err.is_some() {
+                    let (tokens, err) = lex(&string, call.parts[3].span.start() + 1);
-                    return err;
+                    if err.is_some() {
-                };
+                        return err;
-                let (lite_block, err) = parse_block(tokens);
+                    };
-                if err.is_some() {
+                    let (lite_block, err) = parse_block(tokens);
-                    return err;
+                    if err.is_some() {
-                };
+                        return err;
-
+                    };
-                let (mut block, err) = classify_block(&lite_block, scope);
+
-                scope.exit_scope();
+                    let (mut block, err) = classify_block(&lite_block, scope);
-
+                    scope.exit_scope();
-                if let Some(block) = std::sync::Arc::<nu_protocol::hir::Block>::get_mut(&mut block)
+
-                {
+                    if let Some(block) =
-                    block.params = signature;
+                        std::sync::Arc::<nu_protocol::hir::Block>::get_mut(&mut block)
-                    block.params.name = name;
+                    {
-                }
+                        block.params = signature;
-
+                        block.params.name = name;
-                scope.add_definition(block);
+                    }
-
+
-                err
+                    scope.add_definition(block);
+
+                    err
+                }
+                _ => Some(ParseError::mismatch("body", call.parts[3].clone())),
             }
-            _ => Some(ParseError::mismatch("body", call.parts[3].clone())),
         }
-    } else {
+
-        Some(ParseError::internal_error(
+        3 => Some(ParseError::general_error(
-            "Wrong shape. Expected def name [signature] {body}."
+            "wrong shape. Expected: def name [signature] {body}",
+            "expected definition body".to_string().spanned(Span::new(
+                call.parts[2].span.end(),
+                call.parts[2].span.end(),
+            )),
+        )),
+        2 => Some(ParseError::general_error(
+            "wrong shape. Expected: def name [signature] {body}",
+            "expected definition parameters"
                 .to_string()
-                .spanned(call.span()),
+                .spanned(Span::new(
-        ))
+                    call.parts[1].span.end(),
+                    call.parts[1].span.end(),
+                )),
+        )),
+        1 => Some(ParseError::general_error(
+            "wrong shape. Expected: def name [signature] {body}",
+            "expected definition name".to_string().spanned(Span::new(
+                call.parts[0].span.end(),
+                call.parts[0].span.end(),
+            )),
+        )),
+        0 => Some(ParseError::general_error(
+            "wrong shape. Expected: def name [signature] {body}",
+            "expected 'def' keyword'".to_string().spanned(call.span()),
+        )),
+
+        x if x < 4 => Some(ParseError::general_error(
+            "wrong shape. Expected: def name [signature] {body}",
+            "expected: def name [signature] {body}"
+                .to_string()
+                .spanned(Span::new(
+                    call.parts[x - 1].span.end(),
+                    call.parts[x - 1].span.end(),
+                )),
+        )),
+        _ => Some(ParseError::general_error(
+            "extra arguments given. Expected: def name [signature] {body}.",
+            "extra argument given"
+                .to_string()
+                .spanned(call.parts[4].span()),
+        )),
     }
 }