From 760795bc2e50e91852b6fae07dc526620036874b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= Date: Sat, 29 Jun 2024 13:24:01 +0300 Subject: [PATCH] Refactor named call args to not require expr span --- .../nu-cli/src/commands/keybindings_list.rs | 2 +- crates/nu-command/src/debug/explain.rs | 2 +- crates/nu-command/src/generators/cal.rs | 2 + crates/nu-engine/src/documentation.rs | 2 + crates/nu-engine/src/eval.rs | 2 +- crates/nu-parser/src/parse_keywords.rs | 6 +- crates/nu-parser/src/parser.rs | 128 +++++++++++------- crates/nu-parser/tests/test_parser.rs | 4 +- .../nu-plugin-protocol/src/evaluated_call.rs | 2 +- crates/nu-protocol/src/ast/call.rs | 67 +++++++-- .../src/engine/state_working_set.rs | 24 ++-- crates/nu-protocol/src/eval_const.rs | 2 +- 12 files changed, 160 insertions(+), 83 deletions(-) diff --git a/crates/nu-cli/src/commands/keybindings_list.rs b/crates/nu-cli/src/commands/keybindings_list.rs index f4450c0c23..b6375be8a1 100644 --- a/crates/nu-cli/src/commands/keybindings_list.rs +++ b/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() }; diff --git a/crates/nu-command/src/debug/explain.rs b/crates/nu-command/src/debug/explain.rs index b451d6916a..5bd83f223a 100644 --- a/crates/nu-command/src/debug/explain.rs +++ b/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; diff --git a/crates/nu-command/src/generators/cal.rs b/crates/nu-command/src/generators/cal.rs index a257f3ab7c..bc74dbbd75 100644 --- a/crates/nu-command/src/generators/cal.rs +++ b/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 = diff --git a/crates/nu-engine/src/documentation.rs b/crates/nu-engine/src/documentation.rs index a7d4950036..8b82ff91b3 100644 --- a/crates/nu-engine/src/documentation.rs +++ b/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 diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 044bf86980..32957aa387 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -26,7 +26,7 @@ pub fn eval_call( } 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() { diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 70d217c564..25411f4f13 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/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) -> P let plugin_config = call .named_iter() - .find(|(arg_name, _, _)| arg_name.item == "plugin-config") - .map(|(_, _, expr)| { + .find(|(arg_name, _, _, _)| arg_name.item == "plugin-config") + .map(|(_, _, expr, _)| { let expr = expr .as_ref() .expect("--plugin-config arg should have been checked already"); diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index c3d18b55ac..02912dc74f 100644 --- a/crates/nu-parser/src/parser.rs +++ b/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, + short: &Option>, + expr: &Option, +) -> 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(( - Spanned { - item: String::new(), - span: spans[spans_idx], - }, - Some(Spanned { - item: short.to_string(), - span: spans[spans_idx], - }), - Some(arg), - )); + let named = Spanned { + item: String::new(), + span: spans[spans_idx], + }; + let short = Some(Spanned { + item: short.to_string(), + span: spans[spans_idx], + }); + 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(( - Spanned { - item: flag.long.clone(), - span: spans[spans_idx], - }, - None, - Some(arg), - )); + let named = Spanned { + item: flag.long.clone(), + span: spans[spans_idx], + }; + let short = None; + 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(( - Spanned { - item: String::new(), - span: spans[spans_idx], - }, - Some(Spanned { - item: short.to_string(), - span: spans[spans_idx], - }), - None, - )); + let named = Spanned { + item: String::new(), + span: spans[spans_idx], + }; + let short = Some(Spanned { + item: short.to_string(), + span: spans[spans_idx], + }); + 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(( - Spanned { - item: flag.long.clone(), - span: spans[spans_idx], - }, - None, - None, - )); + let named = Spanned { + item: flag.long.clone(), + span: spans[spans_idx], + }; + let short = 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(( - Spanned { - item: "keep-env".to_string(), - span: Span::new(0, 0), - }, - None, - None, - ))); + let named = Spanned { + item: "keep-env".to_string(), + span: Span::new(0, 0), + }; + let short = 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 diff --git a/crates/nu-parser/tests/test_parser.rs b/crates/nu-parser/tests/test_parser.rs index 0784fe69d4..aff738de47 100644 --- a/crates/nu-parser/tests/test_parser.rs +++ b/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[1], Argument::Named((_, None, Some(_)))); + matches!(call.arguments[0], Argument::Named((_, None, None, _))); + matches!(call.arguments[1], Argument::Named((_, None, Some(_), _))); } } diff --git a/crates/nu-plugin-protocol/src/evaluated_call.rs b/crates/nu-plugin-protocol/src/evaluated_call.rs index 19f9049340..1bf90eb723 100644 --- a/crates/nu-plugin-protocol/src/evaluated_call.rs +++ b/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)?), diff --git a/crates/nu-protocol/src/ast/call.rs b/crates/nu-protocol/src/ast/call.rs index 6e8cc64a05..e130b6a633 100644 --- a/crates/nu-protocol/src/ast/call.rs +++ b/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, Option>, Option)), + Named( + ( + Spanned, + Option>, + Option, + 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, Option>, Option)> { + ) -> impl Iterator< + Item = &( + Spanned, + Option>, + Option, + 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, Option>, Option)> - { + ) -> impl Iterator< + Item = &mut ( + Spanned, + Option>, + Option, + 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, Option>, Option), + named: ( + Spanned, + Option>, + Option, + 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()); } diff --git a/crates/nu-protocol/src/engine/state_working_set.rs b/crates/nu-protocol/src/engine/state_working_set.rs index af950b8321..823a9267fa 100644 --- a/crates/nu-protocol/src/engine/state_working_set.rs +++ b/crates/nu-protocol/src/engine/state_working_set.rs @@ -1023,16 +1023,20 @@ 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(); - 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") - } + 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") } } diff --git a/crates/nu-protocol/src/eval_const.rs b/crates/nu-protocol/src/eval_const.rs index 87913e4ee3..0b0859c23b 100644 --- a/crates/nu-protocol/src/eval_const.rs +++ 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 });