From 379e3d70ca54eb48229c45a5f41342a96ee5d446 Mon Sep 17 00:00:00 2001 From: JT <547158+jntrnr@users.noreply.github.com> Date: Thu, 8 Dec 2022 12:02:11 +1300 Subject: [PATCH] Better errors when bash-like operators are used (#7241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description Adds improved errors for when a user uses a bashism that nu doesn't support. fixes #7237 Examples: ``` Error: nu::parser::shell_andand (link) × The '&&' operator is not supported in Nushell ╭─[entry #1:1:1] 1 │ ls && ls · ─┬ · ╰── instead of '&&', use ';' or 'and' ╰──── help: use ';' instead of the shell '&&', or 'and' instead of the boolean '&&' ``` ``` Error: nu::parser::shell_oror (link) × The '||' operator is not supported in Nushell ╭─[entry #8:1:1] 1 │ ls || ls · ─┬ · ╰── instead of '||', use 'try' or 'or' ╰──── help: use 'try' instead of the shell '||', or 'or' instead of the boolean '||' ``` ``` Error: nu::parser::shell_err (link) × The '2>' shell operation is 'err>' in Nushell. ╭─[entry #9:1:1] 1 │ foo 2> bar.txt · ─┬ · ╰── use 'err>' instead of '2>' in Nushell ╰──── ``` ``` Error: nu::parser::shell_outerr (link) × The '2>&1' shell operation is 'out+err>' in Nushell. ╭─[entry #10:1:1] 1 │ foo 2>&1 bar.txt · ──┬─ · ╰── use 'out+err>' instead of '2>&1' in Nushell ╰──── help: Nushell redirection will write all of stdout before stderr. ``` # User-Facing Changes **BREAKING CHANGES** This removes the `&&` and `||` operators. We previously supported by `&&`/`and` and `||`/`or`. With this change, only `and` and `or` are valid boolean operators. # Tests + Formatting Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass # After Submitting If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --- crates/nu-command/tests/commands/math/mod.rs | 8 ++-- crates/nu-command/tests/commands/zip.rs | 2 +- crates/nu-parser/src/errors.rs | 32 +++++++++++++++ crates/nu-parser/src/lex.rs | 24 ++++++++++- crates/nu-parser/src/parser.rs | 43 ++++++++++---------- crates/nu-parser/tests/test_parser.rs | 10 ++--- src/tests/test_engine.rs | 4 +- src/tests/test_math.rs | 6 +-- src/tests/test_type_check.rs | 2 +- 9 files changed, 92 insertions(+), 39 deletions(-) diff --git a/crates/nu-command/tests/commands/math/mod.rs b/crates/nu-command/tests/commands/math/mod.rs index fda1273614..77c22cecc6 100644 --- a/crates/nu-command/tests/commands/math/mod.rs +++ b/crates/nu-command/tests/commands/math/mod.rs @@ -431,7 +431,7 @@ fn compound_comparison() { let actual = nu!( cwd: "tests/fixtures/formats", pipeline( r#" - 4 > 3 && 2 > 1 + 4 > 3 and 2 > 1 "# )); @@ -443,7 +443,7 @@ fn compound_comparison2() { let actual = nu!( cwd: "tests/fixtures/formats", pipeline( r#" - 4 < 3 || 2 > 1 + 4 < 3 or 2 > 1 "# )); @@ -455,7 +455,7 @@ fn compound_where() { let actual = nu!( cwd: "tests/fixtures/formats", pipeline( r#" - echo '[{"a": 1, "b": 1}, {"a": 2, "b": 1}, {"a": 2, "b": 2}]' | from json | where a == 2 && b == 1 | to json -r + echo '[{"a": 1, "b": 1}, {"a": 2, "b": 1}, {"a": 2, "b": 2}]' | from json | where a == 2 and b == 1 | to json -r "# )); @@ -467,7 +467,7 @@ fn compound_where_paren() { let actual = nu!( cwd: "tests/fixtures/formats", pipeline( r#" - echo '[{"a": 1, "b": 1}, {"a": 2, "b": 1}, {"a": 2, "b": 2}]' | from json | where ($it.a == 2 && $it.b == 1) || $it.b == 2 | to json -r + echo '[{"a": 1, "b": 1}, {"a": 2, "b": 1}, {"a": 2, "b": 2}]' | from json | where ($it.a == 2 and $it.b == 1) or $it.b == 2 | to json -r "# )); diff --git a/crates/nu-command/tests/commands/zip.rs b/crates/nu-command/tests/commands/zip.rs index 5c7693a4b1..e8b743616e 100644 --- a/crates/nu-command/tests/commands/zip.rs +++ b/crates/nu-command/tests/commands/zip.rs @@ -9,7 +9,7 @@ export def expect [ right ] { $left | zip $right | all {|row| - $row.name.0 == $row.name.1 && $row.commits.0 == $row.commits.1 + $row.name.0 == $row.name.1 and $row.commits.0 == $row.commits.1 } } "#; diff --git a/crates/nu-parser/src/errors.rs b/crates/nu-parser/src/errors.rs index fc2d05d0a4..f121d71218 100644 --- a/crates/nu-parser/src/errors.rs +++ b/crates/nu-parser/src/errors.rs @@ -42,6 +42,34 @@ pub enum ParseError { #[diagnostic(code(nu::parser::type_mismatch), url(docsrs))] Mismatch(String, String, #[label("expected {0}, found {1}")] Span), // expected, found, span + #[error("The '&&' operator is not supported in Nushell")] + #[diagnostic( + code(nu::parser::shell_andand), + url(docsrs), + help("use ';' instead of the shell '&&', or 'and' instead of the boolean '&&'") + )] + ShellAndAnd(#[label("instead of '&&', use ';' or 'and'")] Span), + + #[error("The '||' operator is not supported in Nushell")] + #[diagnostic( + code(nu::parser::shell_oror), + url(docsrs), + help("use 'try' instead of the shell '||', or 'or' instead of the boolean '||'") + )] + ShellOrOr(#[label("instead of '||', use 'try' or 'or'")] Span), + + #[error("The '2>' shell operation is 'err>' in Nushell.")] + #[diagnostic(code(nu::parser::shell_err), url(docsrs))] + ShellErrRedirect(#[label("use 'err>' instead of '2>' in Nushell")] Span), + + #[error("The '2>&1' shell operation is 'out+err>' in Nushell.")] + #[diagnostic( + code(nu::parser::shell_outerr), + url(docsrs), + help("Nushell redirection will write all of stdout before stderr.") + )] + ShellOutErrRedirect(#[label("use 'out+err>' instead of '2>&1' in Nushell")] Span), + #[error("Types mismatched for operation.")] #[diagnostic( code(nu::parser::unsupported_operation), @@ -421,6 +449,10 @@ impl ParseError { ParseError::FileNotFound(_, s) => *s, ParseError::ReadingFile(_, s) => *s, ParseError::LabeledError(_, _, s) => *s, + ParseError::ShellAndAnd(s) => *s, + ParseError::ShellOrOr(s) => *s, + ParseError::ShellErrRedirect(s) => *s, + ParseError::ShellOutErrRedirect(s) => *s, ParseError::UnknownOperator(_, _, s) => *s, } } diff --git a/crates/nu-parser/src/lex.rs b/crates/nu-parser/src/lex.rs index fb972dccfb..8dce2f90aa 100644 --- a/crates/nu-parser/src/lex.rs +++ b/crates/nu-parser/src/lex.rs @@ -6,6 +6,7 @@ pub enum TokenContents { Item, Comment, Pipe, + PipePipe, Semicolon, OutGreaterThan, ErrGreaterThan, @@ -251,6 +252,27 @@ pub fn lex_item( }, None, ), + b"&&" => ( + Token { + contents: TokenContents::Item, + span, + }, + Some(ParseError::ShellAndAnd(span)), + ), + b"2>" => ( + Token { + contents: TokenContents::Item, + span, + }, + Some(ParseError::ShellErrRedirect(span)), + ), + b"2>&1" => ( + Token { + contents: TokenContents::Item, + span, + }, + Some(ParseError::ShellOutErrRedirect(span)), + ), _ => ( Token { contents: TokenContents::Item, @@ -289,7 +311,7 @@ pub fn lex( let idx = curr_offset; curr_offset += 1; output.push(Token::new( - TokenContents::Item, + TokenContents::PipePipe, Span::new(span_offset + prev_idx, span_offset + idx + 1), )); continue; diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 8dfd5d550a..ea736db3ba 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -1212,18 +1212,17 @@ fn parse_binary_with_base( binary_value.extend_from_slice(contents); } - TokenContents::Pipe => { + TokenContents::Pipe + | TokenContents::PipePipe + | TokenContents::OutGreaterThan + | TokenContents::ErrGreaterThan + | TokenContents::OutErrGreaterThan => { return ( garbage(span), Some(ParseError::Expected("binary".into(), span)), ); } - TokenContents::Comment - | TokenContents::Semicolon - | TokenContents::Eol - | TokenContents::OutGreaterThan - | TokenContents::ErrGreaterThan - | TokenContents::OutErrGreaterThan => {} + TokenContents::Comment | TokenContents::Semicolon | TokenContents::Eol => {} } } @@ -4075,19 +4074,12 @@ pub fn parse_closure_expression( (Some((signature, signature_span)), amt_to_skip) } Some(Token { - contents: TokenContents::Item, + contents: TokenContents::PipePipe, span, - }) => { - let contents = working_set.get_span_contents(*span); - if contents == b"||" { - ( - Some((Box::new(Signature::new("closure".to_string())), *span)), - 1, - ) - } else { - (None, 0) - } - } + }) => ( + Some((Box::new(Signature::new("closure".to_string())), *span)), + 1, + ), _ => (None, 0), }; @@ -4456,8 +4448,8 @@ pub fn parse_operator( b"bit-shr" => Operator::Bits(Bits::ShiftRight), b"starts-with" => Operator::Comparison(Comparison::StartsWith), b"ends-with" => Operator::Comparison(Comparison::EndsWith), - b"&&" | b"and" => Operator::Boolean(Boolean::And), - b"||" | b"or" => Operator::Boolean(Boolean::Or), + b"and" => Operator::Boolean(Boolean::And), + b"or" => Operator::Boolean(Boolean::Or), b"xor" => Operator::Boolean(Boolean::Xor), b"**" => Operator::Math(Math::Pow), // WARNING: not actual operators below! Error handling only @@ -5916,8 +5908,15 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option) { let mut curr_comment: Option> = None; + let mut error = None; + for token in tokens.iter() { match &token.contents { + TokenContents::PipePipe => { + error = error.or(Some(ParseError::ShellOrOr(token.span))); + curr_command.push(token.span); + last_token = TokenContents::Item; + } TokenContents::Item => { // If we have a comment, go ahead and attach it if let Some(curr_comment) = curr_comment.take() { @@ -6163,7 +6162,7 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option) { )), ) } else { - (block, None) + (block, error) } } diff --git a/crates/nu-parser/tests/test_parser.rs b/crates/nu-parser/tests/test_parser.rs index d59da00990..02600ccd79 100644 --- a/crates/nu-parser/tests/test_parser.rs +++ b/crates/nu-parser/tests/test_parser.rs @@ -1388,11 +1388,11 @@ mod input_types { let mut working_set = StateWorkingSet::new(&engine_state); let inputs = vec![ - r#"let a = 'b'; ($a == 'b') || ($a == 'b')"#, - r#"let a = 'b'; ($a == 'b') || ($a == 'b') && ($a == 'b')"#, - r#"let a = 1; ($a == 1) || ($a == 2) && ($a == 3)"#, - r#"let a = 'b'; if ($a == 'b') || ($a == 'b') { true } else { false }"#, - r#"let a = 1; if ($a == 1) || ($a > 0) { true } else { false }"#, + r#"let a = 'b'; ($a == 'b') or ($a == 'b')"#, + r#"let a = 'b'; ($a == 'b') or ($a == 'b') and ($a == 'b')"#, + r#"let a = 1; ($a == 1) or ($a == 2) and ($a == 3)"#, + r#"let a = 'b'; if ($a == 'b') or ($a == 'b') { true } else { false }"#, + r#"let a = 1; if ($a == 1) or ($a > 0) { true } else { false }"#, ]; for input in inputs { diff --git a/src/tests/test_engine.rs b/src/tests/test_engine.rs index b026ea93a1..dc8f7fd388 100644 --- a/src/tests/test_engine.rs +++ b/src/tests/test_engine.rs @@ -238,12 +238,12 @@ fn datetime_literal() -> TestResult { #[test] fn shortcircuiting_and() -> TestResult { - run_test(r#"false && (5 / 0; false)"#, "false") + run_test(r#"false and (5 / 0; false)"#, "false") } #[test] fn shortcircuiting_or() -> TestResult { - run_test(r#"true || (5 / 0; false)"#, "true") + run_test(r#"true or (5 / 0; false)"#, "true") } #[test] diff --git a/src/tests/test_math.rs b/src/tests/test_math.rs index 8dee151bef..094be2e7c6 100644 --- a/src/tests/test_math.rs +++ b/src/tests/test_math.rs @@ -47,12 +47,12 @@ fn sub_bit_shr() -> TestResult { #[test] fn and() -> TestResult { - run_test("true && false", "false") + run_test("true and false", "false") } #[test] fn or() -> TestResult { - run_test("true || false", "true") + run_test("true or false", "true") } #[test] @@ -112,7 +112,7 @@ fn floating_add() -> TestResult { #[test] fn precedence_of_or_groups() -> TestResult { - run_test(r#"4 mod 3 == 0 || 5 mod 5 == 0"#, "true") + run_test(r#"4 mod 3 == 0 or 5 mod 5 == 0"#, "true") } #[test] diff --git a/src/tests/test_type_check.rs b/src/tests/test_type_check.rs index ae93ceb37d..cf022ad57b 100644 --- a/src/tests/test_type_check.rs +++ b/src/tests/test_type_check.rs @@ -2,7 +2,7 @@ use crate::tests::{fail_test, run_test, TestResult}; #[test] fn chained_operator_typecheck() -> TestResult { - run_test("1 != 2 && 3 != 4 && 5 != 6", "true") + run_test("1 != 2 and 3 != 4 and 5 != 6", "true") } #[test]