Revert "Span ID Refactor (Step 2): Make Call SpanId-friendly (#13268)" (#13292)

This reverts commit 0cfd5fbece.

The original PR messed up syntax higlighting of aliases and causes
panics of completion in the presence of alias.

<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the
tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->

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

@@ -4,47 +4,42 @@ use serde::{Deserialize, Serialize};
 
 use crate::{
     ast::Expression, engine::StateWorkingSet, eval_const::eval_constant, DeclId, FromValue,
-    GetSpan, ShellError, Span, SpanId, Spanned, Value,
+    ShellError, Span, Spanned, Value,
 };
 
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
 pub enum Argument {
     Positional(Expression),
-    Named(
-        (
-            Spanned<String>,
-            Option<Spanned<String>>,
-            Option<Expression>,
-            SpanId,
-        ),
-    ),
+    Named((Spanned<String>, Option<Spanned<String>>, Option<Expression>)),
     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, state: &impl GetSpan) -> Span {
+    pub fn span(&self) -> Span {
         match self {
-            Argument::Positional(e) => e.span(state),
-            Argument::Named((_, _, _, span_id)) => state.get_span(*span_id),
-            Argument::Unknown(e) => e.span(state),
-            Argument::Spread(e) => e.span(state),
+            Argument::Positional(e) => e.span,
+            Argument::Named((named, short, expr)) => {
+                let start = named.span.start;
+                let end = if let Some(expr) = expr {
+                    expr.span.end
+                } else if let Some(short) = short {
+                    short.span.end
+                } else {
+                    named.span.end
+                };
+
+                Span::new(start, end)
+            }
+            Argument::Unknown(e) => e.span,
+            Argument::Spread(e) => e.span,
         }
     }
 
     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)
             }
@@ -63,20 +58,17 @@ pub struct Call {
     /// identifier of the declaration to call
     pub decl_id: DeclId,
     pub head: Span,
-    pub arguments: Spanned<Vec<Argument>>,
+    pub arguments: Vec<Argument>,
     /// this field is used by the parser to pass additional command-specific information
     pub parser_info: HashMap<String, Expression>,
 }
 
 impl Call {
-    pub fn new(head: Span, args_span: Span) -> Call {
+    pub fn new(head: Span) -> Call {
         Self {
             decl_id: 0,
             head,
-            arguments: Spanned {
-                item: vec![],
-                span: args_span,
-            },
+            arguments: vec![],
             parser_info: HashMap::new(),
         }
     }
@@ -88,20 +80,23 @@ impl Call {
     /// If there are one or more arguments the span encompasses the start of the first argument to
     /// end of the last argument
     pub fn arguments_span(&self) -> Span {
-        self.arguments.span
+        let past = self.head.past();
+
+        let start = self
+            .arguments
+            .first()
+            .map(|a| a.span())
+            .unwrap_or(past)
+            .start;
+        let end = self.arguments.last().map(|a| a.span()).unwrap_or(past).end;
+
+        Span::new(start, end)
     }
 
     pub fn named_iter(
         &self,
-    ) -> impl Iterator<
-        Item = &(
-            Spanned<String>,
-            Option<Spanned<String>>,
-            Option<Expression>,
-            SpanId,
-        ),
-    > {
-        self.arguments.item.iter().filter_map(|arg| match arg {
+    ) -> impl Iterator<Item = &(Spanned<String>, Option<Spanned<String>>, Option<Expression>)> {
+        self.arguments.iter().filter_map(|arg| match arg {
             Argument::Named(named) => Some(named),
             Argument::Positional(_) => None,
             Argument::Unknown(_) => None,
@@ -111,15 +106,9 @@ impl Call {
 
     pub fn named_iter_mut(
         &mut self,
-    ) -> impl Iterator<
-        Item = &mut (
-            Spanned<String>,
-            Option<Spanned<String>>,
-            Option<Expression>,
-            SpanId,
-        ),
-    > {
-        self.arguments.item.iter_mut().filter_map(|arg| match arg {
+    ) -> impl Iterator<Item = &mut (Spanned<String>, Option<Spanned<String>>, Option<Expression>)>
+    {
+        self.arguments.iter_mut().filter_map(|arg| match arg {
             Argument::Named(named) => Some(named),
             Argument::Positional(_) => None,
             Argument::Unknown(_) => None,
@@ -133,31 +122,25 @@ impl Call {
 
     pub fn add_named(
         &mut self,
-        named: (
-            Spanned<String>,
-            Option<Spanned<String>>,
-            Option<Expression>,
-            SpanId,
-        ),
+        named: (Spanned<String>, Option<Spanned<String>>, Option<Expression>),
     ) {
-        self.arguments.item.push(Argument::Named(named));
+        self.arguments.push(Argument::Named(named));
     }
 
     pub fn add_positional(&mut self, positional: Expression) {
-        self.arguments.item.push(Argument::Positional(positional));
+        self.arguments.push(Argument::Positional(positional));
     }
 
     pub fn add_unknown(&mut self, unknown: Expression) {
-        self.arguments.item.push(Argument::Unknown(unknown));
+        self.arguments.push(Argument::Unknown(unknown));
     }
 
     pub fn add_spread(&mut self, args: Expression) {
-        self.arguments.item.push(Argument::Spread(args));
+        self.arguments.push(Argument::Spread(args));
     }
 
     pub fn positional_iter(&self) -> impl Iterator<Item = &Expression> {
         self.arguments
-            .item
             .iter()
             .take_while(|arg| match arg {
                 Argument::Spread(_) => false, // Don't include positional arguments given to rest parameter
@@ -185,7 +168,6 @@ impl Call {
         // todo maybe rewrite to be more elegant or something
         let args = self
             .arguments
-            .item
             .iter()
             .filter_map(|arg| match arg {
                 Argument::Named(_) => None,
@@ -276,9 +258,9 @@ impl Call {
     ) -> Result<Vec<T>, ShellError> {
         let mut output = vec![];
 
-        for result in self.rest_iter_flattened(&working_set, starting_pos, |expr| {
-            eval_constant(working_set, expr)
-        })? {
+        for result in
+            self.rest_iter_flattened(starting_pos, |expr| eval_constant(working_set, expr))?
+        {
             output.push(FromValue::from_value(result)?);
         }
 
@@ -287,7 +269,6 @@ impl Call {
 
     pub fn rest_iter_flattened<F>(
         &self,
-        state: &impl GetSpan,
         start: usize,
         mut eval: F,
     ) -> Result<Vec<Value>, ShellError>
@@ -301,11 +282,7 @@ impl Call {
             if spread {
                 match result {
                     Value::List { mut vals, .. } => output.append(&mut vals),
-                    _ => {
-                        return Err(ShellError::CannotSpreadAsList {
-                            span: expr.span(state),
-                        })
-                    }
+                    _ => return Err(ShellError::CannotSpreadAsList { span: expr.span }),
                 }
             } else {
                 output.push(result);
@@ -333,23 +310,23 @@ impl Call {
         }
     }
 
-    pub fn span(&self, state: &impl GetSpan) -> Span {
+    pub fn span(&self) -> Span {
         let mut span = self.head;
 
         for positional in self.positional_iter() {
-            if positional.span(state).end > span.end {
-                span.end = positional.span(state).end;
+            if positional.span.end > span.end {
+                span.end = positional.span.end;
             }
         }
 
-        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;
             }
 
             if let Some(val) = &val {
-                if val.span(state).end > span.end {
-                    span.end = val.span(state).end;
+                if val.span.end > span.end {
+                    span.end = val.span.end;
                 }
             }
         }
@@ -357,3 +334,77 @@ impl Call {
         span
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use crate::engine::EngineState;
+
+    #[test]
+    fn argument_span_named() {
+        let engine_state = EngineState::new();
+        let mut working_set = StateWorkingSet::new(&engine_state);
+
+        let named = Spanned {
+            item: "named".to_string(),
+            span: Span::new(2, 3),
+        };
+        let short = Spanned {
+            item: "short".to_string(),
+            span: Span::new(5, 7),
+        };
+        let expr = Expression::garbage(&mut working_set, Span::new(11, 13));
+
+        let arg = Argument::Named((named.clone(), None, None));
+
+        assert_eq!(Span::new(2, 3), arg.span());
+
+        let arg = Argument::Named((named.clone(), Some(short.clone()), None));
+
+        assert_eq!(Span::new(2, 7), arg.span());
+
+        let arg = Argument::Named((named.clone(), None, Some(expr.clone())));
+
+        assert_eq!(Span::new(2, 13), arg.span());
+
+        let arg = Argument::Named((named.clone(), Some(short.clone()), Some(expr.clone())));
+
+        assert_eq!(Span::new(2, 13), arg.span());
+    }
+
+    #[test]
+    fn argument_span_positional() {
+        let engine_state = EngineState::new();
+        let mut working_set = StateWorkingSet::new(&engine_state);
+
+        let span = Span::new(2, 3);
+        let expr = Expression::garbage(&mut working_set, span);
+        let arg = Argument::Positional(expr);
+
+        assert_eq!(span, arg.span());
+    }
+
+    #[test]
+    fn argument_span_unknown() {
+        let engine_state = EngineState::new();
+        let mut working_set = StateWorkingSet::new(&engine_state);
+
+        let span = Span::new(2, 3);
+        let expr = Expression::garbage(&mut working_set, span);
+        let arg = Argument::Unknown(expr);
+
+        assert_eq!(span, arg.span());
+    }
+
+    #[test]
+    fn call_arguments_span() {
+        let engine_state = EngineState::new();
+        let mut working_set = StateWorkingSet::new(&engine_state);
+
+        let mut call = Call::new(Span::new(0, 1));
+        call.add_positional(Expression::garbage(&mut working_set, Span::new(2, 3)));
+        call.add_positional(Expression::garbage(&mut working_set, Span::new(5, 7)));
+
+        assert_eq!(Span::new(2, 7), call.arguments_span());
+    }
+}

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

@@ -173,7 +173,7 @@ impl Expression {
             Expr::Binary(_) => false,
             Expr::Bool(_) => false,
             Expr::Call(call) => {
-                for arg in &call.arguments.item {
+                for arg in &call.arguments {
                     match arg {
                         Argument::Positional(expr)
                         | Argument::Unknown(expr)
@@ -366,7 +366,7 @@ impl Expression {
                 if replaced.contains_span(call.head) {
                     call.head = new_span;
                 }
-                for arg in call.arguments.item.iter_mut() {
+                for arg in call.arguments.iter_mut() {
                     match arg {
                         Argument::Positional(expr)
                         | Argument::Unknown(expr)

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

@@ -1023,32 +1023,16 @@ impl<'a> StateWorkingSet<'a> {
 
 impl<'a> GetSpan for &'a StateWorkingSet<'a> {
     fn get_span(&self, span_id: SpanId) -> Span {
-        get_span(self, span_id)
-    }
-}
-
-impl<'a> GetSpan for &'a mut StateWorkingSet<'a> {
-    fn get_span(&self, span_id: SpanId) -> Span {
-        get_span(self, span_id)
-    }
-}
-
-impl<'a> GetSpan for StateWorkingSet<'a> {
-    fn get_span(&self, span_id: SpanId) -> Span {
-        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 {
-        working_set.permanent_state.get_span(span_id)
-    } else {
-        *working_set
-            .delta
-            .spans
-            .get(span_id.0 - num_permanent_spans)
-            .expect("internal error: missing span")
+        let num_permanent_spans = self.permanent_state.num_spans();
+        if span_id.0 < num_permanent_spans {
+            self.permanent_state.get_span(span_id)
+        } else {
+            *self
+                .delta
+                .spans
+                .get(span_id.0 - num_permanent_spans)
+                .expect("internal error: missing span")
+        }
     }
 }
 

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 });

crates/nu-protocol/src/pipeline/pipeline_data.rs

@@ -567,7 +567,7 @@ impl PipelineData {
                     if command.block_id().is_some() {
                         self.write_all_and_flush(engine_state, no_newline, to_stderr)
                     } else {
-                        let call = Call::new(Span::unknown(), Span::unknown());
+                        let call = Call::new(Span::new(0, 0));
                         let table = command.run(engine_state, stack, &call, self)?;
                         table.write_all_and_flush(engine_state, no_newline, to_stderr)
                     }