diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index e8441b241d..851be44504 100644 --- a/crates/nu-cli/src/repl.rs +++ b/crates/nu-cli/src/repl.rs @@ -734,60 +734,63 @@ pub fn eval_hook( } } Value::Record { .. } => { - let do_run_hook = - if let Ok(condition) = value.clone().follow_cell_path(&[condition_path], false) { - match condition { - Value::Block { - val: block_id, - span: block_span, - .. - } - | Value::Closure { - val: block_id, - span: block_span, - .. - } => { - match run_hook_block( - engine_state, - stack, - block_id, - None, - arguments.clone(), - block_span, - ) { - Ok(pipeline_data) => { - if let PipelineData::Value(Value::Bool { val, .. }, ..) = - pipeline_data - { - val - } else { - return Err(ShellError::UnsupportedConfigValue( - "boolean output".to_string(), - "other PipelineData variant".to_string(), - block_span, - )); - } - } - Err(err) => { - return Err(err); + let do_run_hook = if let Ok(condition) = + value + .clone() + .follow_cell_path(&[condition_path], false, false) + { + match condition { + Value::Block { + val: block_id, + span: block_span, + .. + } + | Value::Closure { + val: block_id, + span: block_span, + .. + } => { + match run_hook_block( + engine_state, + stack, + block_id, + None, + arguments.clone(), + block_span, + ) { + Ok(pipeline_data) => { + if let PipelineData::Value(Value::Bool { val, .. }, ..) = + pipeline_data + { + val + } else { + return Err(ShellError::UnsupportedConfigValue( + "boolean output".to_string(), + "other PipelineData variant".to_string(), + block_span, + )); } } - } - other => { - return Err(ShellError::UnsupportedConfigValue( - "block".to_string(), - format!("{}", other.get_type()), - other.span()?, - )); + Err(err) => { + return Err(err); + } } } - } else { - // always run the hook - true - }; + other => { + return Err(ShellError::UnsupportedConfigValue( + "block".to_string(), + format!("{}", other.get_type()), + other.span()?, + )); + } + } + } else { + // always run the hook + true + }; if do_run_hook { - match value.clone().follow_cell_path(&[code_path], false)? { + match value.clone().follow_cell_path(&[code_path], false, false)? { Value::String { val, span: source_span, diff --git a/crates/nu-command/src/filters/drop/column.rs b/crates/nu-command/src/filters/drop/column.rs index 2884e7a8d2..b34694a20d 100644 --- a/crates/nu-command/src/filters/drop/column.rs +++ b/crates/nu-command/src/filters/drop/column.rs @@ -109,7 +109,10 @@ fn dropcol( let mut vals = vec![]; for path in &keep_columns { - let fetcher = input_val.clone().follow_cell_path(&path.members, false)?; + let fetcher = + input_val + .clone() + .follow_cell_path(&path.members, false, false)?; cols.push(path.into_string()); vals.push(fetcher); } @@ -133,7 +136,10 @@ fn dropcol( let mut vals = vec![]; for path in &keep_columns { - let fetcher = input_val.clone().follow_cell_path(&path.members, false)?; + let fetcher = + input_val + .clone() + .follow_cell_path(&path.members, false, false)?; cols.push(path.into_string()); vals.push(fetcher); } @@ -149,7 +155,9 @@ fn dropcol( let mut vals = vec![]; for cell_path in &keep_columns { - let result = v.clone().follow_cell_path(&cell_path.members, false)?; + let result = v + .clone() + .follow_cell_path(&cell_path.members, false, false)?; cols.push(cell_path.into_string()); vals.push(result); diff --git a/crates/nu-command/src/filters/empty.rs b/crates/nu-command/src/filters/empty.rs index 404ae3b3eb..7500993986 100644 --- a/crates/nu-command/src/filters/empty.rs +++ b/crates/nu-command/src/filters/empty.rs @@ -73,7 +73,7 @@ fn empty( for val in input { for column in &columns { let val = val.clone(); - match val.follow_cell_path(&column.members, false) { + match val.follow_cell_path(&column.members, false, false) { Ok(Value::Nothing { .. }) => {} Ok(_) => return Ok(Value::boolean(false, head).into_pipeline_data()), Err(err) => return Err(err), diff --git a/crates/nu-command/src/filters/get.rs b/crates/nu-command/src/filters/get.rs index 0481175776..4f28c8cfcc 100644 --- a/crates/nu-command/src/filters/get.rs +++ b/crates/nu-command/src/filters/get.rs @@ -64,40 +64,9 @@ impl Command for Get { let metadata = input.metadata(); if rest.is_empty() { - let output = input - .follow_cell_path(&cell_path.members, call.head, !sensitive) - .map(|x| x.into_pipeline_data()); - - if ignore_errors { - match output { - Ok(output) => Ok(output), - Err(_) => Ok(Value::Nothing { span: call.head }.into_pipeline_data()), - } - } else { - match output { - Ok(val) => { - let val_check = val.into_value(span); - match val_check { - Value::List { - ref vals, - span: spanchild, - } => { - if vals.iter().any(|unit| unit.is_empty()) { - Err(nu_protocol::ShellError::CantFindColumn( - "Empty cell".to_string(), - spanchild, - span, - )) - } else { - Ok(val_check.into_pipeline_data()) - } - } - val => Ok(val.into_pipeline_data()), - } - } - Err(e) => Err(e), - } - } + input + .follow_cell_path(&cell_path.members, call.head, !sensitive, ignore_errors) + .map(|x| x.into_pipeline_data()) } else { let mut output = vec![]; @@ -106,25 +75,11 @@ impl Command for Get { let input = input.into_value(span); for path in paths { - let val = input.clone().follow_cell_path(&path.members, !sensitive); + let val = input + .clone() + .follow_cell_path(&path.members, !sensitive, false); - if ignore_errors { - match val { - Ok(Value::Nothing { span: spanchild }) => { - return Err(nu_protocol::ShellError::CantFindColumn( - "Nothing".to_string(), - spanchild, - span, - )); - } - Ok(val) => { - output.push(val); - } - Err(_) => {} - } - } else { - output.push(val?); - } + output.push(val?); } Ok(output.into_iter().into_pipeline_data(ctrlc)) @@ -152,17 +107,13 @@ impl Command for Get { result: Some(Value::test_string("A0")), }, Example { - description: "Extract the name of files as a list", - example: "ls | get name", - result: None, - }, - Example { - description: "Extract the name of the 3rd entry of a file list", + description: + "Extract the name of the 3rd record in a list (same as `ls | $in.name`)", example: "ls | get name.2", result: None, }, Example { - description: "Extract the name of the 3rd entry of a file list (alternative)", + description: "Extract the name of the 3rd record in a list", example: "ls | get 2.name", result: None, }, diff --git a/crates/nu-command/src/filters/length.rs b/crates/nu-command/src/filters/length.rs index 8197f1d6e6..c39bfe7450 100644 --- a/crates/nu-command/src/filters/length.rs +++ b/crates/nu-command/src/filters/length.rs @@ -81,7 +81,17 @@ fn length_row(call: &Call, input: PipelineData) -> Result { Ok(Value::int(0, call.head).into_pipeline_data()) } - _ => Ok(Value::int(input.into_iter().count() as i64, call.head).into_pipeline_data()), + _ => { + let mut count: i64 = 0; + // Check for and propagate errors + for value in input.into_iter() { + if let Value::Error { error } = value { + return Err(error); + } + count += 1 + } + Ok(Value::int(count, call.head).into_pipeline_data()) + } } } diff --git a/crates/nu-command/src/filters/select.rs b/crates/nu-command/src/filters/select.rs index d48e6d27d4..a05bf50204 100644 --- a/crates/nu-command/src/filters/select.rs +++ b/crates/nu-command/src/filters/select.rs @@ -156,7 +156,11 @@ fn select( let mut vals = vec![]; for path in &columns { //FIXME: improve implementation to not clone - match input_val.clone().follow_cell_path(&path.members, false) { + match input_val.clone().follow_cell_path( + &path.members, + false, + ignore_errors, + ) { Ok(fetcher) => { allempty = false; cols.push(path.into_string().replace('.', "_")); @@ -166,12 +170,7 @@ fn select( } } Err(e) => { - if ignore_errors { - cols.push(path.into_string().replace('.', "_")); - vals.push(Value::Nothing { span }) - } else { - return Err(e); - } + return Err(e); } } } @@ -199,17 +198,15 @@ fn select( let mut vals = vec![]; for path in &columns { //FIXME: improve implementation to not clone - match x.clone().follow_cell_path(&path.members, false) { + match x + .clone() + .follow_cell_path(&path.members, false, ignore_errors) + { Ok(value) => { cols.push(path.into_string().replace('.', "_")); vals.push(value); } - Err(e) => { - if ignore_errors { - return Ok(Value::nothing(call_span).into_pipeline_data()); - } - return Err(e); - } + Err(e) => return Err(e), } } values.push(Value::Record { @@ -233,18 +230,15 @@ fn select( for cell_path in columns { // FIXME: remove clone - match v.clone().follow_cell_path(&cell_path.members, false) { + match v + .clone() + .follow_cell_path(&cell_path.members, false, ignore_errors) + { Ok(result) => { cols.push(cell_path.into_string().replace('.', "_")); vals.push(result); } - Err(e) => { - if ignore_errors { - return Ok(Value::nothing(call_span).into_pipeline_data()); - } - - return Err(e); - } + Err(e) => return Err(e), } } diff --git a/crates/nu-command/src/strings/format/command.rs b/crates/nu-command/src/strings/format/command.rs index 4b51e4e8ed..ac3849c689 100644 --- a/crates/nu-command/src/strings/format/command.rs +++ b/crates/nu-command/src/strings/format/command.rs @@ -282,7 +282,10 @@ fn format_record( span: *span, }) .collect(); - match data_as_value.clone().follow_cell_path(&path_members, false) { + match data_as_value + .clone() + .follow_cell_path(&path_members, false, false) + { Ok(value_at_column) => { output.push_str(value_at_column.into_string(", ", config).as_str()) } diff --git a/crates/nu-command/src/viewers/griddle.rs b/crates/nu-command/src/viewers/griddle.rs index edd5beab59..5ebbd64966 100644 --- a/crates/nu-command/src/viewers/griddle.rs +++ b/crates/nu-command/src/viewers/griddle.rs @@ -76,7 +76,7 @@ prints out the list properly."# match input { PipelineData::Value(Value::List { vals, .. }, ..) => { // dbg!("value::list"); - let data = convert_to_list(vals, config, call.head); + let data = convert_to_list(vals, config, call.head)?; if let Some(items) = data { Ok(create_grid_output( items, @@ -93,7 +93,7 @@ prints out the list properly."# } PipelineData::ListStream(stream, ..) => { // dbg!("value::stream"); - let data = convert_to_list(stream, config, call.head); + let data = convert_to_list(stream, config, call.head)?; if let Some(items) = data { Ok(create_grid_output( items, @@ -247,11 +247,12 @@ fn create_grid_output( ) } +#[allow(clippy::type_complexity)] fn convert_to_list( iter: impl IntoIterator, config: &Config, head: Span, -) -> Option> { +) -> Result>, ShellError> { let mut iter = iter.into_iter().peekable(); if let Some(first) = iter.peek() { @@ -267,7 +268,7 @@ fn convert_to_list( let mut row = vec![row_num.to_string()]; if headers.is_empty() { - row.push(item.into_string(", ", config)) + row.push(item.nonerror_into_string(", ", config)?) } else { for header in headers.iter().skip(1) { let result = match item { @@ -277,12 +278,13 @@ fn convert_to_list( span: head, }], false, + false, ), _ => Ok(item.clone()), }; match result { - Ok(value) => row.push(value.into_string(", ", config)), + Ok(value) => row.push(value.nonerror_into_string(", ", config)?), Err(_) => row.push(String::new()), } } @@ -315,9 +317,9 @@ fn convert_to_list( } } - Some(interleaved) + Ok(Some(interleaved)) } else { - None + Ok(None) } } diff --git a/crates/nu-command/src/viewers/table.rs b/crates/nu-command/src/viewers/table.rs index 676e04946a..4bd07cdbae 100644 --- a/crates/nu-command/src/viewers/table.rs +++ b/crates/nu-command/src/viewers/table.rs @@ -5,10 +5,10 @@ use nu_engine::{column::get_columns, env_to_string, CallExt}; use nu_protocol::TrimStrategy; use nu_protocol::{ ast::{Call, PathMember}, - engine::{Command, EngineState, Stack, StateWorkingSet}, - format_error, Category, Config, DataSource, Example, FooterMode, IntoPipelineData, ListStream, - PipelineData, PipelineMetadata, RawStream, ShellError, Signature, Span, SyntaxShape, - TableIndexMode, Type, Value, + engine::{Command, EngineState, Stack}, + Category, Config, DataSource, Example, FooterMode, IntoPipelineData, ListStream, PipelineData, + PipelineMetadata, RawStream, ShellError, Signature, Span, SyntaxShape, TableIndexMode, Type, + Value, }; use nu_table::{string_width, Table as NuTable, TableConfig, TableTheme}; use nu_utils::get_ls_colors; @@ -331,12 +331,9 @@ fn handle_table_command( Ok(val.into_pipeline_data()) } PipelineData::Value(Value::Error { error }, ..) => { - let working_set = StateWorkingSet::new(engine_state); - Ok(Value::String { - val: format_error(&working_set, &error), - span: call.head, - } - .into_pipeline_data()) + // Propagate this error outward, so that it goes to stderr + // instead of stdout. + Err(error) } PipelineData::Value(Value::CustomValue { val, span }, ..) => { let base_pipeline = val.to_base_value(span)?.into_pipeline_data(); @@ -903,7 +900,7 @@ fn convert_to_table( val: text.clone(), span: head, }; - let val = item.clone().follow_cell_path(&[path], false); + let val = item.clone().follow_cell_path(&[path], false, false); match val { Ok(val) => DeferredStyleComputation::Value { value: val }, @@ -1264,7 +1261,7 @@ fn create_table2_entry_basic( Value::Record { .. } => { let val = header.to_owned(); let path = PathMember::String { val, span: head }; - let val = item.clone().follow_cell_path(&[path], false); + let val = item.clone().follow_cell_path(&[path], false, false); match val { Ok(val) => value_to_styled_string(&val, config, style_computer), @@ -1292,7 +1289,7 @@ fn create_table2_entry( Value::Record { .. } => { let val = header.to_owned(); let path = PathMember::String { val, span: head }; - let val = item.clone().follow_cell_path(&[path], false); + let val = item.clone().follow_cell_path(&[path], false, false); match val { Ok(val) => convert_to_table2_entry( diff --git a/crates/nu-command/tests/commands/default.rs b/crates/nu-command/tests/commands/default.rs index 334b45ee68..7a045a6318 100644 --- a/crates/nu-command/tests/commands/default.rs +++ b/crates/nu-command/tests/commands/default.rs @@ -24,7 +24,7 @@ fn adds_row_data_if_column_missing() { r#" open los_tres_amigos.json | get amigos - | default rusty_luck 1 + | default 1 rusty_luck | where rusty_luck == 1 | length "# diff --git a/crates/nu-command/tests/commands/get.rs b/crates/nu-command/tests/commands/get.rs index bdc44a4644..9084ff4e97 100644 --- a/crates/nu-command/tests/commands/get.rs +++ b/crates/nu-command/tests/commands/get.rs @@ -239,5 +239,5 @@ fn get_does_not_delve_too_deep_in_nested_lists() { r#"[[{foo: bar}]] | get foo"# ); - assert!(actual.err.contains("did not find anything under this name")); + assert!(actual.err.contains("cannot find column")); } diff --git a/crates/nu-command/tests/commands/open.rs b/crates/nu-command/tests/commands/open.rs index fad3e9baa0..5c1f4f03a0 100644 --- a/crates/nu-command/tests/commands/open.rs +++ b/crates/nu-command/tests/commands/open.rs @@ -179,7 +179,7 @@ fn parses_json() { fn parses_xml() { let actual = nu!( cwd: "tests/fixtures/formats", - "open jonathan.xml | get rss.children.channel.children | get -i 0.item.children | get 3.link.children.3.0" + "open jonathan.xml | get rss.children.channel.children | get 0.3.item.children | get 3.link.children.0" ); assert_eq!( diff --git a/crates/nu-command/tests/commands/select.rs b/crates/nu-command/tests/commands/select.rs index 957fb57489..f13751c8eb 100644 --- a/crates/nu-command/tests/commands/select.rs +++ b/crates/nu-command/tests/commands/select.rs @@ -161,7 +161,7 @@ fn selects_many_rows() { } #[test] -fn select_ignores_errors_succesfully1() { +fn select_ignores_errors_successfully1() { let actual = nu!( cwd: ".", pipeline( r#" @@ -174,37 +174,40 @@ fn select_ignores_errors_succesfully1() { } #[test] -fn select_ignores_errors_succesfully2() { +fn select_ignores_errors_successfully2() { let actual = nu!( cwd: ".", pipeline( r#" - [{a: 1} {a: 2} {a: 3}] | select -i b + [{a: 1} {a: 2} {a: 3}] | select -i b | to nuon "# )); - assert!(actual.out.is_empty()); + assert_eq!(actual.out, "[[b]; [null], [null], [null]]".to_string()); assert!(actual.err.is_empty()); } #[test] -fn select_ignores_errors_succesfull3() { +fn select_ignores_errors_successfully3() { let actual = nu!( cwd: ".", pipeline( - r#"sys | select -i invalid_key"# + r#"sys | select -i invalid_key | to nuon"# )); - assert!(actual.out.is_empty()); + assert_eq!(actual.out, "{invalid_key: null}".to_string()); assert!(actual.err.is_empty()); } #[test] -fn select_ignores_errors_succesfully4() { +fn select_ignores_errors_successfully4() { let actual = nu!( cwd: ".", pipeline( - r#"[a b c] | select -i invalid_key"# + r#"[a b c] | select -i invalid_key | to nuon"# )); - assert!(actual.out.is_empty()); + assert_eq!( + actual.out, + "[[invalid_key]; [null], [null], [null]]".to_string() + ); assert!(actual.err.is_empty()); } @@ -223,10 +226,13 @@ fn select_ignores_errors_successfully5() { fn select_ignores_errors_successfully6() { let actual = nu!( cwd: ".", pipeline( - r#""key val\na 1\nb 2\n" | lines | split column -c " " | select -i "100""# + r#""key val\na 1\nb 2\n" | lines | split column -c " " | select -i "100" | to nuon"# )); - assert!(actual.out.is_empty()); + assert_eq!( + actual.out, + r#"[["100"]; [null], [null], [null]]"#.to_string() + ); assert!(actual.err.is_empty()); } diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 3c85a341f1..c49882c21f 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -302,7 +302,7 @@ pub fn eval_expression( Expr::FullCellPath(cell_path) => { let value = eval_expression(engine_state, stack, &cell_path.head)?; - value.follow_cell_path(&cell_path.tail, false) + value.follow_cell_path(&cell_path.tail, false, false) } Expr::ImportPattern(_) => Ok(Value::Nothing { span: expr.span }), Expr::Overlay(_) => { @@ -477,6 +477,7 @@ pub fn eval_expression( let vardata = lhs.follow_cell_path( &[cell_path.tail[0].clone()], false, + false, )?; match &cell_path.tail[0] { PathMember::String { val, .. } => { diff --git a/crates/nu-explore/src/nu_common/table.rs b/crates/nu-explore/src/nu_common/table.rs index 299a6b7c89..9c69ab700a 100644 --- a/crates/nu-explore/src/nu_common/table.rs +++ b/crates/nu-explore/src/nu_common/table.rs @@ -605,7 +605,7 @@ fn create_table2_entry_basic( Value::Record { .. } => { let val = header.to_owned(); let path = PathMember::String { val, span: head }; - let val = item.clone().follow_cell_path(&[path], false); + let val = item.clone().follow_cell_path(&[path], false, false); match val { Ok(val) => value_to_styled_string(&val, config, style_computer), @@ -633,7 +633,7 @@ fn create_table2_entry( Value::Record { .. } => { let val = header.to_owned(); let path = PathMember::String { val, span: head }; - let val = item.clone().follow_cell_path(&[path], false); + let val = item.clone().follow_cell_path(&[path], false, false); match val { Ok(val) => convert_to_table2_entry( diff --git a/crates/nu-explore/src/nu_common/value.rs b/crates/nu-explore/src/nu_common/value.rs index f5536168c6..a12ca6536f 100644 --- a/crates/nu-explore/src/nu_common/value.rs +++ b/crates/nu-explore/src/nu_common/value.rs @@ -170,7 +170,7 @@ fn record_lookup_value(item: &Value, header: &str) -> Value { span: NuSpan::unknown(), }; - let value = item.clone().follow_cell_path(&[path], false); + let value = item.clone().follow_cell_path(&[path], false, false); match value { Ok(value) => value, Err(_) => item.clone(), diff --git a/crates/nu-parser/src/eval.rs b/crates/nu-parser/src/eval.rs index 40bfca5752..2bc773ed5e 100644 --- a/crates/nu-parser/src/eval.rs +++ b/crates/nu-parser/src/eval.rs @@ -31,7 +31,7 @@ pub fn eval_constant( Expr::FullCellPath(cell_path) => { let value = eval_constant(working_set, &cell_path.head)?; - match value.follow_cell_path(&cell_path.tail, false) { + match value.follow_cell_path(&cell_path.tail, false, false) { Ok(val) => Ok(val), // TODO: Better error conversion Err(shell_error) => Err(ParseError::LabeledError( diff --git a/crates/nu-protocol/src/pipeline_data.rs b/crates/nu-protocol/src/pipeline_data.rs index c06cb3c55a..e2219e0d3e 100644 --- a/crates/nu-protocol/src/pipeline_data.rs +++ b/crates/nu-protocol/src/pipeline_data.rs @@ -321,6 +321,7 @@ impl PipelineData { cell_path: &[PathMember], head: Span, insensitive: bool, + ignore_errors: bool, ) -> Result { match self { // FIXME: there are probably better ways of doing this @@ -328,8 +329,8 @@ impl PipelineData { vals: stream.collect(), span: head, } - .follow_cell_path(cell_path, insensitive), - PipelineData::Value(v, ..) => v.follow_cell_path(cell_path, insensitive), + .follow_cell_path(cell_path, insensitive, ignore_errors), + PipelineData::Value(v, ..) => v.follow_cell_path(cell_path, insensitive, ignore_errors), _ => Err(ShellError::IOError("can't follow stream paths".into())), } } @@ -671,9 +672,11 @@ impl PipelineData { to_stderr: bool, ) -> Result { for item in self { + let mut is_err = false; let mut out = if let Value::Error { error } = item { let working_set = StateWorkingSet::new(engine_state); - + // Value::Errors must always go to stderr, not stdout. + is_err = true; format_error(&working_set, &error) } else if no_newline { item.into_string("", config) @@ -685,7 +688,7 @@ impl PipelineData { out.push('\n'); } - if !to_stderr { + if !to_stderr && !is_err { stdout_write_all_and_flush(out)? } else { stderr_write_all_and_flush(out)? diff --git a/crates/nu-protocol/src/value/mod.rs b/crates/nu-protocol/src/value/mod.rs index ba8ae442ce..d67470f0ca 100644 --- a/crates/nu-protocol/src/value/mod.rs +++ b/crates/nu-protocol/src/value/mod.rs @@ -468,6 +468,18 @@ impl Value { } } + // Convert Value into String, but propagate errors. + pub fn nonerror_into_string( + &self, + separator: &str, + config: &Config, + ) -> Result { + match self { + Value::Error { error } => Err(error.to_owned()), + _ => Ok(self.into_string(separator, config)), + } + } + /// Convert Value into string. Note that Streams will be consumed. pub fn into_string(&self, separator: &str, config: &Config) -> String { match self { @@ -627,8 +639,9 @@ impl Value { self, cell_path: &[PathMember], insensitive: bool, + nullify_errors: bool, ) -> Result { - self.follow_cell_path_helper(cell_path, insensitive, true) + self.follow_cell_path_helper(cell_path, insensitive, nullify_errors, true) } pub fn follow_cell_path_not_from_user_input( @@ -636,16 +649,26 @@ impl Value { cell_path: &[PathMember], insensitive: bool, ) -> Result { - self.follow_cell_path_helper(cell_path, insensitive, false) + self.follow_cell_path_helper(cell_path, insensitive, false, false) } fn follow_cell_path_helper( self, cell_path: &[PathMember], insensitive: bool, + nullify_errors: bool, // Turn all errors into Value::Nothing from_user_input: bool, ) -> Result { let mut current = self; + macro_rules! err_or_null { + ($e:expr, $span:expr) => { + return if nullify_errors { + Ok(Value::nothing($span)) + } else { + Err($e) + } + }; + } for member in cell_path { // FIXME: this uses a few extra clones for simplicity, but there may be a way // to traverse the path without them @@ -660,34 +683,61 @@ impl Value { if let Some(item) = val.get(*count) { current = item.clone(); } else if val.is_empty() { - return Err(ShellError::AccessEmptyContent(*origin_span)) + err_or_null!( + ShellError::AccessEmptyContent(*origin_span), + *origin_span + ) } else { - return Err(ShellError::AccessBeyondEnd(val.len() - 1, *origin_span)); + err_or_null!( + ShellError::AccessBeyondEnd(val.len() - 1, *origin_span), + *origin_span + ); } } Value::Binary { val, .. } => { if let Some(item) = val.get(*count) { current = Value::int(*item as i64, *origin_span); } else if val.is_empty() { - return Err(ShellError::AccessEmptyContent(*origin_span)) + err_or_null!( + ShellError::AccessEmptyContent(*origin_span), + *origin_span + ) } else { - return Err(ShellError::AccessBeyondEnd(val.len() - 1, *origin_span)); + err_or_null!( + ShellError::AccessBeyondEnd(val.len() - 1, *origin_span), + *origin_span + ); } } Value::Range { val, .. } => { if let Some(item) = val.clone().into_range_iter(None)?.nth(*count) { current = item.clone(); } else { - return Err(ShellError::AccessBeyondEndOfStream(*origin_span)); + err_or_null!( + ShellError::AccessBeyondEndOfStream(*origin_span), + *origin_span + ); } } Value::CustomValue { val, .. } => { current = val.follow_path_int(*count, *origin_span)?; } + // Records (and tables) are the only built-in which support column names, + // so only use this message for them. + Value::Record { .. } => { + err_or_null!(ShellError::TypeMismatchGenericMessage { + err_message: "Can't access record values with a row index. Try specifying a column name instead".into(), + span: *origin_span, }, *origin_span) + } + Value::Error { error } => return Err(error.to_owned()), x => { - return Err(ShellError::TypeMismatchGenericMessage { - err_message: format!("Can't access {} values with a row index. Try specifying a column name instead", x.get_type().to_shape()), - span: *origin_span, }) + err_or_null!( + ShellError::IncompatiblePathAccess( + format!("{}", x.get_type()), + *origin_span, + ), + *origin_span + ) } } } @@ -711,16 +761,26 @@ impl Value { } else { if from_user_input { if let Some(suggestion) = did_you_mean(&cols, column_name) { - return Err(ShellError::DidYouMean(suggestion, *origin_span)); + err_or_null!( + ShellError::DidYouMean(suggestion, *origin_span), + *origin_span + ); } } - return Err(ShellError::CantFindColumn( - column_name.to_string(), - *origin_span, - span, - )); + err_or_null!( + ShellError::CantFindColumn( + column_name.to_string(), + *origin_span, + span, + ), + *origin_span + ); } } + // String access of Lists always means Table access. + // Create a List which contains each matching value for contained + // records in the source list. + // If nullify_errors is true, table holes are converted to null. Value::List { vals, span } => { let mut output = vec![]; let mut found_at_least_1_value = false; @@ -733,14 +793,50 @@ impl Value { span: *origin_span, }], insensitive, + nullify_errors, ) { found_at_least_1_value = true; output.push(result); } else { - output.push(Value::Nothing { span: *span }); + // Consider [{a:1 b:2} {a:3}]: + // [{a:1 b:2} {a:3}].b should error. + // [{a:1 b:2} {a:3}].b.1 should error. + // [{a:1 b:2} {a:3}].b.0 should NOT error because the path can find a proper value (2) + // but if this returns an error immediately, it will. + // + // Solution: push a Value::Error into this result list instead of returning it. + // This also means that `[{a:1 b:2} {a:2}].b | reject 1` also doesn't error. + // Anything that needs to use every value inside the list should propagate + // the error outward, though. + output.push(if nullify_errors { + Value::nothing(*origin_span) + } else { + Value::Error { + error: ShellError::CantFindColumn( + column_name.to_string(), + *origin_span, + // Get the exact span of the value, falling back to + // the list's span if it's a Value::Empty + val.span().unwrap_or(*span), + ), + } + }); } } else { - output.push(Value::Nothing { span: *span }); + // See comment above. + output.push(if nullify_errors { + Value::nothing(*origin_span) + } else { + Value::Error { + error: ShellError::CantFindColumn( + column_name.to_string(), + *origin_span, + // Get the exact span of the value, falling back to + // the list's span if it's a Value::Empty + val.span().unwrap_or(*span), + ), + } + }); } } if found_at_least_1_value { @@ -749,23 +845,39 @@ impl Value { span: *span, }; } else { - return Err(ShellError::NotFound(*span)); + err_or_null!( + ShellError::CantFindColumn( + column_name.to_string(), + *origin_span, + *span, + ), + *origin_span + ); } } Value::CustomValue { val, .. } => { current = val.follow_path_string(column_name.clone(), *origin_span)?; } + Value::Error { error } => err_or_null!(error.to_owned(), *origin_span), x => { - return Err(ShellError::IncompatiblePathAccess( - format!("{}", x.get_type()), - *origin_span, - )) + err_or_null!( + ShellError::IncompatiblePathAccess( + format!("{}", x.get_type()), + *origin_span, + ), + *origin_span + ) } }, } } - - Ok(current) + // If a single Value::Error was produced by the above (which won't happen if nullify_errors is true), unwrap it now. + // Note that Value::Errors inside Lists remain as they are, so that the rest of the list can still potentially be used. + if let Value::Error { error } = current { + Err(error) + } else { + Ok(current) + } } /// Follow a given cell path into the value: for example accessing select elements in a stream or list @@ -776,7 +888,7 @@ impl Value { ) -> Result<(), ShellError> { let orig = self.clone(); - let new_val = callback(&orig.follow_cell_path(cell_path, false)?); + let new_val = callback(&orig.follow_cell_path(cell_path, false, false)?); match new_val { Value::Error { error } => Err(error), @@ -829,6 +941,7 @@ impl Value { } } } + Value::Error { error } => return Err(error.to_owned()), v => { return Err(ShellError::CantFindColumn( col_name.to_string(), @@ -865,6 +978,7 @@ impl Value { } } } + Value::Error { error } => return Err(error.to_owned()), v => { return Err(ShellError::CantFindColumn( col_name.to_string(), @@ -885,6 +999,7 @@ impl Value { return Err(ShellError::InsertAfterNextFreeIndex(vals.len(), *span)); } } + Value::Error { error } => return Err(error.to_owned()), v => return Err(ShellError::NotAList(*span, v.span()?)), }, }, @@ -903,7 +1018,7 @@ impl Value { ) -> Result<(), ShellError> { let orig = self.clone(); - let new_val = callback(&orig.follow_cell_path(cell_path, false)?); + let new_val = callback(&orig.follow_cell_path(cell_path, false, false)?); match new_val { Value::Error { error } => Err(error), @@ -948,6 +1063,7 @@ impl Value { )); } } + Value::Error { error } => return Err(error.to_owned()), v => { return Err(ShellError::CantFindColumn( col_name.to_string(), @@ -981,6 +1097,7 @@ impl Value { )); } } + Value::Error { error } => return Err(error.to_owned()), v => { return Err(ShellError::CantFindColumn( col_name.to_string(), @@ -999,6 +1116,7 @@ impl Value { return Err(ShellError::AccessBeyondEnd(vals.len() - 1, *span)); } } + Value::Error { error } => return Err(error.to_owned()), v => return Err(ShellError::NotAList(*span, v.span()?)), }, }, diff --git a/crates/nu_plugin_inc/src/inc.rs b/crates/nu_plugin_inc/src/inc.rs index f12e48c51d..6696c601a8 100644 --- a/crates/nu_plugin_inc/src/inc.rs +++ b/crates/nu_plugin_inc/src/inc.rs @@ -72,7 +72,7 @@ impl Inc { pub fn inc(&self, head: Span, value: &Value) -> Result { if let Some(cell_path) = &self.cell_path { let working_value = value.clone(); - let cell_value = working_value.follow_cell_path(&cell_path.members, false)?; + let cell_value = working_value.follow_cell_path(&cell_path.members, false, false)?; let cell_value = self.inc_value(head, &cell_value)?; diff --git a/src/tests/test_cell_path.rs b/src/tests/test_cell_path.rs index d6837cbd03..49636506c6 100644 --- a/src/tests/test_cell_path.rs +++ b/src/tests/test_cell_path.rs @@ -8,7 +8,7 @@ fn nothing_fails_string() -> TestResult { #[test] fn nothing_fails_int() -> TestResult { - fail_test("$nothing.3", "Can't access") + fail_test("$nothing.3", "doesn't support cell paths") } // Tests for records @@ -67,9 +67,9 @@ fn list_single_field_failure() -> TestResult { // Test the scenario where the requested column is not present in all rows #[test] -fn jagged_list_access_succeeds() -> TestResult { - run_test("[{foo: 'bar'}, {}].foo == ['bar', $nothing]", "true")?; - run_test("[{}, {foo: 'bar'}].foo == [$nothing, 'bar']", "true") +fn jagged_list_access_fails() -> TestResult { + fail_test("[{foo: 'bar'}, {}].foo", "cannot find column")?; + fail_test("[{}, {foo: 'bar'}].foo", "cannot find column") } // test that accessing a nonexistent row fails @@ -81,8 +81,5 @@ fn list_row_access_failure() -> TestResult { // regression test for an old bug #[test] fn do_not_delve_too_deep_in_nested_lists() -> TestResult { - fail_test( - "[[{foo: bar}]].foo", - "did not find anything under this name", - ) + fail_test("[[{foo: bar}]].foo", "cannot find column") } diff --git a/src/tests/test_table_operations.rs b/src/tests/test_table_operations.rs index 296e53eafd..a7a2e9caa5 100644 --- a/src/tests/test_table_operations.rs +++ b/src/tests/test_table_operations.rs @@ -1,4 +1,4 @@ -use crate::tests::{run_test, TestResult}; +use crate::tests::{fail_test, run_test, TestResult}; #[test] fn cell_path_subexpr1() -> TestResult { @@ -172,11 +172,10 @@ fn update_cell_path_1() -> TestResult { } #[test] -fn missing_column_fills_in_nothing() -> TestResult { - // The empty value will be replaced with null when fetching a column - run_test( +fn missing_column_errors() -> TestResult { + fail_test( r#"[ { name: ABC, size: 20 }, { name: HIJ } ].size.1 == null"#, - "true", + "cannot find column", ) } @@ -257,8 +256,20 @@ fn length_defaulted_columns() -> TestResult { } #[test] -fn get_fuzzy() -> TestResult { - run_test("(ls | get -i foo) == null", "true") +fn nullify_errors() -> TestResult { + run_test("([{a:1} {a:2} {a:3}] | get -i foo | length) == 3", "true")?; + run_test( + "([{a:1} {a:2} {a:3}] | get -i foo | to nuon) == '[null, null, null]'", + "true", + ) +} + +#[test] +fn nullify_holes() -> TestResult { + run_test( + "([{a:1} {b:2} {a:3}] | get -i a | to nuon) == '[1, null, 3]'", + "true", + ) } #[test]