From b947e360db5bec3e3c93ba4a94591624faa6ac1e Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Tue, 4 Aug 2020 21:39:37 +0200 Subject: [PATCH] Allow newlines after && and || We do the same for pipes (#1285). This matches POSIX sh behavior. --- CHANGELOG.rst | 3 ++- src/ast.h | 3 ++- src/fish_tests.cpp | 14 +++++++++++++- src/parse_util.cpp | 13 ++++++++++++- tests/checks/andandoror.fish | 18 ++++++++++++++++++ tests/checks/indent.fish | 16 +++++++++++++++- 6 files changed, 62 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ec4ef8258..8146b6636 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -28,7 +28,8 @@ Notable improvements and fixes Syntax changes and new commands ------------------------------- - A new ``fish_is_root_user`` simplifies checking for superuser privilege, largely for use in prompts (#7031). -- Range limits in index range expansions like ``$x[$start..$end]`` may be omitted: ``$start`` and ``$end`` default to 1 and -1 (the last item) respectively. +- Range limits in index range expansions like ``$x[$start..$end]`` may be omitted: ``$start`` and ``$end`` default to 1 and -1 (the last item) respectively. +- Logical operators ``&&`` and ``||`` can be followed by newlines before their right operand, matching POSIX shells. Scripting improvements ---------------------- diff --git a/src/ast.h b/src/ast.h index 2a394d2b8..56a5ae58c 100644 --- a/src/ast.h +++ b/src/ast.h @@ -730,11 +730,12 @@ struct job_conjunction_continuation_t final : public branch_t { // The && or || token. token_t conjunction; + maybe_newlines_t newlines; // The job itself. job_t job; - FIELDS(conjunction, job) + FIELDS(conjunction, newlines, job) }; // An andor_job just wraps a job, but requires that the job have an 'and' or 'or' job_decorator. diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 0f1b58293..1ae837725 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -1190,6 +1190,18 @@ static void test_parser() { err(L"bogus boolean statement error not detected on line %d", __LINE__); } + if (detect_errors(L"true && ") != PARSER_TEST_INCOMPLETE) { + err(L"unterminated conjunction not reported properly"); + } + + if (detect_errors(L"true && \n true")) { + err(L"newline after && reported as error"); + } + + if (detect_errors(L"true || \n") != PARSER_TEST_INCOMPLETE) { + err(L"unterminated conjunction not reported properly"); + } + say(L"Testing basic evaluation"); // Ensure that we don't crash on infinite self recursion and mutual recursion. These must use @@ -4344,7 +4356,7 @@ static void test_new_parser_correctness() { {L"true || false; and true", true}, {L"true || ||", false}, {L"|| true", false}, - {L"true || \n\n false", false}, + {L"true || \n\n false", true}, }; for (const auto &test : parser_tests) { diff --git a/src/parse_util.cpp b/src/parse_util.cpp index d2cea3634..ca6c53190 100644 --- a/src/parse_util.cpp +++ b/src/parse_util.cpp @@ -1215,6 +1215,10 @@ parser_test_error_bits_t parse_util_detect_errors(const ast::ast_t &ast, const w // detecting job_continuations that have source for pipes but not the statement. bool has_unclosed_pipe = false; + // Whether we encounter a missing job, i.e. a newline after && or ||. This is found by + // detecting job_conjunction_continuations that have source for && or || but not the job. + bool has_unclosed_conjunction = false; + // Expand all commands. // Verify 'or' and 'and' not used inside pipelines. // Verify pipes via parser_is_pipe_forbidden. @@ -1229,6 +1233,12 @@ parser_test_error_bits_t parse_util_detect_errors(const ast::ast_t &ast, const w if (!jc->pipe.unsourced && !jc->statement.try_source_range().has_value()) { has_unclosed_pipe = true; } + } else if (const auto *jcc = node.try_as()) { + // Somewhat clumsy way of checking for a job without source in a conjunction. + // See if our conjunction operator (&& or ||) has source but our job does not. + if (!jcc->conjunction.unsourced && !jcc->job.try_source_range().has_value()) { + has_unclosed_conjunction = true; + } } else if (const argument_t *arg = node.try_as()) { const wcstring &arg_src = arg->source(buff_src, &storage); res |= parse_util_detect_errors_in_argument(*arg, arg_src, out_errors); @@ -1262,7 +1272,8 @@ parser_test_error_bits_t parse_util_detect_errors(const ast::ast_t &ast, const w if (errored) res |= PARSER_TEST_ERROR; - if (has_unclosed_block || has_unclosed_pipe) res |= PARSER_TEST_INCOMPLETE; + if (has_unclosed_block || has_unclosed_pipe || has_unclosed_conjunction) + res |= PARSER_TEST_INCOMPLETE; return res; } diff --git a/tests/checks/andandoror.fish b/tests/checks/andandoror.fish index 4c4473019..b1987fa03 100644 --- a/tests/checks/andandoror.fish +++ b/tests/checks/andandoror.fish @@ -93,6 +93,24 @@ end # CHECK: 3 # CHECK: 4 +# Newlines +true && +echo newline after conjunction +# CHECK: newline after conjunction +false || +echo newline after disjunction +# CHECK: newline after disjunction + +true && + +echo empty lines after conjunction +# CHECK: empty lines after conjunction + +true && +# can have comments here! +echo comment after conjunction +# CHECK: comment after conjunction + # --help works builtin and --help >/dev/null echo $status diff --git a/tests/checks/indent.fish b/tests/checks/indent.fish index f6d1ff98d..2c9548219 100644 --- a/tests/checks/indent.fish +++ b/tests/checks/indent.fish @@ -298,9 +298,23 @@ printf '%s\n' "a; and b" | $fish_indent printf '%s\n' "a;" "and b" | $fish_indent #CHECK: a #CHECK: and b -printf '%s\n' "a" "; and b" | $fish_indent +printf '%s\n' a "; and b" | $fish_indent #CHECK: a #CHECK: and b printf '%s\n' "a; b" | $fish_indent #CHECK: a #CHECK: b + +echo 'foo && + # +bar' | $fish_indent +#CHECK: {{^}}foo && +#CHECK: {{^}}# +#CHECK: {{^}}bar + +echo 'command 1 | +command 1 cont || +command 2' | $fish_indent +#CHECK: {{^}}command 1 | +#CHECK: {{^ }}command 1 cont || +#CHECK: {{^}}command 2