Refactor named call args to not require expr span

crates/nu-cli/src/commands/keybindings_list.rs

@@ -62,7 +62,7 @@ impl Command for KeybindingsList {
                 .collect()
         } else {
             call.named_iter()
-                .flat_map(|(argument, _, _)| get_records(argument.item.as_str(), call.head))
+                .flat_map(|(argument, _, _, _)| get_records(argument.item.as_str(), call.head))
                 .collect()
         };
 

crates/nu-command/src/debug/explain.rs

@@ -115,7 +115,7 @@ fn get_arguments(
         match arg {
             // I think the second argument to Argument::Named is the short name, but I'm not really sure.
             // Please fix it if it's wrong. :)
-            Argument::Named((name, short, opt_expr)) => {
+            Argument::Named((name, short, opt_expr, _)) => {
                 let arg_type = "named";
                 let arg_value_name = name.item.clone();
                 let arg_value_name_span_start = name.span.start as i64;

crates/nu-command/src/generators/cal.rs

@@ -3,6 +3,7 @@ use nu_color_config::StyleComputer;
 use nu_engine::command_prelude::*;
 use nu_protocol::ast::{Expr, Expression};
 
+use nu_protocol::engine::UNKNOWN_SPAN_ID;
 use std::collections::VecDeque;
 
 #[derive(Clone)]
@@ -155,6 +156,7 @@ pub fn cal(
             Span::unknown(),
             Type::Bool,
         )),
+        UNKNOWN_SPAN_ID,
     ));
 
     let cal_table_output =

crates/nu-engine/src/documentation.rs

@@ -306,6 +306,7 @@ fn get_documentation(
                     },
                     None,
                     None,
+                    UNKNOWN_SPAN_ID,
                 ))
             } else {
                 // expand the result
@@ -316,6 +317,7 @@ fn get_documentation(
                     },
                     None,
                     None,
+                    UNKNOWN_SPAN_ID,
                 ))
             }
             let table = engine_state

crates/nu-engine/src/eval.rs

@@ -26,7 +26,7 @@ pub fn eval_call<D: DebugContext>(
     }
     let decl = engine_state.get_decl(call.decl_id);
 
-    if !decl.is_known_external() && call.named_iter().any(|(flag, _, _)| flag.item == "help") {
+    if !decl.is_known_external() && call.named_iter().any(|(flag, _, _, _)| flag.item == "help") {
         let help = get_full_help(decl, engine_state, caller_stack);
         Ok(Value::string(help, call.head).into_pipeline_data())
     } else if let Some(block_id) = decl.block_id() {

crates/nu-parser/src/parse_keywords.rs

@@ -123,7 +123,7 @@ pub fn parse_keyword(working_set: &mut StateWorkingSet, lite_command: &LiteComma
         // Apply parse keyword side effects
         let cmd = working_set.get_decl(call.decl_id);
         // check help flag first.
-        if call.named_iter().any(|(flag, _, _)| flag.item == "help") {
+        if call.named_iter().any(|(flag, _, _, _)| flag.item == "help") {
             let call_span = call.span();
             return Pipeline::from_vec(vec![Expression::new(
                 working_set,
@@ -3884,8 +3884,8 @@ pub fn parse_plugin_use(working_set: &mut StateWorkingSet, call: Box<Call>) -> P
 
         let plugin_config = call
             .named_iter()
-            .find(|(arg_name, _, _)| arg_name.item == "plugin-config")
+            .find(|(arg_name, _, _, _)| arg_name.item == "plugin-config")
-            .map(|(_, _, expr)| {
+            .map(|(_, _, expr, _)| {
                 let expr = expr
                     .as_ref()
                     .expect("--plugin-config arg should have been checked already");

crates/nu-parser/src/parser.rs

@@ -160,7 +160,7 @@ pub(crate) fn check_call(
     call: &Call,
 ) {
     // Allow the call to pass if they pass in the help flag
-    if call.named_iter().any(|(n, _, _)| n.item == "help") {
+    if call.named_iter().any(|(n, _, _, _)| n.item == "help") {
         return;
     }
 
@@ -211,7 +211,10 @@ pub(crate) fn check_call(
         }
     } else {
         for req_flag in sig.named.iter().filter(|x| x.required) {
-            if call.named_iter().all(|(n, _, _)| n.item != req_flag.long) {
+            if call
+                .named_iter()
+                .all(|(n, _, _, _)| n.item != req_flag.long)
+            {
                 working_set.error(ParseError::MissingRequiredFlag(
                     req_flag.long.clone(),
                     command,
@@ -916,6 +919,25 @@ pub struct ParsedInternalCall {
     pub output: Type,
 }
 
+// moved from call.rs since span creation must be done at parse time now
+fn named_arg_span(
+    working_set: &StateWorkingSet,
+    named: &Spanned<String>,
+    short: &Option<Spanned<String>>,
+    expr: &Option<Expression>,
+) -> Span {
+    let start = named.span.start;
+    let end = if let Some(expr) = expr {
+        expr.span(&working_set).end
+    } else if let Some(short) = short {
+        short.span.end
+    } else {
+        named.span.end
+    };
+
+    Span::new(start, end)
+}
+
 pub fn parse_internal_call(
     working_set: &mut StateWorkingSet,
     command_span: Span,
@@ -1002,7 +1024,9 @@ pub fn parse_internal_call(
 
                 call.add_unknown(arg);
             } else {
-                call.add_named((long_name, None, arg));
+                let named_span = named_arg_span(working_set, &long_name, &None, &arg);
+                let named_span_id = working_set.add_span(named_span);
+                call.add_named((long_name, None, arg, named_span_id));
             }
 
             spans_idx += 1;
@@ -1053,27 +1077,30 @@ pub fn parse_internal_call(
 
                             if flag.long.is_empty() {
                                 if let Some(short) = flag.short {
-                                    call.add_named((
+                                    let named = Spanned {
-                                        Spanned {
                                         item: String::new(),
                                         span: spans[spans_idx],
-                                        },
+                                    };
-                                        Some(Spanned {
+                                    let short = Some(Spanned {
                                         item: short.to_string(),
                                         span: spans[spans_idx],
-                                        }),
+                                    });
-                                        Some(arg),
+                                    let expr = Some(arg);
-                                    ));
+                                    let named_span =
+                                        named_arg_span(working_set, &named, &short, &expr);
+                                    let named_span_id = working_set.add_span(named_span);
+                                    call.add_named((named, short, expr, named_span_id));
                                 }
                             } else {
-                                call.add_named((
+                                let named = Spanned {
-                                    Spanned {
                                     item: flag.long.clone(),
                                     span: spans[spans_idx],
-                                    },
+                                };
-                                    None,
+                                let short = None;
-                                    Some(arg),
+                                let expr = Some(arg);
-                                ));
+                                let named_span = named_arg_span(working_set, &named, &short, &expr);
+                                let named_span_id = working_set.add_span(named_span);
+                                call.add_named((named, short, expr, named_span_id));
                             }
                             spans_idx += 1;
                         } else {
@@ -1084,27 +1111,29 @@ pub fn parse_internal_call(
                         }
                     } else if flag.long.is_empty() {
                         if let Some(short) = flag.short {
-                            call.add_named((
+                            let named = Spanned {
-                                Spanned {
                                 item: String::new(),
                                 span: spans[spans_idx],
-                                },
+                            };
-                                Some(Spanned {
+                            let short = Some(Spanned {
                                 item: short.to_string(),
                                 span: spans[spans_idx],
-                                }),
+                            });
-                                None,
+                            let expr = None;
-                            ));
+                            let named_span = named_arg_span(working_set, &named, &short, &expr);
+                            let named_span_id = working_set.add_span(named_span);
+                            call.add_named((named, short, expr, named_span_id));
                         }
                     } else {
-                        call.add_named((
+                        let named = Spanned {
-                            Spanned {
                             item: flag.long.clone(),
                             span: spans[spans_idx],
-                            },
+                        };
-                            None,
+                        let short = None;
-                            None,
+                        let expr = None;
-                        ));
+                        let named_span = named_arg_span(working_set, &named, &short, &expr);
+                        let named_span_id = working_set.add_span(named_span);
+                        call.add_named((named, short, expr, named_span_id));
                     }
                 }
             }
@@ -6324,14 +6353,15 @@ fn wrap_expr_with_collect(working_set: &mut StateWorkingSet, expr: &Expression)
             Type::Any,
         )));
 
-        output.push(Argument::Named((
+        let named = Spanned {
-            Spanned {
             item: "keep-env".to_string(),
             span: Span::new(0, 0),
-            },
+        };
-            None,
+        let short = None;
-            None,
+        let expr = None;
-        )));
+        let named_span = named_arg_span(working_set, &named, &short, &expr);
+        let named_span_id = working_set.add_span(named_span);
+        output.push(Argument::Named((named, short, expr, named_span_id)));
 
         // The containing, synthetic call to `collect`.
         // We don't want to have a real span as it will confuse flattening

crates/nu-parser/tests/test_parser.rs

@@ -599,8 +599,8 @@ pub fn parse_call_short_flag_batch_arg_allowed() {
     if let Expr::Call(call) = &element.expr.expr {
         assert_eq!(call.decl_id, 0);
         assert_eq!(call.arguments.len(), 2);
-        matches!(call.arguments[0], Argument::Named((_, None, None)));
+        matches!(call.arguments[0], Argument::Named((_, None, None, _)));
-        matches!(call.arguments[1], Argument::Named((_, None, Some(_))));
+        matches!(call.arguments[1], Argument::Named((_, None, Some(_), _)));
     }
 }
 

crates/nu-plugin-protocol/src/evaluated_call.rs

@@ -38,7 +38,7 @@ impl EvaluatedCall {
             call.rest_iter_flattened(0, |expr| eval_expression_fn(engine_state, stack, expr))?;
 
         let mut named = Vec::with_capacity(call.named_len());
-        for (string, _, expr) in call.named_iter() {
+        for (string, _, expr, _) in call.named_iter() {
             let value = match expr {
                 None => None,
                 Some(expr) => Some(eval_expression_fn(engine_state, stack, expr)?),

crates/nu-protocol/src/ast/call.rs

@@ -4,23 +4,39 @@ use serde::{Deserialize, Serialize};
 
 use crate::{
     ast::Expression, engine::StateWorkingSet, eval_const::eval_constant, DeclId, FromValue,
-    ShellError, Span, Spanned, Value,
+    ShellError, Span, SpanId, Spanned, Value,
 };
 
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
 pub enum Argument {
     Positional(Expression),
-    Named((Spanned<String>, Option<Spanned<String>>, Option<Expression>)),
+    Named(
+        (
+            Spanned<String>,
+            Option<Spanned<String>>,
+            Option<Expression>,
+            SpanId,
+        ),
+    ),
     Unknown(Expression), // unknown argument used in "fall-through" signatures
     Spread(Expression),  // a list spread to fill in rest arguments
 }
 
 impl Argument {
+    pub fn span_id(&self) -> SpanId {
+        match self {
+            Argument::Positional(e) => e.span_id,
+            Argument::Named((_, _, _, span_id)) => *span_id,
+            Argument::Unknown(e) => e.span_id,
+            Argument::Spread(e) => e.span_id,
+        }
+    }
+
     /// The span for an argument
     pub fn span(&self) -> Span {
         match self {
             Argument::Positional(e) => e.span,
-            Argument::Named((named, short, expr)) => {
+            Argument::Named((named, short, expr, _)) => {
                 let start = named.span.start;
                 let end = if let Some(expr) = expr {
                     expr.span.end
@@ -39,7 +55,7 @@ impl Argument {
 
     pub fn expr(&self) -> Option<&Expression> {
         match self {
-            Argument::Named((_, _, expr)) => expr.as_ref(),
+            Argument::Named((_, _, expr, _)) => expr.as_ref(),
             Argument::Positional(expr) | Argument::Unknown(expr) | Argument::Spread(expr) => {
                 Some(expr)
             }
@@ -95,7 +111,14 @@ impl Call {
 
     pub fn named_iter(
         &self,
-    ) -> impl Iterator<Item = &(Spanned<String>, Option<Spanned<String>>, Option<Expression>)> {
+    ) -> impl Iterator<
+        Item = &(
+            Spanned<String>,
+            Option<Spanned<String>>,
+            Option<Expression>,
+            SpanId,
+        ),
+    > {
         self.arguments.iter().filter_map(|arg| match arg {
             Argument::Named(named) => Some(named),
             Argument::Positional(_) => None,
@@ -106,8 +129,14 @@ impl Call {
 
     pub fn named_iter_mut(
         &mut self,
-    ) -> impl Iterator<Item = &mut (Spanned<String>, Option<Spanned<String>>, Option<Expression>)>
+    ) -> impl Iterator<
-    {
+        Item = &mut (
+            Spanned<String>,
+            Option<Spanned<String>>,
+            Option<Expression>,
+            SpanId,
+        ),
+    > {
         self.arguments.iter_mut().filter_map(|arg| match arg {
             Argument::Named(named) => Some(named),
             Argument::Positional(_) => None,
@@ -122,7 +151,12 @@ impl Call {
 
     pub fn add_named(
         &mut self,
-        named: (Spanned<String>, Option<Spanned<String>>, Option<Expression>),
+        named: (
+            Spanned<String>,
+            Option<Spanned<String>>,
+            Option<Expression>,
+            SpanId,
+        ),
     ) {
         self.arguments.push(Argument::Named(named));
     }
@@ -319,7 +353,7 @@ impl Call {
             }
         }
 
-        for (named, _, val) in self.named_iter() {
+        for (named, _, val, _) in self.named_iter() {
             if named.span.end > span.end {
                 span.end = named.span.end;
             }
@@ -338,7 +372,7 @@ impl Call {
 #[cfg(test)]
 mod test {
     use super::*;
-    use crate::engine::EngineState;
+    use crate::engine::{EngineState, UNKNOWN_SPAN_ID};
 
     #[test]
     fn argument_span_named() {
@@ -355,19 +389,24 @@ mod test {
         };
         let expr = Expression::garbage(&mut working_set, Span::new(11, 13));
 
-        let arg = Argument::Named((named.clone(), None, None));
+        let arg = Argument::Named((named.clone(), None, None, UNKNOWN_SPAN_ID));
 
         assert_eq!(Span::new(2, 3), arg.span());
 
-        let arg = Argument::Named((named.clone(), Some(short.clone()), None));
+        let arg = Argument::Named((named.clone(), Some(short.clone()), None, UNKNOWN_SPAN_ID));
 
         assert_eq!(Span::new(2, 7), arg.span());
 
-        let arg = Argument::Named((named.clone(), None, Some(expr.clone())));
+        let arg = Argument::Named((named.clone(), None, Some(expr.clone()), UNKNOWN_SPAN_ID));
 
         assert_eq!(Span::new(2, 13), arg.span());
 
-        let arg = Argument::Named((named.clone(), Some(short.clone()), Some(expr.clone())));
+        let arg = Argument::Named((
+            named.clone(),
+            Some(short.clone()),
+            Some(expr.clone()),
+            UNKNOWN_SPAN_ID,
+        ));
 
         assert_eq!(Span::new(2, 13), arg.span());
     }

crates/nu-protocol/src/engine/state_working_set.rs

@@ -1023,18 +1023,22 @@ impl<'a> StateWorkingSet<'a> {
 
 impl<'a> GetSpan for &'a StateWorkingSet<'a> {
     fn get_span(&self, span_id: SpanId) -> Span {
-        let num_permanent_spans = self.permanent_state.num_spans();
+        get_span(self, span_id)
+    }
+}
+
+fn get_span(working_set: &StateWorkingSet, span_id: SpanId) -> Span {
+    let num_permanent_spans = working_set.permanent_state.num_spans();
     if span_id.0 < num_permanent_spans {
-            self.permanent_state.get_span(span_id)
+        working_set.permanent_state.get_span(span_id)
     } else {
-            *self
+        *working_set
             .delta
             .spans
             .get(span_id.0 - num_permanent_spans)
             .expect("internal error: missing span")
     }
 }
-}
 
 impl<'a> miette::SourceCode for &StateWorkingSet<'a> {
     fn read_span<'b>(

crates/nu-protocol/src/eval_const.rs

@@ -301,7 +301,7 @@ fn eval_const_call(
         return Err(ShellError::NotAConstCommand { span: call.head });
     }
 
-    if !decl.is_known_external() && call.named_iter().any(|(flag, _, _)| flag.item == "help") {
+    if !decl.is_known_external() && call.named_iter().any(|(flag, _, _, _)| flag.item == "help") {
         // It would require re-implementing get_full_help() for const evaluation. Assuming that
         // getting help messages at parse-time is rare enough, we can simply disallow it.
         return Err(ShellError::NotAConstHelp { span: call.head });