Address feedback from PR #12229 (#12242)

# Description
@sholderbach left a very helpful review and this just implements the
suggestions he made.

Didn't notice any difference in performance, but there could potentially
be for a long running Nushell session or one that loads a lot of stuff.

I also caught a bug where nu-protocol won't build without `plugin`
because of the previous conditional import. Oops. Fixed.

# User-Facing Changes
`blocks` and `modules` type in `EngineState` changed again. Shouldn't
affect plugins or anything else though really

# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

---------

Co-authored-by: sholderbach <sholderbach@users.noreply.github.com>

crates/nu-cli/src/completions/completer.rs

@@ -124,7 +124,7 @@ impl NuCompleter {
 
         let output = parse(&mut working_set, Some("completer"), line.as_bytes(), false);
 
-        for pipeline in output.pipelines.iter() {
+        for pipeline in &output.pipelines {
             for pipeline_element in &pipeline.elements {
                 let flattened = flatten_pipeline_element(&working_set, pipeline_element);
                 let mut spans: Vec<String> = vec![];

crates/nu-explore/src/nu_common/command.rs

@@ -91,6 +91,8 @@ fn eval_source2(
     // So we LITERALLY ignore all expressions except the LAST.
     if block.len() > 1 {
         let range = ..block.pipelines.len() - 1;
+        // Note: `make_mut` will mutate `&mut block: &mut Arc<Block>`
+        // for the outer fn scope `eval_block`
         Arc::make_mut(&mut block).pipelines.drain(range);
     }
 

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

@@ -2,7 +2,7 @@ use std::sync::Arc;
 
 use serde::{Deserialize, Serialize};
 
-use super::{Argument, Expr, ExternalArgument, RecordItem};
+use super::{Argument, Block, Expr, ExternalArgument, RecordItem};
 use crate::ast::ImportPattern;
 use crate::DeclId;
 use crate::{engine::StateWorkingSet, BlockId, Signature, Span, Type, VarId, IN_VARIABLE_ID};
@@ -327,7 +327,8 @@ impl Expression {
                 expr.replace_span(working_set, replaced, new_span);
             }
             Expr::Block(block_id) => {
-                let mut block = (**working_set.get_block(*block_id)).clone();
+                // We are cloning the Block itself, rather than the Arc around it.
+                let mut block = Block::clone(working_set.get_block(*block_id));
 
                 for pipeline in block.pipelines.iter_mut() {
                     for element in pipeline.elements.iter_mut() {

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

@@ -79,8 +79,11 @@ pub struct EngineState {
     pub(super) virtual_paths: Vec<(String, VirtualPath)>,
     vars: Vec<Variable>,
     decls: Arc<Vec<Box<dyn Command + 'static>>>,
-    pub(super) blocks: Vec<Arc<Block>>,
-    pub(super) modules: Vec<Arc<Module>>,
+    // The Vec is wrapped in Arc so that if we don't need to modify the list, we can just clone
+    // the reference and not have to clone each individual Arc inside. These lists can be
+    // especially long, so it helps
+    pub(super) blocks: Arc<Vec<Arc<Block>>>,
+    pub(super) modules: Arc<Vec<Arc<Module>>>,
     usage: Usage,
     pub scope: ScopeFrame,
     pub ctrlc: Option<Arc<AtomicBool>>,
@@ -128,10 +131,10 @@ impl EngineState {
                 Variable::new(Span::new(0, 0), Type::Any, false),
             ],
             decls: Arc::new(vec![]),
-            blocks: vec![],
-            modules: vec![Arc::new(Module::new(
+            blocks: Arc::new(vec![]),
+            modules: Arc::new(vec![Arc::new(Module::new(
                 DEFAULT_OVERLAY_NAME.as_bytes().to_vec(),
-            ))],
+            ))]),
             usage: Usage::new(),
             // make sure we have some default overlay:
             scope: ScopeFrame::with_empty_overlay(
@@ -184,14 +187,18 @@ impl EngineState {
         self.files.extend(delta.files);
         self.virtual_paths.extend(delta.virtual_paths);
         self.vars.extend(delta.vars);
-        self.blocks.extend(delta.blocks);
-        self.modules.extend(delta.modules);
         self.usage.merge_with(delta.usage);
 
-        // Avoid potentially cloning the Arc if we aren't adding anything
+        // Avoid potentially cloning the Arcs if we aren't adding anything
         if !delta.decls.is_empty() {
             Arc::make_mut(&mut self.decls).extend(delta.decls);
         }
+        if !delta.blocks.is_empty() {
+            Arc::make_mut(&mut self.blocks).extend(delta.blocks);
+        }
+        if !delta.modules.is_empty() {
+            Arc::make_mut(&mut self.modules).extend(delta.modules);
+        }
 
         let first = delta.scope.remove(0);
 

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

@@ -3,7 +3,6 @@ use super::{usage::Usage, Command, EngineState, OverlayFrame, ScopeFrame, Variab
 use crate::ast::Block;
 use crate::Module;
 
-#[cfg(feature = "plugin")]
 use std::sync::Arc;
 
 #[cfg(feature = "plugin")]

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

@@ -10,7 +10,6 @@ use core::panic;
 use std::collections::{HashMap, HashSet};
 use std::path::PathBuf;
 
-#[cfg(feature = "plugin")]
 use std::sync::Arc;
 
 #[cfg(feature = "plugin")]
@@ -960,7 +959,7 @@ impl<'a> StateWorkingSet<'a> {
             }
         }
 
-        for block in &self.permanent_state.blocks {
+        for block in self.permanent_state.blocks.iter() {
             if Some(span) == block.span {
                 return Some(block.clone());
             }