nu-cmd-lang cleanup (#12609)

# Description
This PR does miscellaneous cleanup in some of the commands from
`nu-cmd-lang`.

# User-Facing Changes
None.

# After Submitting
Cleanup the other commands in `nu-cmd-lang`.

crates/nu-cmd-lang/src/core_commands/collect.rs

@@ -38,12 +38,12 @@ impl Command for Collect {
     ) -> Result<PipelineData, ShellError> {
         let closure: Closure = call.req(engine_state, stack, 0)?;
 
-        let block = engine_state.get_block(closure.block_id).clone();
+        let block = engine_state.get_block(closure.block_id);
         let mut stack_captures =
             stack.captures_to_stack_preserve_out_dest(closure.captures.clone());
 
         let metadata = input.metadata();
-        let input: Value = input.into_value(call.head);
+        let input = input.into_value(call.head);
 
         let mut saved_positional = None;
         if let Some(var) = block.signature.get_positional(0) {
@@ -58,7 +58,7 @@ impl Command for Collect {
         let result = eval_block(
             engine_state,
             &mut stack_captures,
-            &block,
+            block,
             input.into_pipeline_data(),
         )
         .map(|x| x.set_metadata(metadata));

crates/nu-cmd-lang/src/core_commands/hide_env.rs

@@ -41,11 +41,7 @@ impl Command for HideEnv {
 
         for name in env_var_names {
             if !stack.remove_env_var(engine_state, &name.item) && !ignore_errors {
-                let all_names: Vec<String> = stack
-                    .get_env_var_names(engine_state)
-                    .iter()
-                    .cloned()
-                    .collect();
+                let all_names = stack.get_env_var_names(engine_state);
                 if let Some(closest_match) = did_you_mean(&all_names, &name.item) {
                     return Err(ShellError::DidYouMeanCustom {
                         msg: format!("Environment variable '{}' not found", name.item),

crates/nu-cmd-lang/src/core_commands/if_.rs

@@ -59,17 +59,9 @@ impl Command for If {
             .expect("internal error: missing block");
         let else_case = call.positional_nth(2);
 
-        let result = eval_constant(working_set, cond)?;
-        match &result {
-            Value::Bool { val, .. } => {
-                if *val {
+        if eval_constant(working_set, cond)?.as_bool()? {
             let block = working_set.get_block(then_block);
-                    eval_const_subexpression(
-                        working_set,
-                        block,
-                        input,
-                        block.span.unwrap_or(call.head),
-                    )
+            eval_const_subexpression(working_set, block, input, block.span.unwrap_or(call.head))
         } else if let Some(else_case) = else_case {
             if let Some(else_expr) = else_case.as_keyword() {
                 if let Some(block_id) = else_expr.as_block() {
@@ -90,14 +82,6 @@ impl Command for If {
             Ok(PipelineData::empty())
         }
     }
-            x => Err(ShellError::CantConvert {
-                to_type: "bool".into(),
-                from_type: x.get_type().to_string(),
-                span: result.span(),
-                help: None,
-            }),
-        }
-    }
 
     fn run(
         &self,
@@ -118,10 +102,7 @@ impl Command for If {
         let eval_expression_with_input = get_eval_expression_with_input(engine_state);
         let eval_block = get_eval_block(engine_state);
 
-        let result = eval_expression(engine_state, stack, cond)?;
-        match &result {
-            Value::Bool { val, .. } => {
-                if *val {
+        if eval_expression(engine_state, stack, cond)?.as_bool()? {
             let block = engine_state.get_block(then_block);
             eval_block(engine_state, stack, block, input)
         } else if let Some(else_case) = else_case {
@@ -134,21 +115,12 @@ impl Command for If {
                         .map(|res| res.0)
                 }
             } else {
-                        eval_expression_with_input(engine_state, stack, else_case, input)
-                            .map(|res| res.0)
+                eval_expression_with_input(engine_state, stack, else_case, input).map(|res| res.0)
             }
         } else {
             Ok(PipelineData::empty())
         }
     }
-            x => Err(ShellError::CantConvert {
-                to_type: "bool".into(),
-                from_type: x.get_type().to_string(),
-                span: result.span(),
-                help: None,
-            }),
-        }
-    }
 
     fn examples(&self) -> Vec<Example> {
         vec![

crates/nu-cmd-lang/src/core_commands/let_.rs

@@ -61,7 +61,7 @@ impl Command for Let {
         let eval_block = get_eval_block(engine_state);
         let stack = &mut stack.start_capture();
         let pipeline_data = eval_block(engine_state, stack, block, input)?;
-        let mut value = pipeline_data.into_value(call.head);
+        let value = pipeline_data.into_value(call.head);
 
         // if given variable type is Glob, and our result is string
         // then nushell need to convert from Value::String to Value::Glob
@@ -69,12 +69,12 @@ impl Command for Let {
         // if we pass it to other commands.
         let var_type = &engine_state.get_var(var_id).ty;
         let val_span = value.span();
-        match value {
+        let value = match value {
             Value::String { val, .. } if var_type == &Type::Glob => {
-                value = Value::glob(val, false, val_span);
-            }
-            _ => {}
+                Value::glob(val, false, val_span)
             }
+            value => value,
+        };
 
         stack.add_var(var_id, value);
         Ok(PipelineData::empty())

crates/nu-cmd-lang/src/core_commands/match_.rs

@@ -1,10 +1,7 @@
 use nu_engine::{
     command_prelude::*, get_eval_block, get_eval_expression, get_eval_expression_with_input,
 };
-use nu_protocol::{
-    ast::{Expr, Expression},
-    engine::Matcher,
-};
+use nu_protocol::engine::Matcher;
 
 #[derive(Clone)]
 pub struct Match;
@@ -38,25 +35,25 @@ impl Command for Match {
         input: PipelineData,
     ) -> Result<PipelineData, ShellError> {
         let value: Value = call.req(engine_state, stack, 0)?;
-        let block = call.positional_nth(1);
+        let matches = call
+            .positional_nth(1)
+            .expect("checked through parser")
+            .as_match_block()
+            .expect("missing match block");
+
         let eval_expression = get_eval_expression(engine_state);
         let eval_expression_with_input = get_eval_expression_with_input(engine_state);
         let eval_block = get_eval_block(engine_state);
 
-        if let Some(Expression {
-            expr: Expr::MatchBlock(matches),
-            ..
-        }) = block
-        {
-            for match_ in matches {
         let mut match_variables = vec![];
-                if match_.0.match_value(&value, &mut match_variables) {
+        for (pattern, expr) in matches {
+            if pattern.match_value(&value, &mut match_variables) {
                 // This case does match, go ahead and return the evaluated expression
-                    for match_variable in match_variables {
-                        stack.add_var(match_variable.0, match_variable.1);
+                for (id, value) in match_variables.drain(..) {
+                    stack.add_var(id, value);
                 }
 
-                    let guard_matches = if let Some(guard) = &match_.0.guard {
+                let guard_matches = if let Some(guard) = &pattern.guard {
                     let Value::Bool { val, .. } = eval_expression(engine_state, stack, guard)?
                     else {
                         return Err(ShellError::MatchGuardNotBool { span: guard.span });
@@ -68,15 +65,15 @@ impl Command for Match {
                 };
 
                 if guard_matches {
-                        return if let Some(block_id) = match_.1.as_block() {
+                    return if let Some(block_id) = expr.as_block() {
                         let block = engine_state.get_block(block_id);
                         eval_block(engine_state, stack, block, input)
                     } else {
-                            eval_expression_with_input(engine_state, stack, &match_.1, input)
-                                .map(|x| x.0)
+                        eval_expression_with_input(engine_state, stack, expr, input).map(|x| x.0)
                     };
                 }
-                }
+            } else {
+                match_variables.clear();
             }
         }
 

crates/nu-cmd-lang/src/core_commands/mut_.rs

@@ -61,7 +61,7 @@ impl Command for Mut {
         let eval_block = get_eval_block(engine_state);
         let stack = &mut stack.start_capture();
         let pipeline_data = eval_block(engine_state, stack, block, input)?;
-        let mut value = pipeline_data.into_value(call.head);
+        let value = pipeline_data.into_value(call.head);
 
         // if given variable type is Glob, and our result is string
         // then nushell need to convert from Value::String to Value::Glob
@@ -69,12 +69,12 @@ impl Command for Mut {
         // if we pass it to other commands.
         let var_type = &engine_state.get_var(var_id).ty;
         let val_span = value.span();
-        match value {
+        let value = match value {
             Value::String { val, .. } if var_type == &Type::Glob => {
-                value = Value::glob(val, false, val_span);
-            }
-            _ => {}
+                Value::glob(val, false, val_span)
             }
+            value => value,
+        };
 
         stack.add_var(var_id, value);
         Ok(PipelineData::empty())

crates/nu-cmd-lang/src/core_commands/return_.rs

@@ -40,17 +40,11 @@ impl Command for Return {
         _input: PipelineData,
     ) -> Result<PipelineData, ShellError> {
         let return_value: Option<Value> = call.opt(engine_state, stack, 0)?;
-        if let Some(value) = return_value {
+        let value = return_value.unwrap_or(Value::nothing(call.head));
         Err(ShellError::Return {
             span: call.head,
             value: Box::new(value),
         })
-        } else {
-            Err(ShellError::Return {
-                span: call.head,
-                value: Box::new(Value::nothing(call.head)),
-            })
-        }
     }
 
     fn examples(&self) -> Vec<Example> {

crates/nu-cmd-lang/src/core_commands/try_.rs

@@ -49,9 +49,7 @@ impl Command for Try {
         let try_block = engine_state.get_block(try_block);
         let eval_block = get_eval_block(engine_state);
 
-        let result = eval_block(engine_state, stack, try_block, input);
-
-        match result {
+        match eval_block(engine_state, stack, try_block, input) {
             Err(error) => {
                 let error = intercept_block_control(error)?;
                 let err_record = err_to_record(error, call.head);

crates/nu-cmd-lang/src/core_commands/version.rs

@@ -36,7 +36,7 @@ impl Command for Version {
         call: &Call,
         _input: PipelineData,
     ) -> Result<PipelineData, ShellError> {
-        version(engine_state, call)
+        version(engine_state, call.head)
     }
 
     fn run_const(
@@ -45,7 +45,7 @@ impl Command for Version {
         call: &Call,
         _input: PipelineData,
     ) -> Result<PipelineData, ShellError> {
-        version(working_set.permanent(), call)
+        version(working_set.permanent(), call.head)
     }
 
     fn examples(&self) -> Vec<Example> {
@@ -57,7 +57,13 @@ impl Command for Version {
     }
 }
 
-pub fn version(engine_state: &EngineState, call: &Call) -> Result<PipelineData, ShellError> {
+fn push_non_empty(record: &mut Record, name: &str, value: &str, span: Span) {
+    if !value.is_empty() {
+        record.push(name, Value::string(value, span))
+    }
+}
+
+pub fn version(engine_state: &EngineState, span: Span) -> Result<PipelineData, ShellError> {
     // Pre-allocate the arrays in the worst case (17 items):
     // - version
     // - major
@@ -69,6 +75,7 @@ pub fn version(engine_state: &EngineState, call: &Call) -> Result<PipelineData,
     // - build_os
     // - build_target
     // - rust_version
+    // - rust_channel
     // - cargo_version
     // - build_time
     // - build_rust_channel
@@ -77,68 +84,36 @@ pub fn version(engine_state: &EngineState, call: &Call) -> Result<PipelineData,
     // - installed_plugins
     let mut record = Record::with_capacity(17);
 
-    record.push(
-        "version",
-        Value::string(env!("CARGO_PKG_VERSION"), call.head),
-    );
+    record.push("version", Value::string(env!("CARGO_PKG_VERSION"), span));
 
-    push_version_numbers(&mut record, call.head);
+    push_version_numbers(&mut record, span);
 
-    let version_pre = Some(build::PKG_VERSION_PRE).filter(|x| !x.is_empty());
-    if let Some(version_pre) = version_pre {
-        record.push("pre", Value::string(version_pre, call.head));
+    push_non_empty(&mut record, "pre", build::PKG_VERSION_PRE, span);
+
+    record.push("branch", Value::string(build::BRANCH, span));
+
+    if let Some(commit_hash) = option_env!("NU_COMMIT_HASH") {
+        record.push("commit_hash", Value::string(commit_hash, span));
     }
 
-    record.push("branch", Value::string(build::BRANCH, call.head));
-
-    let commit_hash = option_env!("NU_COMMIT_HASH");
-    if let Some(commit_hash) = commit_hash {
-        record.push("commit_hash", Value::string(commit_hash, call.head));
-    }
-
-    let build_os = Some(build::BUILD_OS).filter(|x| !x.is_empty());
-    if let Some(build_os) = build_os {
-        record.push("build_os", Value::string(build_os, call.head));
-    }
-
-    let build_target = Some(build::BUILD_TARGET).filter(|x| !x.is_empty());
-    if let Some(build_target) = build_target {
-        record.push("build_target", Value::string(build_target, call.head));
-    }
-
-    let rust_version = Some(build::RUST_VERSION).filter(|x| !x.is_empty());
-    if let Some(rust_version) = rust_version {
-        record.push("rust_version", Value::string(rust_version, call.head));
-    }
-
-    let rust_channel = Some(build::RUST_CHANNEL).filter(|x| !x.is_empty());
-    if let Some(rust_channel) = rust_channel {
-        record.push("rust_channel", Value::string(rust_channel, call.head));
-    }
-
-    let cargo_version = Some(build::CARGO_VERSION).filter(|x| !x.is_empty());
-    if let Some(cargo_version) = cargo_version {
-        record.push("cargo_version", Value::string(cargo_version, call.head));
-    }
-
-    let build_time = Some(build::BUILD_TIME).filter(|x| !x.is_empty());
-    if let Some(build_time) = build_time {
-        record.push("build_time", Value::string(build_time, call.head));
-    }
-
-    let build_rust_channel = Some(build::BUILD_RUST_CHANNEL).filter(|x| !x.is_empty());
-    if let Some(build_rust_channel) = build_rust_channel {
-        record.push(
+    push_non_empty(&mut record, "build_os", build::BUILD_OS, span);
+    push_non_empty(&mut record, "build_target", build::BUILD_TARGET, span);
+    push_non_empty(&mut record, "rust_version", build::RUST_VERSION, span);
+    push_non_empty(&mut record, "rust_channel", build::RUST_CHANNEL, span);
+    push_non_empty(&mut record, "cargo_version", build::CARGO_VERSION, span);
+    push_non_empty(&mut record, "build_time", build::BUILD_TIME, span);
+    push_non_empty(
+        &mut record,
         "build_rust_channel",
-            Value::string(build_rust_channel, call.head),
+        build::BUILD_RUST_CHANNEL,
+        span,
     );
-    }
 
-    record.push("allocator", Value::string(global_allocator(), call.head));
+    record.push("allocator", Value::string(global_allocator(), span));
 
     record.push(
         "features",
-        Value::string(features_enabled().join(", "), call.head),
+        Value::string(features_enabled().join(", "), span),
     );
 
     // Get a list of plugin names
@@ -150,10 +125,10 @@ pub fn version(engine_state: &EngineState, call: &Call) -> Result<PipelineData,
 
     record.push(
         "installed_plugins",
-        Value::string(installed_plugins.join(", "), call.head),
+        Value::string(installed_plugins.join(", "), span),
     );
 
-    Ok(Value::record(record, call.head).into_pipeline_data())
+    Ok(Value::record(record, span).into_pipeline_data())
 }
 
 /// Add version numbers as integers to the given record
@@ -167,9 +142,9 @@ fn push_version_numbers(record: &mut Record, head: Span) {
             build::PKG_VERSION_PATCH.parse().expect("Always set"),
         )
     });
-    record.push("major", Value::int(major as _, head));
-    record.push("minor", Value::int(minor as _, head));
-    record.push("patch", Value::int(patch as _, head));
+    record.push("major", Value::int(major.into(), head));
+    record.push("minor", Value::int(minor.into(), head));
+    record.push("patch", Value::int(patch.into(), head));
 }
 
 fn global_allocator() -> &'static str {

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

@@ -1,5 +1,5 @@
 use crate::{
-    ast::{Argument, Block, Expr, ExternalArgument, ImportPattern, RecordItem},
+    ast::{Argument, Block, Expr, ExternalArgument, ImportPattern, MatchPattern, RecordItem},
     engine::StateWorkingSet,
     BlockId, DeclId, Signature, Span, Type, VarId, IN_VARIABLE_ID,
 };
@@ -79,6 +79,13 @@ impl Expression {
         }
     }
 
+    pub fn as_match_block(&self) -> Option<&[(MatchPattern, Expression)]> {
+        match &self.expr {
+            Expr::MatchBlock(matches) => Some(matches),
+            _ => None,
+        }
+    }
+
     pub fn as_signature(&self) -> Option<Box<Signature>> {
         match &self.expr {
             Expr::Signature(sig) => Some(sig.clone()),