Make Record.cols private (#12317)

# Description
Makes the `cols` field in `Record` private and fixes the implementation
of `rename` to account for this change.

crates/nu-command/src/filters/rename.rs

@@ -1,7 +1,6 @@
 use indexmap::IndexMap;
 use nu_engine::{command_prelude::*, get_eval_block_with_early_return};
 use nu_protocol::engine::Closure;
-use std::collections::HashSet;
 
 #[derive(Clone)]
 pub struct Rename;
@@ -157,72 +156,85 @@ fn rename(
             move |item| {
                 let span = item.span();
                 match item {
-                    Value::Record {
+                    Value::Record { val: record, .. } => {
-                        val: mut record, ..
+                        let record =
-                    } => {
                             if let Some((engine_state, block, mut stack, env_vars, env_hidden)) =
                                 block_info.clone()
                             {
-                            for c in &mut record.cols {
+                                record
+                                    .into_iter()
+                                    .map(|(col, val)| {
                                         stack.with_env(&env_vars, &env_hidden);
 
                                         if let Some(var) = block.signature.get_positional(0) {
                                             if let Some(var_id) = &var.var_id {
-                                        stack.add_var(*var_id, Value::string(c.clone(), span))
+                                                stack.add_var(
+                                                    *var_id,
+                                                    Value::string(col.clone(), span),
+                                                )
                                             }
                                         }
 
-                                let eval_result = eval_block_with_early_return(
+                                        eval_block_with_early_return(
                                             &engine_state,
                                             &mut stack,
                                             &block,
-                                    Value::string(c.clone(), span).into_pipeline_data(),
+                                            Value::string(col, span).into_pipeline_data(),
-                                );
+                                        )
-
+                                        .and_then(|data| data.collect_string_strict(span))
-                                match eval_result {
+                                        .map(|(col, _, _)| (col, val))
-                                    Err(e) => return Value::error(e, span),
+                                    })
-                                    Ok(res) => match res.collect_string_strict(span) {
+                                    .collect::<Result<Record, _>>()
-                                        Err(e) => return Value::error(e, span),
-                                        Ok(new_c) => *c = new_c.0,
-                                    },
-                                }
-                            }
                             } else {
                                 match &specified_column {
-                                Some(c) => {
+                                    Some(columns) => {
-                                    let mut column_to_rename: HashSet<String> = HashSet::from_iter(c.keys().cloned());
+                                        // record columns are unique so we can track the number
-                                    for val in record.cols.iter_mut() {
+                                        // of renamed columns to check if any were missed
-                                        if c.contains_key(val) {
+                                        let mut renamed = 0;
-                                            column_to_rename.remove(val);
+                                        let record = record.into_iter().map(|(col, val)| {
-                                            *val = c.get(val).expect("already check exists").to_owned();
+                                            let col = if let Some(col) = columns.get(&col) {
-                                        }
+                                                renamed += 1;
-                                    }
+                                                col.clone()
-                                    if !column_to_rename.is_empty() {
+                                            } else {
-                                        let not_exists_column =
+                                                col
-                                            column_to_rename.into_iter().next().expect(
+                                            };
-                                                "already checked column to rename still exists",
-                                            );
-                                        return Value::error(
-                                            ShellError::UnsupportedInput { msg: format!(
-                                                    "The column '{not_exists_column}' does not exist in the input",
-                                                ), input: "value originated from here".into(), msg_span: head_span, input_span: span },
-                                            span,
-                                        );
-                                    }
-                                }
-                                None => {
-                                    for (idx, val) in columns.iter().enumerate() {
-                                        if idx >= record.len() {
-                                            // skip extra new columns names if we already reached the final column
-                                            break;
-                                        }
-                                        record.cols[idx] = val.clone();
-                                    }
-                                }
-                            }
-                        }
 
-                        Value::record(*record, span)
+                                            (col, val)
+                                        }).collect::<Record>();
+
+                                        let missing_column = if renamed < columns.len() {
+                                            columns.iter().find_map(|(col, new_col)| {
+                                                (!record.contains(new_col)).then_some(col)
+                                            })
+                                        } else {
+                                            None
+                                        };
+
+                                        if let Some(missing) = missing_column {
+                                            Err(ShellError::UnsupportedInput {
+                                                msg: format!("The column '{missing}' does not exist in the input"),
+                                                input: "value originated from here".into(),
+                                                msg_span: head_span,
+                                                input_span: span,
+                                            })
+                                        } else {
+                                            Ok(record)
+                                        }
+                                    }
+                                    None => Ok(record
+                                        .into_iter()
+                                        .enumerate()
+                                        .map(|(i, (col, val))| {
+                                            (columns.get(i).cloned().unwrap_or(col), val)
+                                        })
+                                        .collect()),
+                                }
+                            };
+
+                        match record {
+                            Ok(record) => Value::record(record, span),
+                            Err(err) => Value::error(err, span),
+                        }
                     }
                     // Propagate errors by explicitly matching them before the final case.
                     Value::Error { .. } => item.clone(),

crates/nu-protocol/src/value/record.rs

@@ -6,11 +6,7 @@ use serde::{Deserialize, Serialize};
 
 #[derive(Debug, Clone, Default, Serialize, Deserialize)]
 pub struct Record {
-    /// Don't use this field publicly!
+    cols: Vec<String>,
-    ///
-    /// Only public as command `rename` is not reimplemented in a sane way yet
-    /// Using it or making `vals` public will draw shaming by @sholderbach
-    pub cols: Vec<String>,
     vals: Vec<Value>,
 }