Disallow background operator before && or ||

Co-authored-by: Johannes Altmanninger <aclopte@gmail.com>

Closes #10228
Fixes #9911

src/parse_util.rs

@@ -1,5 +1,5 @@
 //! Various mostly unrelated utility functions related to parsing, loading and evaluating fish code.
-use crate::ast::{self, Ast, Keyword, Leaf, List, Node, NodeVisitor};
+use crate::ast::{self, Ast, Keyword, Leaf, List, Node, NodeVisitor, Token};
 use crate::builtins::shared::builtin_exists;
 use crate::common::{
     escape_string, unescape_string, valid_var_name, valid_var_name_char, EscapeFlags,
@@ -29,7 +29,7 @@ use crate::wchar::prelude::*;
 use crate::wcstringutil::count_newlines;
 use crate::wcstringutil::truncate;
 use crate::wildcard::{ANY_CHAR, ANY_STRING, ANY_STRING_RECURSIVE};
-use std::ops;
+use std::{iter, ops};
 
 /// Handles slices: the square brackets in an expression like $foo[5..4]
 /// \return the length of the slice starting at \p in, or 0 if there is no slice, or None on error.
@@ -1114,6 +1114,8 @@ pub fn parse_util_detect_errors_in_ast(
             if jc.pipe.has_source() && jc.statement.try_source_range().is_none() {
                 has_unclosed_pipe = true;
             }
+        } else if let Some(job_conjunction) = node.as_job_conjunction() {
+            errored |= detect_errors_in_job_conjunction(job_conjunction, &mut out_errors);
         } else if let Some(jcc) = node.as_job_conjunction_continuation() {
             // 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.
@@ -1392,6 +1394,36 @@ pub fn parse_util_detect_errors_in_argument(
     }
 }
 
+fn detect_errors_in_job_conjunction(
+    job_conjunction: &ast::JobConjunction,
+    parse_errors: &mut Option<&mut ParseErrorList>,
+) -> bool {
+    // Disallow background immediately before conjunction continuations. For example:
+    // foo & && bar
+    // foo & || baz
+    let continuations = &job_conjunction.continuations;
+    let jobs = iter::once(&job_conjunction.job)
+        .chain(continuations.iter().map(|continuation| &continuation.job));
+    for (job, continuation) in jobs.zip(continuations.iter()) {
+        if job.bg.is_some() {
+            let conjunction = &continuation.conjunction;
+            return append_syntax_error!(
+                parse_errors,
+                conjunction.source_range().start(),
+                conjunction.source_range().length(),
+                BOOL_AFTER_BACKGROUND_ERROR_MSG,
+                if conjunction.token_type() == ParseTokenType::andand {
+                    L!("&&")
+                } else {
+                    L!("||")
+                }
+            );
+        }
+    }
+
+    false
+}
+
 /// Given that the job given by node should be backgrounded, return true if we detect any errors.
 fn detect_errors_in_backgrounded_job(
     job: &ast::JobPipeline,
@@ -1778,7 +1810,7 @@ pub fn parse_util_expand_variable_error(
 }
 
 /// Error message for use of backgrounded commands before and/or.
-const BOOL_AFTER_BACKGROUND_ERROR_MSG: &str =
+pub(crate) const BOOL_AFTER_BACKGROUND_ERROR_MSG: &str =
     "The '%ls' command can not be used immediately after a backgrounded job";
 
 /// Error message for backgrounded commands as conditionals.

src/tests/parse_util.rs

@@ -5,7 +5,7 @@ use crate::parse_constants::{
     ERROR_NOT_ARGV_AT, ERROR_NOT_ARGV_COUNT, ERROR_NOT_ARGV_STAR, ERROR_NOT_PID, ERROR_NOT_STATUS,
     ERROR_NO_VAR_NAME,
 };
-use crate::parse_util::parse_util_detect_errors;
+use crate::parse_util::{parse_util_detect_errors, BOOL_AFTER_BACKGROUND_ERROR_MSG};
 use crate::tests::prelude::*;
 use crate::wchar::prelude::*;
 use crate::wchar_ext::WExt;
@@ -22,10 +22,15 @@ fn test_error_messages() {
         let mut offset = 0;
         for mtch in format_specifier_regex.find_iter(format.as_char_slice()) {
             let mtch = mtch.unwrap();
-            result.push(&format[offset..mtch.start()]);
+            let component = &format[offset..mtch.start()];
+            result.push(component);
             offset = mtch.end();
         }
         result.push(&format[offset..]);
+        // Avoid mismatch from localized  quotes.
+        for component in &mut result {
+            *component = component.trim_matches('\'');
+        }
         result
     }
 
@@ -74,4 +79,9 @@ fn test_error_messages() {
     validate!("echo foo\"$\"bar", ERROR_NO_VAR_NAME);
     validate!("echo \"foo\"$\"bar\"", ERROR_NO_VAR_NAME);
     validate!("echo foo $ bar", ERROR_NO_VAR_NAME);
+    validate!("echo 1 & && echo 2", BOOL_AFTER_BACKGROUND_ERROR_MSG);
+    validate!(
+        "echo 1 && echo 2 & && echo 3",
+        BOOL_AFTER_BACKGROUND_ERROR_MSG
+    );
 }

src/wchar_ext.rs

@@ -3,6 +3,7 @@ use std::{iter, slice};
 use crate::{
     common::subslice_position,
     wchar::{wstr, WString},
+    L,
 };
 use widestring::utfstr::CharsUtf32;
 
@@ -284,6 +285,17 @@ pub trait WExt {
             self.as_char_slice().iter().copied().rev(),
         )
     }
+
+    fn trim_matches(&self, pat: char) -> &wstr {
+        let slice = self.as_char_slice();
+        let leading_count = slice.chars().take_while(|&c| c == pat).count();
+        let trailing_count = slice.chars().rev().take_while(|&c| c == pat).count();
+        if leading_count == slice.len() {
+            return L!("");
+        }
+        let slice = self.slice_from(leading_count);
+        slice.slice_to(slice.len() - trailing_count)
+    }
 }
 
 impl WExt for WString {
@@ -396,4 +408,11 @@ mod tests {
         let needle = L!("hello world");
         assert_eq!(haystack.find(needle), None);
     }
+
+    #[test]
+    fn test_trim_matches() {
+        assert_eq!(L!("|foo|").trim_matches('|'), L!("foo"));
+        assert_eq!(L!("<foo|").trim_matches('|'), L!("<foo"));
+        assert_eq!(L!("|foo>").trim_matches('|'), L!("foo>"));
+    }
 }