From 6a274b860a2e39e416f32ac0495484b602651d00 Mon Sep 17 00:00:00 2001 From: Reilly Wood <26268125+rgwood@users.noreply.github.com> Date: Wed, 22 Mar 2023 13:54:19 -0700 Subject: [PATCH] Cell paths: make optional path members short-circuit (#8554) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a follow-up to https://github.com/nushell/nushell/pull/8379 and https://github.com/nushell/nushell/discussions/8502. This PR makes it so that the new `?` syntax for marking a path member as optional short-circuits, as voted on in the [8502](https://github.com/nushell/nushell/discussions/8502) poll. Previously, `{ foo: 123 }.bar?.baz` would raise an error: ``` > { foo: 123 }.bar?.baz × Data cannot be accessed with a cell path ╭─[entry #15:1:1] 1 │ { foo: 123 }.bar?.baz · ─┬─ · ╰── nothing doesn't support cell paths ╰──── ``` Here's what was happening: 1. The `bar?` path member access returns `nothing` because there is no field named `bar` on the record 2. The `baz` path member access fails when trying to access a `baz` field on that `nothing` value After this change, `{ foo: 123 }.bar?.baz` returns `nothing`; the failed `bar?` access immediately returns `nothing` and the `baz` access never runs. --- crates/nu-protocol/src/value/mod.rs | 17 +++++++++-------- src/tests/test_cell_path.rs | 15 +++++++++++++-- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/crates/nu-protocol/src/value/mod.rs b/crates/nu-protocol/src/value/mod.rs index 83ebddb718..c4cb313d93 100644 --- a/crates/nu-protocol/src/value/mod.rs +++ b/crates/nu-protocol/src/value/mod.rs @@ -742,7 +742,7 @@ impl Value { if let Some(item) = val.get(*count) { current = item.clone(); } else if *optional { - current = Value::nothing(*origin_span); + return Ok(Value::nothing(*origin_span)); // short-circuit } else if val.is_empty() { err_or_null!( ShellError::AccessEmptyContent { span: *origin_span }, @@ -762,7 +762,7 @@ impl Value { if let Some(item) = val.get(*count) { current = Value::int(*item as i64, *origin_span); } else if *optional { - current = Value::nothing(*origin_span); + return Ok(Value::nothing(*origin_span)); // short-circuit } else if val.is_empty() { err_or_null!( ShellError::AccessEmptyContent { span: *origin_span }, @@ -782,7 +782,7 @@ impl Value { if let Some(item) = val.clone().into_range_iter(None)?.nth(*count) { current = item.clone(); } else if *optional { - current = Value::nothing(*origin_span); + return Ok(Value::nothing(*origin_span)); // short-circuit } else { err_or_null!( ShellError::AccessBeyondEndOfStream { span: *origin_span }, @@ -795,7 +795,8 @@ impl Value { Ok(val) => val, Err(err) => { if *optional { - Value::nothing(*origin_span) + return Ok(Value::nothing(*origin_span)); + // short-circuit } else { return Err(err); } @@ -803,7 +804,7 @@ impl Value { }; } Value::Nothing { .. } if *optional => { - current = Value::nothing(*origin_span); + return Ok(Value::nothing(*origin_span)); // short-circuit } // Records (and tables) are the only built-in which support column names, // so only use this message for them. @@ -843,7 +844,7 @@ impl Value { }) { current = found.1.clone(); } else if *optional { - current = Value::nothing(*origin_span); + return Ok(Value::nothing(*origin_span)); // short-circuit } else { if from_user_input { if let Some(suggestion) = did_you_mean(&cols, column_name) { @@ -869,7 +870,7 @@ impl Value { if columns.contains(&column_name.as_str()) { current = val.get_column_value(column_name)?; } else if *optional { - current = Value::nothing(*origin_span); + return Ok(Value::nothing(*origin_span)); // short-circuit } else { if from_user_input { if let Some(suggestion) = did_you_mean(&columns, column_name) { @@ -934,7 +935,7 @@ impl Value { current = val.follow_path_string(column_name.clone(), *origin_span)?; } Value::Nothing { .. } if *optional => { - current = Value::nothing(*origin_span); + return Ok(Value::nothing(*origin_span)); // short-circuit } Value::Error { error } => err_or_null!(*error.to_owned(), *origin_span), x => { diff --git a/src/tests/test_cell_path.rs b/src/tests/test_cell_path.rs index 021246b6a3..fc434a4818 100644 --- a/src/tests/test_cell_path.rs +++ b/src/tests/test_cell_path.rs @@ -48,8 +48,10 @@ fn record_single_field_optional() -> TestResult { } #[test] -fn record_single_field_optional_does_not_short_circuit() -> TestResult { - fail_test("{foo: 'bar'}.foobar?.baz", "nothing") +fn record_single_field_optional_short_circuits() -> TestResult { + // Check that we return null as soon as the `.foobar?` access + // fails instead of erroring on the `.baz` access + run_test("{foo: 'bar'}.foobar?.baz | to nuon", "null") } #[test] @@ -138,3 +140,12 @@ fn do_not_delve_too_deep_in_nested_lists() -> TestResult { fn cell_path_literals() -> TestResult { run_test("let cell_path = $.a.b; {a: {b: 3}} | get $cell_path", "3") } + +// Test whether cell path access short-circuits properly +#[test] +fn deeply_nested_cell_path_short_circuits() -> TestResult { + run_test( + "{foo: [{bar: 'baz'}]}.foo.3?.bar.asdfdafg.234.foobar | to nuon", + "null", + ) +}