Try to re-apply #1039

crates/nu-errors/src/lib.rs

@@ -9,64 +9,41 @@ use serde::{Deserialize, Serialize};
 use std::fmt;
 use std::ops::Range;
 
-// TODO: Spanned<T> -> HasSpanAndItem<T> ?
-
-#[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd, Hash, Serialize, Deserialize)]
-pub enum Description {
-    Source(Spanned<String>),
-    Synthetic(String),
-}
-
-impl Description {
-    fn from_spanned(item: Spanned<impl Into<String>>) -> Description {
-        Description::Source(item.map(|s| s.into()))
-    }
-
-    fn into_label(self) -> Result<Label<Span>, String> {
-        match self {
-            Description::Source(s) => Ok(Label::new_primary(s.span).with_message(s.item)),
-            Description::Synthetic(s) => Err(s),
-        }
-    }
-}
-
-impl PrettyDebug for Description {
-    fn pretty(&self) -> DebugDocBuilder {
-        match self {
-            Description::Source(s) => b::description(&s.item),
-            Description::Synthetic(s) => b::description(s),
-        }
-    }
-}
-
+/// A structured reason for a ParseError. Note that parsing in nu is more like macro expansion in
+/// other languages, so the kinds of errors that can occur during parsing are more contextual than
+/// you might expect.
 #[derive(Debug, Clone)]
 pub enum ParseErrorReason {
-    Eof {
-        expected: &'static str,
-        span: Span,
-    },
+    /// The parser encountered an EOF rather than what it was expecting
+    Eof { expected: &'static str, span: Span },
+    /// The parser encountered something other than what it was expecting
     Mismatch {
         expected: &'static str,
         actual: Spanned<String>,
     },
+    /// The parser tried to parse an argument for a command, but it failed for
+    /// some reason
     ArgumentError {
         command: Spanned<String>,
         error: ArgumentError,
     },
 }
 
+/// A newtype for `ParseErrorReason`
 #[derive(Debug, Clone)]
 pub struct ParseError {
     reason: ParseErrorReason,
 }
 
 impl ParseError {
+    /// Construct a [ParseErrorReason::Eof](ParseErrorReason::Eof)
     pub fn unexpected_eof(expected: &'static str, span: Span) -> ParseError {
         ParseError {
             reason: ParseErrorReason::Eof { expected, span },
         }
     }
 
+    /// Construct a [ParseErrorReason::Mismatch](ParseErrorReason::Mismatch)
     pub fn mismatch(expected: &'static str, actual: Spanned<impl Into<String>>) -> ParseError {
         let Spanned { span, item } = actual;
 
@@ -78,6 +55,7 @@ impl ParseError {
         }
     }
 
+    /// Construct a [ParseErrorReason::ArgumentError](ParseErrorReason::ArgumentError)
     pub fn argument_error(command: Spanned<impl Into<String>>, kind: ArgumentError) -> ParseError {
         ParseError {
             reason: ParseErrorReason::ArgumentError {
@@ -88,6 +66,7 @@ impl ParseError {
     }
 }
 
+/// Convert a [ParseError](ParseError) into a [ShellError](ShellError)
 impl From<ParseError> for ShellError {
     fn from(error: ParseError) -> ShellError {
         match error.reason {
@@ -102,11 +81,20 @@ impl From<ParseError> for ShellError {
     }
 }
 
+/// ArgumentError describes various ways that the parser could fail because of unexpected arguments.
+/// Nu commands are like a combination of functions and macros, and these errors correspond to
+/// problems that could be identified during expansion based on the syntactic signature of a
+/// command.
 #[derive(Debug, Eq, PartialEq, Clone, Ord, Hash, PartialOrd, Serialize, Deserialize)]
 pub enum ArgumentError {
+    /// The command specified a mandatory flag, but it was missing.
     MissingMandatoryFlag(String),
+    /// The command specified a mandatory positional argument, but it was missing.
     MissingMandatoryPositional(String),
+    /// A flag was found, and it should have been followed by a value, but no value was found
     MissingValueForName(String),
+    /// A sequence of characters was found that was not syntactically valid (but would have
+    /// been valid if the command was an external command)
     InvalidExternalWord,
 }
 
@@ -133,12 +121,16 @@ impl PrettyDebug for ArgumentError {
     }
 }
 
+/// A `ShellError` is a proximate error and a possible cause, which could have its own cause,
+/// creating a cause chain.
 #[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Clone, Serialize, Deserialize, Hash)]
 pub struct ShellError {
     error: ProximateShellError,
-    cause: Option<Box<ProximateShellError>>,
+    cause: Option<Box<ShellError>>,
 }
 
+/// `PrettyDebug` is for internal debugging. For user-facing debugging, [to_diagnostic](ShellError::to_diagnostic)
+/// is used, which prints an error, highlighting spans.
 impl PrettyDebug for ShellError {
     fn pretty(&self) -> DebugDocBuilder {
         match &self.error {
@@ -171,12 +163,12 @@ impl PrettyDebug for ShellError {
                         "(",
                         b::description("expr:")
                             + b::space()
-                            + expr.pretty()
+                            + b::description(&expr.item)
                             + b::description(",")
                             + b::space()
                             + b::description("subpath:")
                             + b::space()
-                            + subpath.pretty(),
+                            + b::description(&subpath.item),
                         ")",
                     )
             }
@@ -185,7 +177,7 @@ impl PrettyDebug for ShellError {
                     + b::space()
                     + b::delimit(
                         "(",
-                        b::description("subpath:") + b::space() + subpath.pretty(),
+                        b::description("subpath:") + b::space() + b::description(&subpath.item),
                         ")",
                     )
             }
@@ -295,8 +287,8 @@ impl ShellError {
         expr: Spanned<impl Into<String>>,
     ) -> ShellError {
         ProximateShellError::MissingProperty {
-            subpath: Description::from_spanned(subpath),
-            expr: Description::from_spanned(expr),
+            subpath: subpath.map(|s| s.into()),
+            expr: expr.map(|e| e.into()),
         }
         .start()
     }
@@ -306,7 +298,7 @@ impl ShellError {
         integer: impl Into<Span>,
     ) -> ShellError {
         ProximateShellError::InvalidIntegerIndex {
-            subpath: Description::from_spanned(subpath),
+            subpath: subpath.map(|s| s.into()),
             integer: integer.into(),
         }
         .start()
@@ -489,7 +481,7 @@ impl ShellError {
                 Label::new_primary(span).with_message(format!(
                     "Expected to convert {} to {} while {}, but it was out of range",
                     item,
-                    kind.desc(),
+                    kind.display(),
                     operation
                 )),
             ),
@@ -504,31 +496,33 @@ impl ShellError {
                 .with_label(Label::new_primary(span).with_message(item)),
 
             ProximateShellError::MissingProperty { subpath, expr, .. } => {
-                let subpath = subpath.into_label();
-                let expr = expr.into_label();
 
                 let mut diag = Diagnostic::new(Severity::Error, "Missing property");
 
-                match subpath {
-                    Ok(label) => diag = diag.with_label(label),
-                    Err(ty) => diag.message = format!("Missing property (for {})", ty),
-                }
+                if subpath.span == Span::unknown() {
+                    diag.message = format!("Missing property (for {})", subpath.item);
+                } else {
+                    let subpath = Label::new_primary(subpath.span).with_message(subpath.item);
+                    diag = diag.with_label(subpath);
+
+                    if expr.span != Span::unknown() {
+                        let expr = Label::new_primary(expr.span).with_message(expr.item);
+                        diag = diag.with_label(expr)
+                    }
 
-                if let Ok(label) = expr {
-                    diag = diag.with_label(label);
                 }
 
                 diag
             }
 
             ProximateShellError::InvalidIntegerIndex { subpath,integer } => {
-                let subpath = subpath.into_label();
-
                 let mut diag = Diagnostic::new(Severity::Error, "Invalid integer property");
 
-                match subpath {
-                    Ok(label) => diag = diag.with_label(label),
-                    Err(ty) => diag.message = format!("Invalid integer property (for {})", ty)
+                if subpath.span == Span::unknown() {
+                    diag.message = format!("Invalid integer property (for {})", subpath.item)
+                } else {
+                    let label = Label::new_primary(subpath.span).with_message(subpath.item);
+                    diag = diag.with_label(label)
                 }
 
                 diag = diag.with_label(Label::new_secondary(integer).with_message("integer"));
@@ -586,6 +580,10 @@ impl ShellError {
     }
 }
 
+/// `ExpectedRange` describes a range of values that was expected by a command. In addition
+/// to typical ranges, this enum allows an error to specify that the range of allowed values
+/// corresponds to a particular numeric type (which is a dominant use-case for the
+/// [RangeError](ProximateShellError::RangeError) error type).
 #[derive(Debug, Eq, PartialEq, Ord, PartialOrd, Hash, Clone, Serialize, Deserialize)]
 pub enum ExpectedRange {
     I8,
@@ -607,6 +605,7 @@ pub enum ExpectedRange {
     Range { start: usize, end: usize },
 }
 
+/// Convert a Rust range into an [ExpectedRange](ExpectedRange).
 impl From<Range<usize>> for ExpectedRange {
     fn from(range: Range<usize>) -> Self {
         ExpectedRange::Range {
@@ -618,13 +617,7 @@ impl From<Range<usize>> for ExpectedRange {
 
 impl PrettyDebug for ExpectedRange {
     fn pretty(&self) -> DebugDocBuilder {
-        b::description(self.desc())
-    }
-}
-
-impl ExpectedRange {
-    fn desc(&self) -> String {
-        match self {
+        b::description(match self {
             ExpectedRange::I8 => "an 8-bit signed integer",
             ExpectedRange::I16 => "a 16-bit signed integer",
             ExpectedRange::I32 => "a 32-bit signed integer",
@@ -641,9 +634,10 @@ impl ExpectedRange {
             ExpectedRange::Size => "a list offset",
             ExpectedRange::BigDecimal => "a decimal",
             ExpectedRange::BigInt => "an integer",
-            ExpectedRange::Range { start, end } => return format!("{} to {}", start, end),
-        }
-        .to_string()
+            ExpectedRange::Range { start, end } => {
+                return b::description(format!("{} to {}", start, end))
+            }
+        })
     }
 }
 
@@ -661,11 +655,11 @@ pub enum ProximateShellError {
         actual: Spanned<Option<String>>,
     },
     MissingProperty {
-        subpath: Description,
-        expr: Description,
+        subpath: Spanned<String>,
+        expr: Spanned<String>,
     },
     InvalidIntegerIndex {
-        subpath: Description,
+        subpath: Spanned<String>,
         integer: Span,
     },
     MissingValue {

crates/nu-parser/src/hir/tokens_iterator.rs

@@ -8,7 +8,7 @@ use crate::TokenNode;
 #[allow(unused)]
 use getset::{Getters, MutGetters};
 use nu_errors::{ParseError, ShellError};
-use nu_source::{HasFallibleSpan, Span, SpannedItem, Spanned, HasSpan, Tag, Text};
+use nu_source::{HasFallibleSpan, HasSpan, Span, Spanned, SpannedItem, Tag, Text};
 
 cfg_if::cfg_if! {
     if #[cfg(coloring_in_tokens)] {

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

@@ -1,7 +1,7 @@
 use crate::hir::syntax_shape::flat_shape::FlatShape;
 use derive_new::new;
 use getset::Getters;
-use nu_source::{Span, b, Spanned, SpannedItem, PrettyDebugWithSource, DebugDocBuilder};
+use nu_source::{b, DebugDocBuilder, PrettyDebugWithSource, Span, Spanned, SpannedItem};
 use serde::{Deserialize, Serialize};
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Deserialize, Serialize)]

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

@@ -1,5 +1,5 @@
+use nu_source::{b, DebugDocBuilder, PrettyDebug};
 use serde::{Deserialize, Serialize};
-use nu_source::{b, PrettyDebug, DebugDocBuilder};
 
 use std::str::FromStr;
 

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

@@ -1,7 +1,7 @@
 use crate::TokenNode;
 use derive_new::new;
 use getset::Getters;
-use nu_source::{b, DebugDocBuilder, PrettyDebugWithSource, Span, Spanned, HasSpan};
+use nu_source::{b, DebugDocBuilder, HasSpan, PrettyDebugWithSource, Span, Spanned};
 
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Getters, new)]
 pub struct Pipeline {

crates/nu-protocol/src/call_info.rs

@@ -1,7 +1,7 @@
-use nu_errors::ShellError;
 use crate::value::Value;
 use derive_new::new;
 use indexmap::IndexMap;
+use nu_errors::ShellError;
 use nu_source::Tag;
 use serde::{Deserialize, Serialize};
 

crates/nu-protocol/src/return_value.rs

@@ -1,5 +1,5 @@
-use nu_errors::ShellError;
 use crate::value::Value;
+use nu_errors::ShellError;
 use nu_source::{b, DebugDocBuilder, PrettyDebug};
 use serde::{Deserialize, Serialize};
 

crates/nu-protocol/src/value.rs

@@ -139,6 +139,13 @@ impl Value {
         }
     }
 
+    pub fn as_forgiving_string(&self) -> Result<&str, ShellError> {
+        match &self.value {
+            UntaggedValue::Primitive(Primitive::String(string)) => Ok(&string[..]),
+            _ => Err(ShellError::type_error("string", self.spanned_type_name())),
+        }
+    }
+
     pub fn as_path(&self) -> Result<PathBuf, ShellError> {
         match &self.value {
             UntaggedValue::Primitive(Primitive::Path(path)) => Ok(path.clone()),

crates/nu-protocol/src/value/convert.rs

@@ -1,8 +1,8 @@
-use nu_errors::{CoerceInto, ShellError};
 use crate::type_name::SpannedTypeName;
 use crate::value::dict::Dictionary;
 use crate::value::primitive::Primitive;
 use crate::value::{UntaggedValue, Value};
+use nu_errors::{CoerceInto, ShellError};
 use nu_source::TaggedItem;
 
 impl std::convert::TryFrom<&Value> for i64 {

src/cli.rs

@@ -1,5 +1,4 @@
-use crate::commands::classified::external::{run_external_command, StreamNext};
-use crate::commands::classified::internal::run_internal_command;
+use crate::commands::classified::pipeline::run_pipeline;
 use crate::commands::classified::ClassifiedInputStream;
 use crate::commands::plugin::JsonRpc;
 use crate::commands::plugin::{PluginCommand, PluginSink};
@@ -14,7 +13,7 @@ use nu_parser::{
     expand_syntax, hir, ClassifiedCommand, ClassifiedPipeline, InternalCommand, PipelineShape,
     TokenNode, TokensIterator,
 };
-use nu_protocol::{ReturnSuccess, Signature, UntaggedValue, Value};
+use nu_protocol::{Signature, UntaggedValue, Value};
 
 use log::{debug, log_enabled, trace};
 use rustyline::error::ReadlineError;
@@ -582,113 +581,16 @@ async fn process_line(readline: Result<String, ReadlineError>, ctx: &mut Context
                     })),
             }
 
-            let mut input = ClassifiedInputStream::new();
-            let mut iter = pipeline.commands.list.into_iter().peekable();
-
             // Check the config to see if we need to update the path
             // TODO: make sure config is cached so we don't path this load every call
             set_env_from_config();
 
-            loop {
-                let item: Option<ClassifiedCommand> = iter.next();
-                let next: Option<&ClassifiedCommand> = iter.peek();
+            let input = ClassifiedInputStream::new();
 
-                input = match (item, next) {
-                    (None, _) => break,
-
-                    (Some(ClassifiedCommand::Dynamic(_)), _)
-                    | (_, Some(ClassifiedCommand::Dynamic(_))) => {
-                        return LineResult::Error(
-                            line.to_string(),
-                            ShellError::unimplemented("Dynamic commands"),
-                        )
-                    }
-
-                    (Some(ClassifiedCommand::Expr(_)), _) => {
-                        return LineResult::Error(
-                            line.to_string(),
-                            ShellError::unimplemented("Expression-only commands"),
-                        )
-                    }
-
-                    (_, Some(ClassifiedCommand::Expr(_))) => {
-                        return LineResult::Error(
-                            line.to_string(),
-                            ShellError::unimplemented("Expression-only commands"),
-                        )
-                    }
-
-                    (
-                        Some(ClassifiedCommand::Internal(left)),
-                        Some(ClassifiedCommand::External(_)),
-                    ) => match run_internal_command(left, ctx, input, Text::from(line)).await {
-                        Ok(val) => ClassifiedInputStream::from_input_stream(val),
-                        Err(err) => return LineResult::Error(line.to_string(), err),
-                    },
-
-                    (Some(ClassifiedCommand::Internal(left)), Some(_)) => {
-                        match run_internal_command(left, ctx, input, Text::from(line)).await {
-                            Ok(val) => ClassifiedInputStream::from_input_stream(val),
-                            Err(err) => return LineResult::Error(line.to_string(), err),
-                        }
-                    }
-
-                    (Some(ClassifiedCommand::Internal(left)), None) => {
-                        match run_internal_command(left, ctx, input, Text::from(line)).await {
-                            Ok(val) => {
-                                use futures::stream::TryStreamExt;
-
-                                let mut output_stream: OutputStream = val.into();
-                                loop {
-                                    match output_stream.try_next().await {
-                                        Ok(Some(ReturnSuccess::Value(Value {
-                                            value: UntaggedValue::Error(e),
-                                            ..
-                                        }))) => {
-                                            return LineResult::Error(line.to_string(), e);
-                                        }
-                                        Ok(Some(_item)) => {
-                                            if ctx.ctrl_c.load(Ordering::SeqCst) {
-                                                break;
-                                            }
-                                        }
-                                        _ => {
-                                            break;
-                                        }
-                                    }
-                                }
-
-                                return LineResult::Success(line.to_string());
-                            }
-                            Err(err) => return LineResult::Error(line.to_string(), err),
-                        }
-                    }
-
-                    (
-                        Some(ClassifiedCommand::External(left)),
-                        Some(ClassifiedCommand::External(_)),
-                    ) => match run_external_command(left, ctx, input, StreamNext::External).await {
-                        Ok(val) => val,
-                        Err(err) => return LineResult::Error(line.to_string(), err),
-                    },
-
-                    (Some(ClassifiedCommand::External(left)), Some(_)) => {
-                        match run_external_command(left, ctx, input, StreamNext::Internal).await {
-                            Ok(val) => val,
-                            Err(err) => return LineResult::Error(line.to_string(), err),
-                        }
-                    }
-
-                    (Some(ClassifiedCommand::External(left)), None) => {
-                        match run_external_command(left, ctx, input, StreamNext::Last).await {
-                            Ok(val) => val,
-                            Err(err) => return LineResult::Error(line.to_string(), err),
-                        }
-                    }
-                };
+            match run_pipeline(pipeline, ctx, input, line).await {
+                Ok(_) => LineResult::Success(line.to_string()),
+                Err(err) => LineResult::Error(line.to_string(), err),
             }
-
-            LineResult::Success(line.to_string())
         }
         Err(ReadlineError::Interrupted) => LineResult::CtrlC,
         Err(ReadlineError::Eof) => LineResult::Break,

src/commands/classified/mod.rs

@@ -4,6 +4,7 @@ use crate::prelude::*;
 mod dynamic;
 pub(crate) mod external;
 pub(crate) mod internal;
+pub(crate) mod pipeline;
 
 #[allow(unused_imports)]
 pub(crate) use dynamic::Command as DynamicCommand;

src/commands/classified/pipeline.rs

@@ -1,40 +1,74 @@
-use super::ClassifiedCommand;
-use crate::prelude::*;
+use crate::commands::classified::external::{run_external_command, StreamNext};
+use crate::commands::classified::internal::run_internal_command;
+use crate::commands::classified::ClassifiedInputStream;
+use crate::context::Context;
+use crate::stream::OutputStream;
+use nu_errors::ShellError;
+use nu_parser::{ClassifiedCommand, ClassifiedPipeline};
+use nu_protocol::{ReturnSuccess, UntaggedValue, Value};
+use nu_source::Text;
+use std::sync::atomic::Ordering;
 
-#[derive(Debug, Clone)]
-pub(crate) struct Pipeline {
-    pub(crate) commands: ClassifiedCommands,
-}
+pub(crate) async fn run_pipeline(
+    pipeline: ClassifiedPipeline,
+    ctx: &mut Context,
+    mut input: ClassifiedInputStream,
+    line: &str,
+) -> Result<(), ShellError> {
+    let mut iter = pipeline.commands.list.into_iter().peekable();
 
-impl Pipeline {
-    pub fn commands(list: Vec<ClassifiedCommand>, span: impl Into<Span>) -> Pipeline {
-        Pipeline {
-            commands: ClassifiedCommands {
-                list,
-                span: span.into(),
-            },
+    loop {
+        let item: Option<ClassifiedCommand> = iter.next();
+        let next: Option<&ClassifiedCommand> = iter.peek();
+
+        input = match (item, next) {
+            (Some(ClassifiedCommand::Dynamic(_)), _) | (_, Some(ClassifiedCommand::Dynamic(_))) => {
+                return Err(ShellError::unimplemented("Dynamic commands"))
+            }
+
+            (Some(ClassifiedCommand::Expr(_)), _) | (_, Some(ClassifiedCommand::Expr(_))) => {
+                return Err(ShellError::unimplemented("Expression-only commands"))
+            }
+
+            (Some(ClassifiedCommand::Internal(left)), _) => {
+                let stream = run_internal_command(left, ctx, input, Text::from(line)).await?;
+                ClassifiedInputStream::from_input_stream(stream)
+            }
+
+            (Some(ClassifiedCommand::External(left)), Some(ClassifiedCommand::External(_))) => {
+                run_external_command(left, ctx, input, StreamNext::External).await?
+            }
+
+            (Some(ClassifiedCommand::External(left)), Some(_)) => {
+                run_external_command(left, ctx, input, StreamNext::Internal).await?
+            }
+
+            (Some(ClassifiedCommand::External(left)), None) => {
+                run_external_command(left, ctx, input, StreamNext::Last).await?
+            }
+
+            (None, _) => break,
+        };
+    }
+
+    use futures::stream::TryStreamExt;
+    let mut output_stream: OutputStream = input.objects.into();
+    loop {
+        match output_stream.try_next().await {
+            Ok(Some(ReturnSuccess::Value(Value {
+                value: UntaggedValue::Error(e),
+                ..
+            }))) => return Err(e),
+            Ok(Some(_item)) => {
+                if ctx.ctrl_c.load(Ordering::SeqCst) {
+                    break;
+                }
+            }
+            _ => {
+                break;
+            }
         }
     }
-}
 
-#[derive(Debug, Clone)]
-pub struct ClassifiedCommands {
-    pub list: Vec<ClassifiedCommand>,
-    pub span: Span,
-}
-
-impl HasSpan for Pipeline {
-    fn span(&self) -> Span {
-        self.commands.span
-    }
-}
-
-impl PrettyDebugWithSource for Pipeline {
-    fn pretty_debug(&self, source: &str) -> DebugDocBuilder {
-        b::intersperse(
-            self.commands.list.iter().map(|c| c.pretty_debug(source)),
-            b::operator(" | "),
-        )
-        .or(b::delimit("<", b::description("empty pipeline"), ">"))
-    }
+    Ok(())
 }