From 20f6114617ed958a22164ade968e4153f8060af9 Mon Sep 17 00:00:00 2001
From: JT <jonathandturner@users.noreply.github.com>
Date: Thu, 20 May 2021 16:26:54 +1200
Subject: [PATCH] Improve block params (#3450)

---
 .../nu-command/src/commands/each/command.rs   |  8 ++++-
 crates/nu-engine/src/evaluate/evaluator.rs    |  9 -----
 crates/nu-parser/src/parse.rs                 | 15 ++++++--
 tests/shell/pipeline/commands/internal.rs     | 34 +++++++++++++++----
 4 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/crates/nu-command/src/commands/each/command.rs b/crates/nu-command/src/commands/each/command.rs
index 8ab4ba491..9f6eefed1 100644
--- a/crates/nu-command/src/commands/each/command.rs
+++ b/crates/nu-command/src/commands/each/command.rs
@@ -89,7 +89,13 @@ pub fn process_row(
     context.scope.add_vars(&captured_block.captured.entries);
 
     if !captured_block.block.params.positional.is_empty() {
-        // FIXME: add check for more than parameter, once that's supported
+        if captured_block.block.params.positional.len() > 1 {
+            return Err(ShellError::labeled_error(
+                "Expected block with less than two parameters",
+                "too many parameters",
+                captured_block.block.span,
+            ));
+        }
         context
             .scope
             .add_var(captured_block.block.params.positional[0].0.name(), input);
diff --git a/crates/nu-engine/src/evaluate/evaluator.rs b/crates/nu-engine/src/evaluate/evaluator.rs
index dca73e50e..5533d47cc 100644
--- a/crates/nu-engine/src/evaluate/evaluator.rs
+++ b/crates/nu-engine/src/evaluate/evaluator.rs
@@ -253,15 +253,6 @@ fn evaluate_reference(
             tag: span.into(),
         }),
 
-        "$it" => match ctx.scope.get_var("$it") {
-            Some(v) => Ok(v),
-            None => Err(ShellError::labeled_error(
-                "Variable not in scope",
-                "missing '$it' (note: $it is only available inside of a block)",
-                span,
-            )),
-        },
-
         "$nothing" => Ok(Value {
             value: UntaggedValue::nothing(),
             tag: span.into(),
diff --git a/crates/nu-parser/src/parse.rs b/crates/nu-parser/src/parse.rs
index 95b6a95f3..9f8cb1799 100644
--- a/crates/nu-parser/src/parse.rs
+++ b/crates/nu-parser/src/parse.rs
@@ -972,6 +972,15 @@ fn parse_arg(
                         }
 
                         tokens = token_iter.collect();
+                        if tokens.is_empty() {
+                            return (
+                                garbage(lite_arg.span),
+                                Some(ParseError::mismatch(
+                                    "block with parameters",
+                                    lite_arg.clone(),
+                                )),
+                            );
+                        }
                         params
                     } else {
                         vec![]
@@ -986,8 +995,10 @@ fn parse_arg(
                     let (mut classified_block, err) = classify_block(&lite_block, scope);
                     scope.exit_scope();
 
-                    if !params.is_empty() && classified_block.params.positional.is_empty() {
-                        if let Some(classified_block) = Arc::get_mut(&mut classified_block) {
+                    if let Some(classified_block) = Arc::get_mut(&mut classified_block) {
+                        classified_block.span = lite_arg.span;
+                        if !params.is_empty() {
+                            classified_block.params.positional.clear();
                             for param in params {
                                 classified_block
                                     .params
diff --git a/tests/shell/pipeline/commands/internal.rs b/tests/shell/pipeline/commands/internal.rs
index b86d5d924..047358c5c 100644
--- a/tests/shell/pipeline/commands/internal.rs
+++ b/tests/shell/pipeline/commands/internal.rs
@@ -295,7 +295,7 @@ fn run_custom_command_with_empty_rest() {
 }
 
 #[test]
-fn set_variable() {
+fn let_variable() {
     let actual = nu!(
         cwd: ".",
         r#"
@@ -309,7 +309,7 @@ fn set_variable() {
 }
 
 #[test]
-fn set_doesnt_leak() {
+fn let_doesnt_leak() {
     let actual = nu!(
         cwd: ".",
         r#"
@@ -321,7 +321,7 @@ fn set_doesnt_leak() {
 }
 
 #[test]
-fn set_env_variable() {
+fn let_env_variable() {
     let actual = nu!(
         cwd: ".",
         r#"
@@ -334,7 +334,7 @@ fn set_env_variable() {
 }
 
 #[test]
-fn set_env_doesnt_leak() {
+fn let_env_doesnt_leak() {
     let actual = nu!(
         cwd: ".",
         r#"
@@ -346,7 +346,7 @@ fn set_env_doesnt_leak() {
 }
 
 #[test]
-fn proper_shadow_set_env_aliases() {
+fn proper_shadow_let_env_aliases() {
     let actual = nu!(
         cwd: ".",
         r#"
@@ -357,7 +357,7 @@ fn proper_shadow_set_env_aliases() {
 }
 
 #[test]
-fn proper_shadow_set_aliases() {
+fn proper_shadow_let_aliases() {
     let actual = nu!(
         cwd: ".",
         r#"
@@ -367,6 +367,28 @@ fn proper_shadow_set_aliases() {
     assert_eq!(actual.out, "falsetruefalse");
 }
 
+#[test]
+fn block_param_too_many() {
+    let actual = nu!(
+        cwd: ".",
+        r#"
+        [1, 2, 3] | each { |a, b| echo $a }
+        "#
+    );
+    assert!(actual.err.contains("too many"));
+}
+
+#[test]
+fn block_params_override() {
+    let actual = nu!(
+        cwd: ".",
+        r#"
+        [1, 2, 3] | each { |a| echo $it }
+        "#
+    );
+    assert!(actual.err.contains("unknown variable"));
+}
+
 #[test]
 fn run_dynamic_blocks() {
     let actual = nu!(