From 649b428f190d844f8df70c524063fa35d6922017 Mon Sep 17 00:00:00 2001 From: 132ikl <132@ikl.sh> Date: Wed, 26 Feb 2025 22:01:54 -0500 Subject: [PATCH 1/6] Add warning when using $in at the beginning of a block --- crates/nu-parser/src/parser.rs | 39 ++++++++++++++++++- .../src/engine/state_working_set.rs | 15 +++++++ .../nu-protocol/src/errors/parse_warning.rs | 8 ++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 7c2b162c37..8367e4c9a4 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -14,8 +14,8 @@ use log::trace; use nu_engine::DIR_VAR_PARSER_INFO; use nu_protocol::{ ast::*, engine::StateWorkingSet, eval_const::eval_constant, BlockId, DeclId, DidYouMean, - FilesizeUnit, Flag, ParseError, PositionalArg, ShellError, Signature, Span, Spanned, - SyntaxShape, Type, Value, VarId, ENV_VARIABLE_ID, IN_VARIABLE_ID, + FilesizeUnit, Flag, ParseError, ParseWarning, PositionalArg, ShellError, Signature, Span, + Spanned, SyntaxShape, Type, Value, VarId, ENV_VARIABLE_ID, IN_VARIABLE_ID, }; use std::{ collections::{HashMap, HashSet}, @@ -6330,6 +6330,8 @@ pub fn parse_block( .flat_map(|pipeline| pipeline.elements.first()) .any(|element| element.has_in_variable(working_set)) { + check_block_pipes_in(working_set, &block); + // Move the block out to prepare it to become a subexpression let inner_block = std::mem::take(&mut block); block.span = inner_block.span; @@ -6839,6 +6841,39 @@ fn wrap_expr_with_collect(working_set: &mut StateWorkingSet, expr: Expression) - ) } +// Report a warning if the first pipeline element of a block is `$in` followed by a pipe +fn check_block_pipes_in(working_set: &mut StateWorkingSet, block: &Block) { + if let Some(elements) = block.pipelines.first().map(|pipeline| &pipeline.elements) { + // only warn if we're also piping into something + if elements.len() < 2 { + return; + } + let element = match elements.len() { + ..2 => return, + 2.. => elements.first().expect("at least two elements"), + }; + + if let Expr::FullCellPath(cell_path) = &element.expr.expr { + if matches!( + **cell_path, + FullCellPath { + head: Expression { + expr: Expr::Var(id), + .. + }, + .. + } if id == IN_VARIABLE_ID + ) { + working_set + .parse_warnings + .push(ParseWarning::UnnecessaryInVariable { + span: element.expr.span(working_set), + }) + } + } + } +} + // Parses a vector of u8 to create an AST Block. If a file name is given, then // the name is stored in the working set. When parsing a source without a file // name, the source of bytes is stored as "source" diff --git a/crates/nu-protocol/src/engine/state_working_set.rs b/crates/nu-protocol/src/engine/state_working_set.rs index 11be80efb9..61229fdc6d 100644 --- a/crates/nu-protocol/src/engine/state_working_set.rs +++ b/crates/nu-protocol/src/engine/state_working_set.rs @@ -1132,6 +1132,21 @@ impl<'a> GetSpan for &'a StateWorkingSet<'a> { } } +impl GetSpan for StateWorkingSet<'_> { + fn get_span(&self, span_id: SpanId) -> Span { + let num_permanent_spans = self.permanent_state.num_spans(); + if span_id.get() < num_permanent_spans { + self.permanent_state.get_span(span_id) + } else { + *self + .delta + .spans + .get(span_id.get() - num_permanent_spans) + .expect("internal error: missing span") + } + } +} + impl miette::SourceCode for &StateWorkingSet<'_> { fn read_span<'b>( &'b self, diff --git a/crates/nu-protocol/src/errors/parse_warning.rs b/crates/nu-protocol/src/errors/parse_warning.rs index 5f34deb20c..4c36c1f380 100644 --- a/crates/nu-protocol/src/errors/parse_warning.rs +++ b/crates/nu-protocol/src/errors/parse_warning.rs @@ -14,12 +14,20 @@ pub enum ParseWarning { span: Span, url: String, }, + + #[error("Found $in at the start of a command.")] + #[diagnostic(help("Using $in at the start of a command collects the pipeline input.\nIf you did mean to collect the pipeline input, replace this with the `collect` command."))] + UnnecessaryInVariable { + #[label("try removing this")] + span: Span, + }, } impl ParseWarning { pub fn span(&self) -> Span { match self { ParseWarning::DeprecatedWarning { span, .. } => *span, + ParseWarning::UnnecessaryInVariable { span, .. } => *span, } } } From 083c05c3720473c51437b0355322c5bdc324d452 Mon Sep 17 00:00:00 2001 From: 132ikl <132@ikl.sh> Date: Wed, 26 Feb 2025 22:31:29 -0500 Subject: [PATCH 2/6] Fix recursive tests --- crates/nu-command/tests/commands/def.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/nu-command/tests/commands/def.rs b/crates/nu-command/tests/commands/def.rs index 0705c0085f..c31f39c6fb 100644 --- a/crates/nu-command/tests/commands/def.rs +++ b/crates/nu-command/tests/commands/def.rs @@ -329,9 +329,9 @@ fn def_recursive_func_should_work() { def recursive [c: int] { if ($c == 0) { return } if ($c mod 2 > 0) { - $in | recursive ($c - 1) - } else { recursive ($c - 1) + } else { + ignore | recursive ($c - 1) } }"#); assert!(actual.err.is_empty()); @@ -346,9 +346,9 @@ fn export_def_recursive_func_should_work() { export def recursive [c: int] { if ($c == 0) { return } if ($c mod 2 > 0) { - $in | recursive ($c - 1) - } else { recursive ($c - 1) + } else { + ignore | recursive ($c - 1) } }"#); assert!(actual.err.is_empty()); From c292b5f609b314000874fd5a26323d39610ee002 Mon Sep 17 00:00:00 2001 From: 132ikl <132@ikl.sh> Date: Wed, 26 Feb 2025 22:34:48 -0500 Subject: [PATCH 3/6] Revert "Fix recursive tests" This reverts commit 083c05c3720473c51437b0355322c5bdc324d452. --- crates/nu-command/tests/commands/def.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/nu-command/tests/commands/def.rs b/crates/nu-command/tests/commands/def.rs index c31f39c6fb..0705c0085f 100644 --- a/crates/nu-command/tests/commands/def.rs +++ b/crates/nu-command/tests/commands/def.rs @@ -329,9 +329,9 @@ fn def_recursive_func_should_work() { def recursive [c: int] { if ($c == 0) { return } if ($c mod 2 > 0) { - recursive ($c - 1) + $in | recursive ($c - 1) } else { - ignore | recursive ($c - 1) + recursive ($c - 1) } }"#); assert!(actual.err.is_empty()); @@ -346,9 +346,9 @@ fn export_def_recursive_func_should_work() { export def recursive [c: int] { if ($c == 0) { return } if ($c mod 2 > 0) { - recursive ($c - 1) + $in | recursive ($c - 1) } else { - ignore | recursive ($c - 1) + recursive ($c - 1) } }"#); assert!(actual.err.is_empty()); From f5b8ae61cc0bdc0f384b0621872dd24531a57642 Mon Sep 17 00:00:00 2001 From: 132ikl <132@ikl.sh> Date: Wed, 26 Feb 2025 22:39:19 -0500 Subject: [PATCH 4/6] Only report warning for closures, not any block --- crates/nu-cmd-lang/src/parse_const_test.rs | 10 ++++-- crates/nu-parser/src/parse_keywords.rs | 8 +++-- crates/nu-parser/src/parser.rs | 36 ++++++++++++++++++---- 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/crates/nu-cmd-lang/src/parse_const_test.rs b/crates/nu-cmd-lang/src/parse_const_test.rs index 7870b37648..92b2198b24 100644 --- a/crates/nu-cmd-lang/src/parse_const_test.rs +++ b/crates/nu-cmd-lang/src/parse_const_test.rs @@ -11,8 +11,14 @@ fn quickcheck_parse(data: String) -> bool { let mut working_set = StateWorkingSet::new(&context); let _ = working_set.add_file("quickcheck".into(), data.as_bytes()); - let _ = - nu_parser::parse_block(&mut working_set, &tokens, Span::new(0, 0), false, false); + let _ = nu_parser::parse_block( + &mut working_set, + &tokens, + Span::new(0, 0), + false, + false, + false, + ); } } true diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 31dfbb6ea3..9fe06aaa54 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -3339,7 +3339,8 @@ pub fn parse_let(working_set: &mut StateWorkingSet, spans: &[Span]) -> Pipeline } let rvalue_span = Span::concat(&spans[(span.0 + 1)..]); - let rvalue_block = parse_block(working_set, &tokens, rvalue_span, false, true); + let rvalue_block = + parse_block(working_set, &tokens, rvalue_span, false, true, false); let output_type = rvalue_block.output_type(); @@ -3459,7 +3460,7 @@ pub fn parse_const(working_set: &mut StateWorkingSet, spans: &[Span]) -> (Pipeli trace!("parsing: const right-hand side subexpression"); let rvalue_block = - parse_block(working_set, &rvalue_tokens, rvalue_span, false, true); + parse_block(working_set, &rvalue_tokens, rvalue_span, false, true, false); let rvalue_ty = rvalue_block.output_type(); let rvalue_block_id = working_set.add_block(Arc::new(rvalue_block)); let rvalue = Expression::new( @@ -3621,7 +3622,8 @@ pub fn parse_mut(working_set: &mut StateWorkingSet, spans: &[Span]) -> Pipeline } let rvalue_span = Span::concat(&spans[(span.0 + 1)..]); - let rvalue_block = parse_block(working_set, &tokens, rvalue_span, false, true); + let rvalue_block = + parse_block(working_set, &tokens, rvalue_span, false, true, false); let output_type = rvalue_block.output_type(); diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 8367e4c9a4..f1df558de1 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -2452,7 +2452,7 @@ pub fn parse_full_cell_path( // Creating a Type scope to parse the new block. This will keep track of // the previous input type found in that block - let output = parse_block(working_set, &output, span, true, true); + let output = parse_block(working_set, &output, span, true, true, false); let ty = output.output_type(); @@ -4596,7 +4596,14 @@ pub fn parse_block_expression(working_set: &mut StateWorkingSet, span: Span) -> _ => (None, 0), }; - let mut output = parse_block(working_set, &output[amt_to_skip..], span, false, false); + let mut output = parse_block( + working_set, + &output[amt_to_skip..], + span, + false, + false, + false, + ); if let Some(signature) = signature { output.signature = signature.0; @@ -4919,7 +4926,14 @@ pub fn parse_closure_expression( } } - let mut output = parse_block(working_set, &output[amt_to_skip..], span, false, false); + let mut output = parse_block( + working_set, + &output[amt_to_skip..], + span, + false, + false, + true, + ); if let Some(signature) = signature { output.signature = signature.0; @@ -5217,7 +5231,7 @@ pub fn parse_assignment_expression( working_set.parse_errors.extend(rhs_error); trace!("parsing: assignment right-hand side subexpression"); - let rhs_block = parse_block(working_set, &rhs_tokens, rhs_span, false, true); + let rhs_block = parse_block(working_set, &rhs_tokens, rhs_span, false, true, false); let rhs_ty = rhs_block.output_type(); // TEMP: double-check that if the RHS block starts with an external call, it must start with a @@ -6293,6 +6307,7 @@ pub fn parse_block( span: Span, scoped: bool, is_subexpression: bool, + is_closure: bool, ) -> Block { let (lite_block, err) = lite_parse(tokens, working_set); if let Some(err) = err { @@ -6330,7 +6345,9 @@ pub fn parse_block( .flat_map(|pipeline| pipeline.elements.first()) .any(|element| element.has_in_variable(working_set)) { - check_block_pipes_in(working_set, &block); + if is_closure { + check_block_pipes_in(working_set, &block); + } // Move the block out to prepare it to become a subexpression let inner_block = std::mem::take(&mut block); @@ -6912,7 +6929,14 @@ pub fn parse( working_set.error(err) } - Arc::new(parse_block(working_set, &output, new_span, scoped, false)) + Arc::new(parse_block( + working_set, + &output, + new_span, + scoped, + false, + false, + )) } }; From d02431bc1b6d8b91f0737f04b0926e33f4b63c9d Mon Sep 17 00:00:00 2001 From: 132ikl <132@ikl.sh> Date: Wed, 26 Feb 2025 23:04:04 -0500 Subject: [PATCH 5/6] Remove unnecessary $in usages --- crates/nu-command/tests/commands/try_.rs | 2 +- crates/nu-std/std/input/mod.nu | 4 ++-- crates/nu-std/std/xml/mod.nu | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/nu-command/tests/commands/try_.rs b/crates/nu-command/tests/commands/try_.rs index 7e6952d2b9..8e7842a1ab 100644 --- a/crates/nu-command/tests/commands/try_.rs +++ b/crates/nu-command/tests/commands/try_.rs @@ -23,7 +23,7 @@ fn catch_can_access_error() { #[test] fn catch_can_access_error_as_dollar_in() { - let output = nu!("try { foobarbaz } catch { $in | get raw }"); + let output = nu!("try { foobarbaz } catch { get raw }"); assert!(output.err.contains("External command failed")); } diff --git a/crates/nu-std/std/input/mod.nu b/crates/nu-std/std/input/mod.nu index 0a79210c19..d381c865a2 100644 --- a/crates/nu-std/std/input/mod.nu +++ b/crates/nu-std/std/input/mod.nu @@ -4,7 +4,7 @@ def format-event [ ] { # Replace numeric value of raw_code with hex string let record = match $record { {raw_code: $code} => { - $record | update raw_code {|| $in | format number | get upperhex} + $record | update raw_code {|| format number | get upperhex} } _ => $record } @@ -12,7 +12,7 @@ def format-event [ ] { # Replace numeric value of raw_modifiers with binary string let record = match $record { {raw_modifiers: $flags} => { - $record | update raw_modifiers {|| $in | format number | get binary} + $record | update raw_modifiers {|| format number | get binary} } _ => $record } diff --git a/crates/nu-std/std/xml/mod.nu b/crates/nu-std/std/xml/mod.nu index 6cf8159afd..9616236fda 100644 --- a/crates/nu-std/std/xml/mod.nu +++ b/crates/nu-std/std/xml/mod.nu @@ -95,7 +95,7 @@ def xupdate-string-step [ step: string rest: list updater: closure ] { } def xupdate-int-step [ step: int rest: list updater: closure ] { - $in | enumerate | each {|it| + enumerate | each {|it| let item = $it.item let idx = $it.index @@ -108,7 +108,7 @@ def xupdate-int-step [ step: int rest: list updater: closure ] { } def xupdate-closure-step [ step: closure rest: list updater: closure ] { - $in | each {|it| + each {|it| if (do $step $it) { [ $it ] | xupdate-internal $rest $updater | get 0 } else { @@ -194,7 +194,7 @@ export def xinsert [ # position is greater than number of elements) in content of all entries of input matched by # path. If not specified inserts at the end. ] { - $in | xupdate $path {|entry| + xupdate $path {|entry| match ($entry | xtype) { 'tag' => { let new_content = if $position == null { From 0736ea995a533740b997db14c3af6b8d3e29d28d Mon Sep 17 00:00:00 2001 From: 132ikl <132@ikl.sh> Date: Thu, 27 Feb 2025 10:25:43 -0500 Subject: [PATCH 6/6] Update crates/nu-parser/src/parser.rs Co-authored-by: Stefan Holderbach --- crates/nu-parser/src/parser.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index f1df558de1..bc55107b96 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -6862,9 +6862,6 @@ fn wrap_expr_with_collect(working_set: &mut StateWorkingSet, expr: Expression) - fn check_block_pipes_in(working_set: &mut StateWorkingSet, block: &Block) { if let Some(elements) = block.pipelines.first().map(|pipeline| &pipeline.elements) { // only warn if we're also piping into something - if elements.len() < 2 { - return; - } let element = match elements.len() { ..2 => return, 2.. => elements.first().expect("at least two elements"),