From 771e24913d3bf85998058c131ccb99445cd0e4e4 Mon Sep 17 00:00:00 2001 From: Bob Hyman Date: Fri, 7 Apr 2023 07:40:05 -0400 Subject: [PATCH] range operator accepts bot..=top as well as bot..top (#8382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description A compromise fix for #8162. Nushell range operator now accepts `..=` to mean the range includes the top value, so you can use your Rust habits. But the unadorned `..` range operator also includes the value, so you can also use your Nushell habits. _(Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience.)_ ```nushell 〉1..5 ╭───┬───╮ │ 0 │ 1 │ │ 1 │ 2 │ │ 2 │ 3 │ │ 3 │ 4 │ │ 4 │ 5 │ ╰───┴───╯ -------------------------------------------- /home/bobhy/src/rust/nushell -------------------------------------------- 〉1..=5 ╭───┬───╮ │ 0 │ 1 │ │ 1 │ 2 │ │ 2 │ 3 │ │ 3 │ 4 │ │ 4 │ 5 │ ╰───┴───╯ -------------------------------------------- /home/bobhy/src/rust/nushell -------------------------------------------- 〉1..<5 ╭───┬───╮ │ 0 │ 1 │ │ 1 │ 2 │ │ 2 │ 3 │ │ 3 │ 4 │ ╰───┴───╯ ``` # User-Facing Changes Existing scripts with range operator will continue to operate as heretofore. _(List of all changes that impact the user experience here. This helps us keep track of breaking changes.)_ # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - [x] `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - [x] `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - [x] `cargo test --workspace` to check that all tests pass # After Submitting Will update the book to include new syntax. --- Cargo.lock | 33 +- crates/nu-parser/Cargo.toml | 3 + crates/nu-parser/src/parser.rs | 13 +- crates/nu-parser/tests/test_parser.rs | 442 +++++++++++--------------- 4 files changed, 229 insertions(+), 262 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 81f3a2912..9f7f71e9e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2708,7 +2708,7 @@ dependencies = [ "pretty_assertions", "rayon", "reedline", - "rstest", + "rstest 0.17.0", "serde_json", "serial_test", "signal-hook", @@ -2751,7 +2751,7 @@ dependencies = [ "once_cell", "percent-encoding", "reedline", - "rstest", + "rstest 0.17.0", "sysinfo 0.28.2", "thiserror", ] @@ -2863,7 +2863,7 @@ dependencies = [ "reedline", "regex", "roxmltree", - "rstest", + "rstest 0.17.0", "rusqlite", "rust-embed", "same-file", @@ -2954,6 +2954,7 @@ dependencies = [ "nu-path", "nu-plugin", "nu-protocol", + "rstest 0.16.0", "serde_json", "thiserror", ] @@ -4348,16 +4349,40 @@ dependencies = [ "xmlparser", ] +[[package]] +name = "rstest" +version = "0.16.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b07f2d176c472198ec1e6551dc7da28f1c089652f66a7b722676c2238ebc0edf" +dependencies = [ + "rstest_macros 0.16.0", + "rustc_version", +] + [[package]] name = "rstest" version = "0.17.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "de1bb486a691878cd320c2f0d319ba91eeaa2e894066d8b5f8f117c000e9d962" dependencies = [ - "rstest_macros", + "rstest_macros 0.17.0", "rustc_version", ] +[[package]] +name = "rstest_macros" +version = "0.16.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7229b505ae0706e64f37ffc54a9c163e11022a6636d58fe1f3f52018257ff9f7" +dependencies = [ + "cfg-if 1.0.0", + "proc-macro2", + "quote", + "rustc_version", + "syn 1.0.109", + "unicode-ident", +] + [[package]] name = "rstest_macros" version = "0.17.0" diff --git a/crates/nu-parser/Cargo.toml b/crates/nu-parser/Cargo.toml index af9cb6aad..a12690327 100644 --- a/crates/nu-parser/Cargo.toml +++ b/crates/nu-parser/Cargo.toml @@ -23,5 +23,8 @@ nu-plugin = { path = "../nu-plugin", optional = true, version = "0.78.1" } nu-engine = { path = "../nu-engine", version = "0.78.1" } log = "0.4" +[dev-dependencies] +rstest = { version = "0.16.0", default-features = false } + [features] plugin = ["nu-plugin"] diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 982cea32b..38c157844 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -1500,7 +1500,7 @@ pub fn parse_range( // Range follows the following syntax: [][][] // where is ".." - // and is ".." or "..<" + // and is "..", "..=" or "..<" // and one of the or bounds must be present (just '..' is not allowed since it // looks like parent directory) //bugbug range cannot be [..] because that looks like parent directory @@ -1553,12 +1553,12 @@ pub fn parse_range( return garbage(span); } } else { - let op_str = ".."; + let op_str = if token.contains("..=") { "..=" } else { ".." }; let op_span = Span::new( span.start + range_op_pos, span.start + range_op_pos + op_str.len(), ); - (RangeInclusion::Inclusive, "..", op_span) + (RangeInclusion::Inclusive, op_str, op_span) }; // Now, based on the operator positions, figure out where the bounds & next are located and @@ -5178,7 +5178,12 @@ pub fn parse_expression( let split = name.splitn(2, |x| *x == b'='); let split: Vec<_> = split.collect(); - if !name.starts_with(b"^") && split.len() == 2 && !split[0].is_empty() { + if !name.starts_with(b"^") + && split.len() == 2 + && !split[0].is_empty() + && !split[0].ends_with(b"..") + // was range op ..= + { let point = split[0].len() + 1; let starting_error_count = working_set.parse_errors.len(); diff --git a/crates/nu-parser/tests/test_parser.rs b/crates/nu-parser/tests/test_parser.rs index 00e784372..d36d53e59 100644 --- a/crates/nu-parser/tests/test_parser.rs +++ b/crates/nu-parser/tests/test_parser.rs @@ -6,6 +6,7 @@ use nu_protocol::{ engine::{Command, EngineState, Stack, StateWorkingSet}, ParseError, PipelineData, ShellError, Signature, SyntaxShape, }; +use rstest::rstest; #[cfg(test)] #[derive(Clone)] @@ -974,334 +975,267 @@ mod range { use super::*; use nu_protocol::ast::{RangeInclusion, RangeOperator}; - #[test] - fn parse_inclusive_range() { + #[rstest] + #[case(b"0..10", RangeInclusion::Inclusive, "inclusive")] + #[case(b"0..=10", RangeInclusion::Inclusive, "=inclusive")] + #[case(b"0..<10", RangeInclusion::RightExclusive, "exclusive")] + #[case(b"10..0", RangeInclusion::Inclusive, "reverse inclusive")] + #[case(b"10..=0", RangeInclusion::Inclusive, "reverse =inclusive")] + #[case( + b"(3 - 3)..<(8 + 2)", + RangeInclusion::RightExclusive, + "subexpression exclusive" + )] + #[case( + b"(3 - 3)..(8 + 2)", + RangeInclusion::Inclusive, + "subexpression inclusive" + )] + #[case( + b"(3 - 3)..=(8 + 2)", + RangeInclusion::Inclusive, + "subexpression =inclusive" + )] + #[case(b"-10..-3", RangeInclusion::Inclusive, "negative inclusive")] + #[case(b"-10..=-3", RangeInclusion::Inclusive, "negative =inclusive")] + #[case(b"-10..<-3", RangeInclusion::RightExclusive, "negative exclusive")] + + fn parse_bounded_range( + #[case] phrase: &[u8], + #[case] inclusion: RangeInclusion, + #[case] tag: &str, + ) { let engine_state = EngineState::new(); let mut working_set = StateWorkingSet::new(&engine_state); - let block = parse(&mut working_set, None, b"0..10", true, &[]); + let block = parse(&mut working_set, None, phrase, true, &[]); assert!(working_set.parse_errors.is_empty()); - assert_eq!(block.len(), 1); + assert_eq!(block.len(), 1, "{tag}: block length"); let expressions = &block[0]; - assert_eq!(expressions.len(), 1); - assert!(matches!( - expressions[0], - PipelineElement::Expression( - _, - Expression { - expr: Expr::Range( + assert_eq!(expressions.len(), 1, "{tag}: expression length"); + if let PipelineElement::Expression( + _, + Expression { + expr: + Expr::Range( Some(_), None, Some(_), RangeOperator { - inclusion: RangeInclusion::Inclusive, + inclusion: the_inclusion, .. - } + }, ), - .. - } - ) - )) + .. + }, + ) = expressions[0] + { + assert_eq!( + the_inclusion, inclusion, + "{tag}: wrong RangeInclusion {the_inclusion:?}" + ); + } else { + panic!("{tag}: expression mismatch.") + }; } - #[test] - fn parse_exclusive_range() { - let engine_state = EngineState::new(); - let mut working_set = StateWorkingSet::new(&engine_state); - - let block = parse(&mut working_set, None, b"0..<10", true, &[]); - - assert!(working_set.parse_errors.is_empty()); - assert_eq!(block.len(), 1); - - let expressions = &block[0]; - assert_eq!(expressions.len(), 1); - assert!(matches!( - expressions[0], - PipelineElement::Expression( - _, - Expression { - expr: Expr::Range( - Some(_), - None, - Some(_), - RangeOperator { - inclusion: RangeInclusion::RightExclusive, - .. - } - ), - .. - } - ) - )) - } - - #[test] - fn parse_reverse_range() { - let engine_state = EngineState::new(); - let mut working_set = StateWorkingSet::new(&engine_state); - - let block = parse(&mut working_set, None, b"10..0", true, &[]); - - assert!(working_set.parse_errors.is_empty()); - assert_eq!(block.len(), 1); - - let expressions = &block[0]; - assert_eq!(expressions.len(), 1); - assert!(matches!( - expressions[0], - PipelineElement::Expression( - _, - Expression { - expr: Expr::Range( - Some(_), - None, - Some(_), - RangeOperator { - inclusion: RangeInclusion::Inclusive, - .. - } - ), - .. - } - ) - )) - } - - #[test] - fn parse_subexpression_range() { - let engine_state = EngineState::new(); - let mut working_set = StateWorkingSet::new(&engine_state); - - let block = parse(&mut working_set, None, b"(3 - 3)..<(8 + 2)", true, &[]); - - assert!(working_set.parse_errors.is_empty()); - assert_eq!(block.len(), 1); - - let expressions = &block[0]; - assert_eq!(expressions.len(), 1); - assert!(matches!( - expressions[0], - PipelineElement::Expression( - _, - Expression { - expr: Expr::Range( - Some(_), - None, - Some(_), - RangeOperator { - inclusion: RangeInclusion::RightExclusive, - .. - } - ), - .. - } - ) - )) - } - - #[test] - fn parse_variable_range() { + #[rstest] + #[case( + b"let a = 2; $a..10", + RangeInclusion::Inclusive, + "variable start inclusive" + )] + #[case( + b"let a = 2; $a..=10", + RangeInclusion::Inclusive, + "variable start =inclusive" + )] + #[case( + b"let a = 2; $a..<($a + 10)", + RangeInclusion::RightExclusive, + "subexpression variable exclusive" + )] + fn parse_variable_range( + #[case] phrase: &[u8], + #[case] inclusion: RangeInclusion, + #[case] tag: &str, + ) { let engine_state = EngineState::new(); let mut working_set = StateWorkingSet::new(&engine_state); working_set.add_decl(Box::new(Let)); - let block = parse(&mut working_set, None, b"let a = 2; $a..10", true, &[]); + let block = parse(&mut working_set, None, phrase, true, &[]); assert!(working_set.parse_errors.is_empty()); - assert_eq!(block.len(), 2); + assert_eq!(block.len(), 2, "{tag} block len 2"); let expressions = &block[1]; - assert_eq!(expressions.len(), 1); - assert!(matches!( - expressions[0], - PipelineElement::Expression( - _, - Expression { - expr: Expr::Range( + assert_eq!(expressions.len(), 1, "{tag}: expression length 1"); + if let PipelineElement::Expression( + _, + Expression { + expr: + Expr::Range( Some(_), None, Some(_), RangeOperator { - inclusion: RangeInclusion::Inclusive, + inclusion: the_inclusion, .. - } + }, ), - .. - } - ) - )) + .. + }, + ) = expressions[0] + { + assert_eq!( + the_inclusion, inclusion, + "{tag}: wrong RangeInclusion {the_inclusion:?}" + ); + } else { + panic!("{tag}: expression mismatch.") + }; } - #[test] - fn parse_subexpression_variable_range() { + #[rstest] + #[case(b"0..", RangeInclusion::Inclusive, "right unbounded")] + #[case(b"0..=", RangeInclusion::Inclusive, "right unbounded =inclusive")] + #[case(b"0..<", RangeInclusion::RightExclusive, "right unbounded")] + + fn parse_right_unbounded_range( + #[case] phrase: &[u8], + #[case] inclusion: RangeInclusion, + #[case] tag: &str, + ) { let engine_state = EngineState::new(); let mut working_set = StateWorkingSet::new(&engine_state); - working_set.add_decl(Box::new(Let)); - - let block = parse( - &mut working_set, - None, - b"let a = 2; $a..<($a + 10)", - true, - &[], - ); + let block = parse(&mut working_set, None, phrase, true, &[]); assert!(working_set.parse_errors.is_empty()); - assert_eq!(block.len(), 2); - - let expressions = &block[1]; - assert_eq!(expressions.len(), 1); - assert!(matches!( - expressions[0], - PipelineElement::Expression( - _, - Expression { - expr: Expr::Range( - Some(_), - None, - Some(_), - RangeOperator { - inclusion: RangeInclusion::RightExclusive, - .. - } - ), - .. - } - ) - )) - } - - #[test] - fn parse_right_unbounded_range() { - let engine_state = EngineState::new(); - let mut working_set = StateWorkingSet::new(&engine_state); - - let block = parse(&mut working_set, None, b"0..", true, &[]); - - assert!(working_set.parse_errors.is_empty()); - assert_eq!(block.len(), 1); + assert_eq!(block.len(), 1, "{tag}: block len 1"); let expressions = &block[0]; - assert_eq!(expressions.len(), 1); - assert!(matches!( - expressions[0], - PipelineElement::Expression( - _, - Expression { - expr: Expr::Range( + assert_eq!(expressions.len(), 1, "{tag}: expression length 1"); + if let PipelineElement::Expression( + _, + Expression { + expr: + Expr::Range( Some(_), None, None, RangeOperator { - inclusion: RangeInclusion::Inclusive, + inclusion: the_inclusion, .. - } + }, ), - .. - } - ) - )) + .. + }, + ) = expressions[0] + { + assert_eq!( + the_inclusion, inclusion, + "{tag}: wrong RangeInclusion {the_inclusion:?}" + ); + } else { + panic!("{tag}: expression mismatch.") + }; } - #[test] - fn parse_left_unbounded_range() { + #[rstest] + #[case(b"..10", RangeInclusion::Inclusive, "left unbounded inclusive")] + #[case(b"..=10", RangeInclusion::Inclusive, "left unbounded =inclusive")] + #[case(b"..<10", RangeInclusion::RightExclusive, "left unbounded exclusive")] + + fn parse_left_unbounded_range( + #[case] phrase: &[u8], + #[case] inclusion: RangeInclusion, + #[case] tag: &str, + ) { let engine_state = EngineState::new(); let mut working_set = StateWorkingSet::new(&engine_state); - let block = parse(&mut working_set, None, b"..10", true, &[]); + let block = parse(&mut working_set, None, phrase, true, &[]); assert!(working_set.parse_errors.is_empty()); - assert_eq!(block.len(), 1); + assert_eq!(block.len(), 1, "{tag}: block len 1"); let expressions = &block[0]; - assert_eq!(expressions.len(), 1); - assert!(matches!( - expressions[0], - PipelineElement::Expression( - _, - Expression { - expr: Expr::Range( + assert_eq!(expressions.len(), 1, "{tag}: expression length 1"); + if let PipelineElement::Expression( + _, + Expression { + expr: + Expr::Range( None, None, Some(_), RangeOperator { - inclusion: RangeInclusion::Inclusive, + inclusion: the_inclusion, .. - } + }, ), - .. - } - ) - )) + .. + }, + ) = expressions[0] + { + assert_eq!( + the_inclusion, inclusion, + "{tag}: wrong RangeInclusion {the_inclusion:?}" + ); + } else { + panic!("{tag}: expression mismatch.") + }; } - #[test] - fn parse_negative_range() { + #[rstest] + #[case(b"2.0..4.0..10.0", RangeInclusion::Inclusive, "float inclusive")] + #[case(b"2.0..4.0..=10.0", RangeInclusion::Inclusive, "float =inclusive")] + #[case(b"2.0..4.0..<10.0", RangeInclusion::RightExclusive, "float exclusive")] + + fn parse_float_range( + #[case] phrase: &[u8], + #[case] inclusion: RangeInclusion, + #[case] tag: &str, + ) { let engine_state = EngineState::new(); let mut working_set = StateWorkingSet::new(&engine_state); - let block = parse(&mut working_set, None, b"-10..-3", true, &[]); + let block = parse(&mut working_set, None, phrase, true, &[]); assert!(working_set.parse_errors.is_empty()); - assert_eq!(block.len(), 1); + assert_eq!(block.len(), 1, "{tag}: block length 1"); let expressions = &block[0]; - assert_eq!(expressions.len(), 1); - assert!(matches!( - expressions[0], - PipelineElement::Expression( - _, - Expression { - expr: Expr::Range( - Some(_), - None, - Some(_), - RangeOperator { - inclusion: RangeInclusion::Inclusive, - .. - } - ), - .. - } - ) - )) - } - - #[test] - fn parse_float_range() { - let engine_state = EngineState::new(); - let mut working_set = StateWorkingSet::new(&engine_state); - - let block = parse(&mut working_set, None, b"2.0..4.0..10.0", true, &[]); - - assert!(working_set.parse_errors.is_empty()); - assert_eq!(block.len(), 1); - - let expressions = &block[0]; - assert_eq!(expressions.len(), 1); - assert!(matches!( - expressions[0], - PipelineElement::Expression( - _, - Expression { - expr: Expr::Range( + assert_eq!(expressions.len(), 1, "{tag}: expression length 1"); + if let PipelineElement::Expression( + _, + Expression { + expr: + Expr::Range( Some(_), Some(_), Some(_), RangeOperator { - inclusion: RangeInclusion::Inclusive, + inclusion: the_inclusion, .. - } + }, ), - .. - } - ) - )) + .. + }, + ) = expressions[0] + { + assert_eq!( + the_inclusion, inclusion, + "{tag}: wrong RangeInclusion {the_inclusion:?}" + ); + } else { + panic!("{tag}: expression mismatch.") + }; } #[test] @@ -1309,7 +1243,7 @@ mod range { let engine_state = EngineState::new(); let mut working_set = StateWorkingSet::new(&engine_state); - parse(&mut working_set, None, b"(0)..\"a\"", true, &[]); + let _ = parse(&mut working_set, None, b"(0)..\"a\"", true, &[]); assert!(!working_set.parse_errors.is_empty()); }