From b6fcd46075c3d3c253d4c54b48330767ce1c3cda Mon Sep 17 00:00:00 2001 From: JT <547158+jntrnr@users.noreply.github.com> Date: Tue, 4 Jan 2022 10:14:33 +1100 Subject: [PATCH] Some error improvements (#659) --- crates/nu-engine/src/documentation.rs | 48 +------------------- crates/nu-parser/src/errors.rs | 26 +++++++---- crates/nu-parser/src/parse_keywords.rs | 19 +++++++- crates/nu-parser/src/parser.rs | 63 ++++++++++++++++++++++---- crates/nu-protocol/src/signature.rs | 46 +++++++++++++++++++ crates/nu-protocol/src/span.rs | 9 ++++ 6 files changed, 145 insertions(+), 66 deletions(-) diff --git a/crates/nu-engine/src/documentation.rs b/crates/nu-engine/src/documentation.rs index 5151f23299..a7ff88c7d2 100644 --- a/crates/nu-engine/src/documentation.rs +++ b/crates/nu-engine/src/documentation.rs @@ -1,7 +1,5 @@ use itertools::Itertools; -use nu_protocol::{ - engine::EngineState, Example, PositionalArg, Signature, Span, SyntaxShape, Value, -}; +use nu_protocol::{engine::EngineState, Example, Signature, Span, 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(); - 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(" "); - } - - if !sig.named.is_empty() { - one_liner.push_str("{flags} "); - } - - long_desc.push_str(&format!("Usage:\n > {}\n", one_liner)); + long_desc.push_str(&format!("Usage:\n > {}\n", sig.call_signature())); 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"); diff --git a/crates/nu-parser/src/errors.rs b/crates/nu-parser/src/errors.rs index 61b313067e..ae7432d388 100644 --- a/crates/nu-parser/src/errors.rs +++ b/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))] - ExtraPositional(#[label = "extra positional argument"] Span), + #[diagnostic(code(nu::parser::extra_positional), url(docsrs), help("Usage: {0}"))] + 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))] - MissingPositional(String, #[label("missing {0}")] Span), + #[diagnostic(code(nu::parser::missing_positional), url(docsrs), help("Usage: {2}"))] + 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))] diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index a4c170e239..c7edf474be 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/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; diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 8a2508ac8c..aa5ae7974c 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -102,15 +102,42 @@ pub fn check_call(command: Span, sig: &Signature, call: &Call) -> Option= 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); diff --git a/crates/nu-protocol/src/signature.rs b/crates/nu-protocol/src/signature.rs index 662d0983b5..a3406d472d 100644 --- a/crates/nu-protocol/src/signature.rs +++ b/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(" "); + // } + + 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 { 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, diff --git a/crates/nu-protocol/src/span.rs b/crates/nu-protocol/src/span.rs index 1a5cf900f5..350962587b 100644 --- a/crates/nu-protocol/src/span.rs +++ b/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