From fb4251aba754195eae1000763a0c221988ffa3de Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Sun, 18 Feb 2024 12:20:22 +0000 Subject: [PATCH] Remove `Record::from_raw_cols_vals_unchecked` (#11810) # Description Follows from #11718 and replaces all usages of `Record::from_raw_cols_vals_unchecked` with iterator or `record!` equivalents. --- .../src/dataframe/eager/cast.rs | 24 +-- .../src/dataframe/eager/schema.rs | 22 +-- .../values/nu_dataframe/conversion.rs | 35 ++-- .../src/dataframe/values/nu_dataframe/mod.rs | 14 +- .../src/dataframe/values/nu_schema.rs | 52 ++---- .../src/extra/filters/roll/mod.rs | 8 +- crates/nu-command/src/charting/histogram.rs | 26 +-- .../nu-command/src/database/values/sqlite.rs | 29 +-- .../nu-command/src/formats/from/delimited.rs | 42 ++--- crates/nu-command/src/formats/from/nuon.rs | 15 +- .../nu-command/src/strings/detect_columns.rs | 36 ++-- .../tests/commands/database/into_sqlite.rs | 28 ++- crates/nu-engine/src/scope.rs | 175 ++++++++---------- crates/nu-explore/src/commands/help.rs | 7 +- crates/nu-explore/src/views/record/mod.rs | 26 +-- crates/nu-protocol/src/eval_base.rs | 11 +- crates/nu-protocol/src/value/record.rs | 56 +++--- crates/nu_plugin_example/src/example.rs | 20 +- 18 files changed, 267 insertions(+), 359 deletions(-) diff --git a/crates/nu-cmd-dataframe/src/dataframe/eager/cast.rs b/crates/nu-cmd-dataframe/src/dataframe/eager/cast.rs index 4a8133bf38..1abf6f62a8 100644 --- a/crates/nu-cmd-dataframe/src/dataframe/eager/cast.rs +++ b/crates/nu-cmd-dataframe/src/dataframe/eager/cast.rs @@ -5,7 +5,7 @@ use nu_engine::CallExt; use nu_protocol::{ ast::Call, engine::{Command, EngineState, Stack}, - Category, Example, PipelineData, Record, ShellError, Signature, Span, SyntaxShape, Type, Value, + record, Category, Example, PipelineData, ShellError, Signature, Span, SyntaxShape, Type, Value, }; use polars::prelude::*; @@ -52,13 +52,10 @@ impl Command for CastDF { description: "Cast a column in a dataframe to a different dtype", example: "[[a b]; [1 2] [3 4]] | dfr into-df | dfr cast u8 a | dfr schema", result: Some(Value::record( - Record::from_raw_cols_vals_unchecked( - vec!["a".to_string(), "b".to_string()], - vec![ - Value::string("u8", Span::test_data()), - Value::string("i64", Span::test_data()), - ], - ), + record! { + "a" => Value::string("u8", Span::test_data()), + "b" => Value::string("i64", Span::test_data()), + }, Span::test_data(), )), }, @@ -66,13 +63,10 @@ impl Command for CastDF { description: "Cast a column in a lazy dataframe to a different dtype", example: "[[a b]; [1 2] [3 4]] | dfr into-df | dfr into-lazy | dfr cast u8 a | dfr schema", result: Some(Value::record( - Record::from_raw_cols_vals_unchecked( - vec!["a".to_string(), "b".to_string()], - vec![ - Value::string("u8", Span::test_data()), - Value::string("i64", Span::test_data()), - ], - ), + record! { + "a" => Value::string("u8", Span::test_data()), + "b" => Value::string("i64", Span::test_data()), + }, Span::test_data(), )), }, diff --git a/crates/nu-cmd-dataframe/src/dataframe/eager/schema.rs b/crates/nu-cmd-dataframe/src/dataframe/eager/schema.rs index 349a374e2c..7028d15e7c 100644 --- a/crates/nu-cmd-dataframe/src/dataframe/eager/schema.rs +++ b/crates/nu-cmd-dataframe/src/dataframe/eager/schema.rs @@ -3,7 +3,7 @@ use nu_engine::CallExt; use nu_protocol::{ ast::Call, engine::{Command, EngineState, Stack}, - Category, Example, PipelineData, Record, ShellError, Signature, Span, Type, Value, + record, Category, Example, PipelineData, ShellError, Signature, Span, Type, Value, }; #[derive(Clone)] @@ -33,13 +33,10 @@ impl Command for SchemaDF { description: "Dataframe schema", example: r#"[[a b]; [1 "foo"] [3 "bar"]] | dfr into-df | dfr schema"#, result: Some(Value::record( - Record::from_raw_cols_vals_unchecked( - vec!["a".to_string(), "b".to_string()], - vec![ - Value::string("i64", Span::test_data()), - Value::string("str", Span::test_data()), - ], - ), + record! { + "a" => Value::string("i64", Span::test_data()), + "b" => Value::string("str", Span::test_data()), + }, Span::test_data(), )), }] @@ -98,10 +95,11 @@ fn datatype_list(span: Span) -> Value { ] .iter() .map(|(dtype, note)| { - Value::record(Record::from_raw_cols_vals_unchecked( - vec!["dtype".to_string(), "note".to_string()], - vec![Value::string(*dtype, span), Value::string(*note, span)], - ),span) + Value::record(record! { + "dtype" => Value::string(*dtype, span), + "note" => Value::string(*note, span), + }, + span) }) .collect(); Value::list(types, span) diff --git a/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/conversion.rs b/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/conversion.rs index bc70db8d60..22f77fc412 100644 --- a/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/conversion.rs +++ b/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/conversion.rs @@ -1033,15 +1033,14 @@ fn series_to_values( Either::Right(it) } .map(|any_values| { - let vals: Result, ShellError> = any_values + let record = polar_fields .iter() - .map(|v| any_value_to_value(v, span)) - .collect(); - let cols: Vec = polar_fields - .iter() - .map(|field| field.name.to_string()) - .collect(); - let record = Record::from_raw_cols_vals_unchecked(cols, vals?); + .zip(any_values) + .map(|(field, val)| { + any_value_to_value(val, span).map(|val| (field.name.to_string(), val)) + }) + .collect::>()?; + Ok(Value::record(record, span)) }) .collect(); @@ -1138,20 +1137,16 @@ fn any_value_to_value(any_value: &AnyValue, span: Span) -> Result { - let values: Result, ShellError> = struct_tuple - .0 - .iter() - .map(|s| any_value_to_value(s, span)) - .collect(); - let fields = struct_tuple + let record = struct_tuple .1 .iter() - .map(|f| f.name().to_string()) - .collect(); - Ok(Value::Record { - val: Record::from_raw_cols_vals_unchecked(fields, values?), - internal_span: span, - }) + .zip(&struct_tuple.0) + .map(|(field, val)| { + any_value_to_value(val, span).map(|val| (field.name.to_string(), val)) + }) + .collect::>()?; + + Ok(Value::record(record, span)) } AnyValue::StringOwned(s) => Ok(Value::string(s.to_string(), span)), AnyValue::Binary(bytes) => Ok(Value::binary(*bytes, span)), diff --git a/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/mod.rs b/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/mod.rs index c389f0765b..9a3f0f6eb5 100644 --- a/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/mod.rs +++ b/crates/nu-cmd-dataframe/src/dataframe/values/nu_dataframe/mod.rs @@ -154,15 +154,13 @@ impl NuDataFrame { match value { Value::CustomValue { .. } => return Self::try_from_value(value), Value::List { vals, .. } => { - let cols = (0..vals.len()) - .map(|i| format!("{i}")) - .collect::>(); + let record = vals + .into_iter() + .enumerate() + .map(|(i, val)| (format!("{i}"), val)) + .collect(); - conversion::insert_record( - &mut column_values, - Record::from_raw_cols_vals_unchecked(cols, vals), - &maybe_schema, - )? + conversion::insert_record(&mut column_values, record, &maybe_schema)? } Value::Record { val: record, .. } => { conversion::insert_record(&mut column_values, record, &maybe_schema)? diff --git a/crates/nu-cmd-dataframe/src/dataframe/values/nu_schema.rs b/crates/nu-cmd-dataframe/src/dataframe/values/nu_schema.rs index ed02acdfa8..662bfae842 100644 --- a/crates/nu-cmd-dataframe/src/dataframe/values/nu_schema.rs +++ b/crates/nu-cmd-dataframe/src/dataframe/values/nu_schema.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use nu_protocol::{Record, ShellError, Span, Value}; +use nu_protocol::{ShellError, Span, Value}; use polars::prelude::{DataType, Field, Schema, SchemaRef, TimeUnit}; #[derive(Debug, Clone)] @@ -37,15 +37,14 @@ impl From for SchemaRef { } fn fields_to_value(fields: impl Iterator, span: Span) -> Value { - let (cols, vals) = fields + let record = fields .map(|field| { - let val = dtype_to_value(field.data_type(), span); let col = field.name().to_string(); + let val = dtype_to_value(field.data_type(), span); (col, val) }) - .unzip(); + .collect(); - let record = Record::from_raw_cols_vals_unchecked(cols, vals); Value::record(record, Span::unknown()) } @@ -188,42 +187,23 @@ fn str_to_time_unit(ts_string: &str, span: Span) -> Result #[cfg(test)] mod test { + use nu_protocol::record; + use super::*; #[test] fn test_value_to_schema() { - let value = Value::Record { - val: Record::from_raw_cols_vals_unchecked( - vec!["name".into(), "age".into(), "address".into()], - vec![ - Value::String { - val: "str".into(), - internal_span: Span::test_data(), - }, - Value::String { - val: "i32".into(), - internal_span: Span::test_data(), - }, - Value::Record { - val: Record::from_raw_cols_vals_unchecked( - vec!["street".into(), "city".into()], - vec![ - Value::String { - val: "str".into(), - internal_span: Span::test_data(), - }, - Value::String { - val: "str".into(), - internal_span: Span::test_data(), - }, - ], - ), - internal_span: Span::test_data(), - }, - ], - ), - internal_span: Span::test_data(), + let address = record! { + "street" => Value::test_string("str"), + "city" => Value::test_string("str"), }; + + let value = Value::test_record(record! { + "name" => Value::test_string("str"), + "age" => Value::test_string("i32"), + "address" => Value::test_record(address) + }); + let schema = value_to_schema(&value, Span::unknown()).unwrap(); let expected = Schema::from_iter(vec![ Field::new("name", DataType::String), diff --git a/crates/nu-cmd-extra/src/extra/filters/roll/mod.rs b/crates/nu-cmd-extra/src/extra/filters/roll/mod.rs index 54779238d0..8fb5567558 100644 --- a/crates/nu-cmd-extra/src/extra/filters/roll/mod.rs +++ b/crates/nu-cmd-extra/src/extra/filters/roll/mod.rs @@ -4,7 +4,7 @@ mod roll_left; mod roll_right; mod roll_up; -use nu_protocol::{Record, ShellError, Value}; +use nu_protocol::{ShellError, Value}; pub use roll_::Roll; pub use roll_down::RollDown; pub use roll_left::RollLeft; @@ -70,10 +70,8 @@ fn horizontal_rotate_value( HorizontalDirection::Left => vals.rotate_left(rotations), } - Ok(Value::record( - Record::from_raw_cols_vals_unchecked(cols, vals), - span, - )) + let record = cols.into_iter().zip(vals).collect(); + Ok(Value::record(record, span)) } Value::List { vals, .. } => { let values = vals diff --git a/crates/nu-command/src/charting/histogram.rs b/crates/nu-command/src/charting/histogram.rs index c756af86f2..9c0b9334b6 100755 --- a/crates/nu-command/src/charting/histogram.rs +++ b/crates/nu-command/src/charting/histogram.rs @@ -4,7 +4,7 @@ use nu_engine::CallExt; use nu_protocol::ast::Call; use nu_protocol::engine::{Command, EngineState, Stack}; use nu_protocol::{ - record, Category, Example, IntoPipelineData, PipelineData, Record, ShellError, Signature, Span, + record, Category, Example, IntoPipelineData, PipelineData, ShellError, Signature, Span, Spanned, SyntaxShape, Type, Value, }; use std::collections::HashMap; @@ -239,13 +239,6 @@ fn histogram_impl( } let mut result = vec![]; - let result_cols = vec![ - value_column_name.to_string(), - "count".to_string(), - "quantile".to_string(), - "percentage".to_string(), - freq_column.to_string(), - ]; const MAX_FREQ_COUNT: f64 = 100.0; for (val, count) in counter.into_iter().sorted() { let quantile = match calc_method { @@ -259,16 +252,13 @@ fn histogram_impl( result.push(( count, // attach count first for easily sorting. Value::record( - Record::from_raw_cols_vals_unchecked( - result_cols.clone(), - vec![ - val.into_value(), - Value::int(count, span), - Value::float(quantile, span), - Value::string(percentage, span), - Value::string(freq, span), - ], - ), + record! { + value_column_name => val.into_value(), + "count" => Value::int(count, span), + "quantile" => Value::float(quantile, span), + "percentage" => Value::string(percentage, span), + freq_column => Value::string(freq, span), + }, span, ), )); diff --git a/crates/nu-command/src/database/values/sqlite.rs b/crates/nu-command/src/database/values/sqlite.rs index d86deaa3e2..e1c18e97be 100644 --- a/crates/nu-command/src/database/values/sqlite.rs +++ b/crates/nu-command/src/database/values/sqlite.rs @@ -441,15 +441,15 @@ fn prepared_statement_to_nu_list( ) -> Result { let column_names = stmt .column_names() - .iter() - .map(|c| c.to_string()) + .into_iter() + .map(String::from) .collect::>(); let row_results = stmt.query_map([], |row| { Ok(convert_sqlite_row_to_nu_value( row, call_span, - column_names.clone(), + &column_names, )) })?; @@ -491,18 +491,19 @@ fn read_entire_sqlite_db( Ok(Value::record(tables, call_span)) } -pub fn convert_sqlite_row_to_nu_value(row: &Row, span: Span, column_names: Vec) -> Value { - let mut vals = Vec::with_capacity(column_names.len()); +pub fn convert_sqlite_row_to_nu_value(row: &Row, span: Span, column_names: &[String]) -> Value { + let record = column_names + .iter() + .enumerate() + .map(|(i, col)| { + ( + col.clone(), + convert_sqlite_value_to_nu_value(row.get_ref_unwrap(i), span), + ) + }) + .collect(); - for i in 0..column_names.len() { - let val = convert_sqlite_value_to_nu_value(row.get_ref_unwrap(i), span); - vals.push(val); - } - - Value::record( - Record::from_raw_cols_vals_unchecked(column_names, vals), - span, - ) + Value::record(record, span) } pub fn convert_sqlite_value_to_nu_value(value: ValueRef, span: Span) -> Value { diff --git a/crates/nu-command/src/formats/from/delimited.rs b/crates/nu-command/src/formats/from/delimited.rs index 26b3c09892..1fdea19482 100644 --- a/crates/nu-command/src/formats/from/delimited.rs +++ b/crates/nu-command/src/formats/from/delimited.rs @@ -1,5 +1,5 @@ use csv::{ReaderBuilder, Trim}; -use nu_protocol::{IntoPipelineData, PipelineData, Record, ShellError, Span, Value}; +use nu_protocol::{IntoPipelineData, PipelineData, ShellError, Span, Value}; fn from_delimited_string_to_value( DelimitedReaderConfig { @@ -36,28 +36,28 @@ fn from_delimited_string_to_value( let mut rows = vec![]; for row in reader.records() { let row = row?; - let output_row = (0..headers.len()) - .map(|i| { - row.get(i) - .map(|value| { - if no_infer { - Value::string(value.to_string(), span) - } else if let Ok(i) = value.parse::() { - Value::int(i, span) - } else if let Ok(f) = value.parse::() { - Value::float(f, span) - } else { - Value::string(value.to_string(), span) - } - }) - .unwrap_or(Value::nothing(span)) + let columns = headers.iter().cloned(); + let values = row + .into_iter() + .map(|s| { + if no_infer { + Value::string(s, span) + } else if let Ok(i) = s.parse() { + Value::int(i, span) + } else if let Ok(f) = s.parse() { + Value::float(f, span) + } else { + Value::string(s, span) + } }) - .collect::>(); + .chain(std::iter::repeat(Value::nothing(span))); - rows.push(Value::record( - Record::from_raw_cols_vals_unchecked(headers.clone(), output_row), - span, - )); + // If there are more values than the number of headers, + // then the remaining values are ignored. + // + // Otherwise, if there are less values than headers, + // then `Value::nothing(span)` is used to fill the remaining columns. + rows.push(Value::record(columns.zip(values).collect(), span)); } Ok(Value::list(rows, span)) diff --git a/crates/nu-command/src/formats/from/nuon.rs b/crates/nu-command/src/formats/from/nuon.rs index edaccb51f2..efa771f5fd 100644 --- a/crates/nu-command/src/formats/from/nuon.rs +++ b/crates/nu-command/src/formats/from/nuon.rs @@ -421,15 +421,16 @@ fn convert_to_value( span: expr.span, }); } - let vals: Vec = row - .into_iter() - .map(|cell| convert_to_value(cell, span, original_text)) + + let record = cols + .iter() + .zip(row) + .map(|(col, cell)| { + convert_to_value(cell, span, original_text).map(|val| (col.clone(), val)) + }) .collect::>()?; - output.push(Value::record( - Record::from_raw_cols_vals_unchecked(cols.clone(), vals), - span, - )); + output.push(Value::record(record, span)); } Ok(Value::list(output, span)) diff --git a/crates/nu-command/src/strings/detect_columns.rs b/crates/nu-command/src/strings/detect_columns.rs index 437c90014c..91d4daf892 100644 --- a/crates/nu-command/src/strings/detect_columns.rs +++ b/crates/nu-command/src/strings/detect_columns.rs @@ -136,13 +136,11 @@ fn detect_columns( .map(move |x| { let row = find_columns(&x); - let mut cols = vec![]; - let mut vals = vec![]; + let mut record = Record::new(); if headers.len() == row.len() { for (header, val) in headers.iter().zip(row.iter()) { - cols.push(header.item.clone()); - vals.push(Value::string(val.item.clone(), name_span)); + record.push(&header.item, Value::string(&val.item, name_span)); } } else { let mut pre_output = vec![]; @@ -176,8 +174,7 @@ fn detect_columns( for header in &headers { for pre_o in &pre_output { if pre_o.0 == header.item { - cols.push(header.item.clone()); - vals.push(pre_o.1.clone()) + record.push(&header.item, pre_o.1.clone()); } } } @@ -187,25 +184,25 @@ fn detect_columns( match nu_cmd_base::util::process_range(range) { Ok((l_idx, r_idx)) => { let l_idx = if l_idx < 0 { - cols.len() as isize + l_idx + record.len() as isize + l_idx } else { l_idx }; let r_idx = if r_idx < 0 { - cols.len() as isize + r_idx + record.len() as isize + r_idx } else { r_idx }; - if !(l_idx <= r_idx && (r_idx >= 0 || l_idx < (cols.len() as isize))) { - return Value::record( - Record::from_raw_cols_vals_unchecked(cols, vals), - name_span, - ); + if !(l_idx <= r_idx && (r_idx >= 0 || l_idx < (record.len() as isize))) { + return Value::record(record, name_span); } - (l_idx.max(0) as usize, (r_idx as usize + 1).min(cols.len())) + ( + l_idx.max(0) as usize, + (r_idx as usize + 1).min(record.len()), + ) } Err(processing_error) => { let err = processing_error("could not find range index", name_span); @@ -213,9 +210,11 @@ fn detect_columns( } } } else { - return Value::record(Record::from_raw_cols_vals_unchecked(cols, vals), name_span); + return Value::record(record, name_span); }; + let (mut cols, mut vals): (Vec<_>, Vec<_>) = record.into_iter().unzip(); + // Merge Columns ((start_index + 1)..(cols.len() - end_index + start_index + 1)).for_each(|idx| { cols.swap(idx, end_index - start_index - 1 + idx); @@ -233,9 +232,12 @@ fn detect_columns( let last_seg = vals.split_off(end_index); vals.truncate(start_index); vals.push(binding); - last_seg.into_iter().for_each(|v| vals.push(v)); + vals.extend(last_seg); - Value::record(Record::from_raw_cols_vals_unchecked(cols, vals), name_span) + match Record::from_raw_cols_vals(cols, vals, Span::unknown(), name_span) { + Ok(record) => Value::record(record, name_span), + Err(err) => Value::error(err, name_span), + } }) .into_pipeline_data(ctrlc)) } else { diff --git a/crates/nu-command/tests/commands/database/into_sqlite.rs b/crates/nu-command/tests/commands/database/into_sqlite.rs index 585fc44515..b02bfa8b01 100644 --- a/crates/nu-command/tests/commands/database/into_sqlite.rs +++ b/crates/nu-command/tests/commands/database/into_sqlite.rs @@ -1,7 +1,7 @@ use std::{io::Write, path::PathBuf}; use chrono::{DateTime, FixedOffset, NaiveDateTime, Offset}; -use nu_protocol::{ast::PathMember, Record, Span, Value}; +use nu_protocol::{ast::PathMember, record, Span, Value}; use nu_test_support::{ fs::{line_ending, Stub}, nu, pipeline, @@ -234,22 +234,16 @@ impl TestRow { impl From for Value { fn from(row: TestRow) -> Self { Value::record( - Record::from_iter(vec![ - ("somebool".into(), Value::bool(row.0, Span::unknown())), - ("someint".into(), Value::int(row.1, Span::unknown())), - ("somefloat".into(), Value::float(row.2, Span::unknown())), - ( - "somefilesize".into(), - Value::filesize(row.3, Span::unknown()), - ), - ( - "someduration".into(), - Value::duration(row.4, Span::unknown()), - ), - ("somedate".into(), Value::date(row.5, Span::unknown())), - ("somestring".into(), Value::string(row.6, Span::unknown())), - ("somebinary".into(), Value::binary(row.7, Span::unknown())), - ]), + record! { + "somebool" => Value::bool(row.0, Span::unknown()), + "someint" => Value::int(row.1, Span::unknown()), + "somefloat" => Value::float(row.2, Span::unknown()), + "somefilesize" => Value::filesize(row.3, Span::unknown()), + "someduration" => Value::duration(row.4, Span::unknown()), + "somedate" => Value::date(row.5, Span::unknown()), + "somestring" => Value::string(row.6, Span::unknown()), + "somebinary" => Value::binary(row.7, Span::unknown()), + }, Span::unknown(), ) } diff --git a/crates/nu-engine/src/scope.rs b/crates/nu-engine/src/scope.rs index 548056fe63..ec67afa079 100644 --- a/crates/nu-engine/src/scope.rs +++ b/crates/nu-engine/src/scope.rs @@ -1,7 +1,7 @@ use nu_protocol::{ ast::Expr, engine::{Command, EngineState, Stack, Visibility}, - record, ModuleId, Record, Signature, Span, SyntaxShape, Type, Value, + record, ModuleId, Signature, Span, SyntaxShape, Type, Value, }; use std::cmp::Ordering; use std::collections::HashMap; @@ -185,101 +185,81 @@ impl<'e, 's> ScopeData<'e, 's> { ) -> Vec { let mut sig_records = vec![]; - let sig_cols = vec![ - "parameter_name".to_string(), - "parameter_type".to_string(), - "syntax_shape".to_string(), - "is_optional".to_string(), - "short_flag".to_string(), - "description".to_string(), - "custom_completion".to_string(), - "parameter_default".to_string(), - ]; - // input sig_records.push(Value::record( - Record::from_raw_cols_vals_unchecked( - sig_cols.clone(), - vec![ - Value::nothing(span), - Value::string("input", span), - Value::string(input_type.to_shape().to_string(), span), - Value::bool(false, span), - Value::nothing(span), - Value::nothing(span), - Value::nothing(span), - Value::nothing(span), - ], - ), + record! { + "parameter_name" => Value::nothing(span), + "parameter_type" => Value::string("input", span), + "syntax_shape" => Value::string(input_type.to_shape().to_string(), span), + "is_optional" => Value::bool(false, span), + "short_flag" => Value::nothing(span), + "description" => Value::nothing(span), + "custom_completion" => Value::nothing(span), + "parameter_default" => Value::nothing(span), + }, span, )); // required_positional for req in &signature.required_positional { - let sig_vals = vec![ - Value::string(&req.name, span), - Value::string("positional", span), - Value::string(req.shape.to_string(), span), - Value::bool(false, span), - Value::nothing(span), - Value::string(&req.desc, span), - Value::string( - extract_custom_completion_from_arg(self.engine_state, &req.shape), - span, - ), - Value::nothing(span), - ]; + let custom = extract_custom_completion_from_arg(self.engine_state, &req.shape); sig_records.push(Value::record( - Record::from_raw_cols_vals_unchecked(sig_cols.clone(), sig_vals), + record! { + "parameter_name" => Value::string(&req.name, span), + "parameter_type" => Value::string("positional", span), + "syntax_shape" => Value::string(req.shape.to_string(), span), + "is_optional" => Value::bool(false, span), + "short_flag" => Value::nothing(span), + "description" => Value::string(&req.desc, span), + "custom_completion" => Value::string(custom, span), + "parameter_default" => Value::nothing(span), + }, span, )); } // optional_positional for opt in &signature.optional_positional { - let sig_vals = vec![ - Value::string(&opt.name, span), - Value::string("positional", span), - Value::string(opt.shape.to_string(), span), - Value::bool(true, span), - Value::nothing(span), - Value::string(&opt.desc, span), - Value::string( - extract_custom_completion_from_arg(self.engine_state, &opt.shape), - span, - ), - if let Some(val) = &opt.default_value { - val.clone() - } else { - Value::nothing(span) - }, - ]; + let custom = extract_custom_completion_from_arg(self.engine_state, &opt.shape); + let default = if let Some(val) = &opt.default_value { + val.clone() + } else { + Value::nothing(span) + }; sig_records.push(Value::record( - Record::from_raw_cols_vals_unchecked(sig_cols.clone(), sig_vals), + record! { + "parameter_name" => Value::string(&opt.name, span), + "parameter_type" => Value::string("positional", span), + "syntax_shape" => Value::string(opt.shape.to_string(), span), + "is_optional" => Value::bool(true, span), + "short_flag" => Value::nothing(span), + "description" => Value::string(&opt.desc, span), + "custom_completion" => Value::string(custom, span), + "parameter_default" => default, + }, span, )); } // rest_positional if let Some(rest) = &signature.rest_positional { - let sig_vals = vec![ - Value::string(if rest.name == "rest" { "" } else { &rest.name }, span), - Value::string("rest", span), - Value::string(rest.shape.to_string(), span), - Value::bool(true, span), - Value::nothing(span), - Value::string(&rest.desc, span), - Value::string( - extract_custom_completion_from_arg(self.engine_state, &rest.shape), - span, - ), - Value::nothing(span), // rest_positional does have default, but parser prohibits specifying it?! - ]; + let name = if rest.name == "rest" { "" } else { &rest.name }; + let custom = extract_custom_completion_from_arg(self.engine_state, &rest.shape); sig_records.push(Value::record( - Record::from_raw_cols_vals_unchecked(sig_cols.clone(), sig_vals), + record! { + "parameter_name" => Value::string(name, span), + "parameter_type" => Value::string("rest", span), + "syntax_shape" => Value::string(rest.shape.to_string(), span), + "is_optional" => Value::bool(true, span), + "short_flag" => Value::nothing(span), + "description" => Value::string(&rest.desc, span), + "custom_completion" => Value::string(custom, span), + // rest_positional does have default, but parser prohibits specifying it?! + "parameter_default" => Value::nothing(span), + }, span, )); } @@ -310,42 +290,39 @@ impl<'e, 's> ScopeData<'e, 's> { Value::nothing(span) }; - let sig_vals = vec![ - Value::string(&named.long, span), - flag_type, - shape, - Value::bool(!named.required, span), - short_flag, - Value::string(&named.desc, span), - Value::string(custom_completion_command_name, span), - if let Some(val) = &named.default_value { - val.clone() - } else { - Value::nothing(span) - }, - ]; + let default = if let Some(val) = &named.default_value { + val.clone() + } else { + Value::nothing(span) + }; sig_records.push(Value::record( - Record::from_raw_cols_vals_unchecked(sig_cols.clone(), sig_vals), + record! { + "parameter_name" => Value::string(&named.long, span), + "parameter_type" => flag_type, + "syntax_shape" => shape, + "is_optional" => Value::bool(!named.required, span), + "short_flag" => short_flag, + "description" => Value::string(&named.desc, span), + "custom_completion" => Value::string(custom_completion_command_name, span), + "parameter_default" => default, + }, span, )); } // output sig_records.push(Value::record( - Record::from_raw_cols_vals_unchecked( - sig_cols, - vec![ - Value::nothing(span), - Value::string("output", span), - Value::string(output_type.to_shape().to_string(), span), - Value::bool(false, span), - Value::nothing(span), - Value::nothing(span), - Value::nothing(span), - Value::nothing(span), - ], - ), + record! { + "parameter_name" => Value::nothing(span), + "parameter_type" => Value::string("output", span), + "syntax_shape" => Value::string(output_type.to_shape().to_string(), span), + "is_optional" => Value::bool(false, span), + "short_flag" => Value::nothing(span), + "description" => Value::nothing(span), + "custom_completion" => Value::nothing(span), + "parameter_default" => Value::nothing(span), + }, span, )); diff --git a/crates/nu-explore/src/commands/help.rs b/crates/nu-explore/src/commands/help.rs index d3f69c6f3c..4245df07ef 100644 --- a/crates/nu-explore/src/commands/help.rs +++ b/crates/nu-explore/src/commands/help.rs @@ -4,7 +4,7 @@ use std::io::{self, Result}; use crossterm::event::KeyEvent; use nu_protocol::{ engine::{EngineState, Stack}, - record, Record, Value, + record, Value, }; use ratatui::layout::Rect; @@ -165,10 +165,7 @@ fn help_frame_data( let (cols, mut vals) = help_manual_data(manual, aliases); let vals = vals.remove(0); - Value::record( - Record::from_raw_cols_vals_unchecked(cols, vals), - NuSpan::unknown(), - ) + Value::record(cols.into_iter().zip(vals).collect(), NuSpan::unknown()) }) .collect(); let commands = Value::list(commands, NuSpan::unknown()); diff --git a/crates/nu-explore/src/views/record/mod.rs b/crates/nu-explore/src/views/record/mod.rs index 163a6a5504..a956399271 100644 --- a/crates/nu-explore/src/views/record/mod.rs +++ b/crates/nu-explore/src/views/record/mod.rs @@ -701,16 +701,13 @@ fn build_last_value(v: &RecordView) -> Value { fn build_table_as_list(v: &RecordView) -> Value { let layer = v.get_layer_last(); - let headers = layer.columns.to_vec(); + let cols = &layer.columns; let vals = layer .records .iter() - .cloned() .map(|vals| { - Value::record( - Record::from_raw_cols_vals_unchecked(headers.clone(), vals), - NuSpan::unknown(), - ) + let record = cols.iter().cloned().zip(vals.iter().cloned()).collect(); + Value::record(record, NuSpan::unknown()) }) .collect(); @@ -720,13 +717,18 @@ fn build_table_as_list(v: &RecordView) -> Value { fn build_table_as_record(v: &RecordView) -> Value { let layer = v.get_layer_last(); - let cols = layer.columns.to_vec(); - let vals = layer.records.first().map_or(Vec::new(), |row| row.clone()); + let record = if let Some(row) = layer.records.first() { + layer + .columns + .iter() + .cloned() + .zip(row.iter().cloned()) + .collect() + } else { + Record::new() + }; - Value::record( - Record::from_raw_cols_vals_unchecked(cols, vals), - NuSpan::unknown(), - ) + Value::record(record, NuSpan::unknown()) } fn report_cursor_position(mode: UIMode, cursor: XYCursor) -> String { diff --git a/crates/nu-protocol/src/eval_base.rs b/crates/nu-protocol/src/eval_base.rs index 0f5d8c8fa3..52b720720b 100644 --- a/crates/nu-protocol/src/eval_base.rs +++ b/crates/nu-protocol/src/eval_base.rs @@ -119,13 +119,12 @@ pub trait Eval { let mut output_rows = vec![]; for val in vals { - let mut row = vec![]; - for expr in val { - row.push(Self::eval(state, mut_state, expr)?); - } - // length equality already ensured in parser + let record = output_headers.iter().zip(val).map(|(col, expr)| { + Self::eval(state, mut_state, expr).map(|val| (col.clone(), val)) + }).collect::>()?; + output_rows.push(Value::record( - Record::from_raw_cols_vals_unchecked(output_headers.clone(), row), + record, expr.span, )); } diff --git a/crates/nu-protocol/src/value/record.rs b/crates/nu-protocol/src/value/record.rs index 97565c4f5e..405d4e67f2 100644 --- a/crates/nu-protocol/src/value/record.rs +++ b/crates/nu-protocol/src/value/record.rs @@ -26,24 +26,11 @@ impl Record { } } - /// Constructor that checks that `cols` and `vals` are of the same length. + /// Create a [`Record`] from a `Vec` of columns and a `Vec` of [`Value`]s /// - /// WARNING! Panics with assertion failure if cols and vals have different length! - /// Should be used only when the same lengths are guaranteed! + /// Returns an error if `cols` and `vals` have different lengths. /// - /// For perf reasons does not validate the rest of the record assumptions. - /// - unique keys - pub fn from_raw_cols_vals_unchecked(cols: Vec, vals: Vec) -> Self { - assert_eq!(cols.len(), vals.len()); - - Self { cols, vals } - } - - /// Constructor that checks that `cols` and `vals` are of the same length. - /// - /// Returns None if cols and vals have different length. - /// - /// For perf reasons does not validate the rest of the record assumptions. + /// For perf reasons, this will not validate the rest of the record assumptions: /// - unique keys pub fn from_raw_cols_vals( cols: Vec, @@ -70,11 +57,13 @@ impl Record { } pub fn is_empty(&self) -> bool { - self.cols.is_empty() || self.vals.is_empty() + debug_assert_eq!(self.cols.len(), self.vals.len()); + self.cols.is_empty() } pub fn len(&self) -> usize { - usize::min(self.cols.len(), self.vals.len()) + debug_assert_eq!(self.cols.len(), self.vals.len()); + self.cols.len() } /// Naive push to the end of the datastructure. @@ -99,14 +88,13 @@ impl Record { let curr_val = &mut self.vals[idx]; Some(std::mem::replace(curr_val, val)) } else { - self.cols.push(col.into()); - self.vals.push(val); + self.push(col, val); None } } pub fn contains(&self, col: impl AsRef) -> bool { - self.cols.iter().any(|k| k == col.as_ref()) + self.columns().any(|k| k == col.as_ref()) } pub fn index_of(&self, col: impl AsRef) -> Option { @@ -138,7 +126,6 @@ impl Record { /// Remove elements in-place that do not satisfy `keep` /// - /// Note: Panics if `vals.len() > cols.len()` /// ```rust /// use nu_protocol::{record, Value}; /// @@ -147,7 +134,7 @@ impl Record { /// "b" => Value::test_int(42), /// "c" => Value::test_nothing(), /// "d" => Value::test_int(42), - /// ); + /// ); /// rec.retain(|_k, val| !val.is_nothing()); /// let mut iter_rec = rec.columns(); /// assert_eq!(iter_rec.next().map(String::as_str), Some("b")); @@ -165,7 +152,6 @@ impl Record { /// /// This can for example be used to recursively prune nested records. /// - /// Note: Panics if `vals.len() > cols.len()` /// ```rust /// use nu_protocol::{record, Record, Value}; /// @@ -235,6 +221,7 @@ impl Record { /// Truncate record to the first `len` elements. /// /// `len > self.len()` will be ignored + /// /// ```rust /// use nu_protocol::{record, Value}; /// @@ -243,7 +230,7 @@ impl Record { /// "b" => Value::test_int(42), /// "c" => Value::test_nothing(), /// "d" => Value::test_int(42), - /// ); + /// ); /// rec.truncate(42); // this is fine /// assert_eq!(rec.columns().map(String::as_str).collect::(), "abcd"); /// rec.truncate(2); // truncate @@ -299,11 +286,7 @@ impl Record { where R: RangeBounds + Clone, { - assert_eq!( - self.cols.len(), - self.vals.len(), - "Length of cols and vals must be equal for sane `Record::drain`" - ); + debug_assert_eq!(self.cols.len(), self.vals.len()); Drain { keys: self.cols.drain(range.clone()), values: self.vals.drain(range), @@ -323,8 +306,7 @@ impl Extend<(String, Value)> for Record { fn extend>(&mut self, iter: T) { for (k, v) in iter { // TODO: should this .insert with a check? - self.cols.push(k); - self.vals.push(v); + self.push(k, v) } } } @@ -552,11 +534,15 @@ impl ExactSizeIterator for Drain<'_> { #[macro_export] macro_rules! record { + // The macro only compiles if the number of columns equals the number of values, + // so it's safe to call `unwrap` below. {$($col:expr => $val:expr),+ $(,)?} => { - $crate::Record::from_raw_cols_vals_unchecked( + $crate::Record::from_raw_cols_vals( ::std::vec![$($col.into(),)+], - ::std::vec![$($val,)+] - ) + ::std::vec![$($val,)+], + $crate::Span::unknown(), + $crate::Span::unknown(), + ).unwrap() }; {} => { $crate::Record::new() diff --git a/crates/nu_plugin_example/src/example.rs b/crates/nu_plugin_example/src/example.rs index 4e73e2c1c9..b9b8a2de51 100644 --- a/crates/nu_plugin_example/src/example.rs +++ b/crates/nu_plugin_example/src/example.rs @@ -1,5 +1,5 @@ use nu_plugin::{EvaluatedCall, LabeledError}; -use nu_protocol::{Record, Value}; +use nu_protocol::{record, Value}; pub struct Example; impl Example { @@ -75,20 +75,16 @@ impl Example { pub fn test2(&self, call: &EvaluatedCall, input: &Value) -> Result { self.print_values(2, call, input)?; - let cols = vec!["one".to_string(), "two".to_string(), "three".to_string()]; - let vals = (0..10i64) .map(|i| { - let vals = (0..3) - .map(|v| Value::int(v * i, call.head)) - .collect::>(); - - Value::record( - Record::from_raw_cols_vals_unchecked(cols.clone(), vals), - call.head, - ) + let record = record! { + "one" => Value::int(i, call.head), + "two" => Value::int(2 * i, call.head), + "three" => Value::int(3 * i, call.head), + }; + Value::record(record, call.head) }) - .collect::>(); + .collect(); Ok(Value::list(vals, call.head)) }