fix(builtins): correct more 'set' argument parsing (#356)

Fixes handling of '+' options in set builtin.

README.md

@@ -54,17 +54,17 @@ installed on your system, then you can also author a `~/.brushrc` file.
 
 There are some known gaps in compatibility. Most notably:
 
-* **Honoring the full semantics of all `set` and `shopt` options.**
-  The `set` builtin is implemented, as is `set -x` and many frequently used options, but a number of options aren't fully implemented. `set -e`, for example, will execute but its semantics aren't applied across execution.
+* **Some `set` and `shopt` options.**
+  The `set` builtin is implemented, as is `set -x` and many frequently used `set`/`shopt` options, but a number aren't fully implemented. For example, `set -e` will execute but its semantics aren't applied across execution.
 
 * **Anything tagged with a `TODO` comment or where `error::unimp()` is used to return a "not implemented" error**.
   These aren't all tracked with GitHub issues right now, but there's a number of these scattered throughout the code base. Some are indicative of missing functionality that may be straightforward to implement; others may be more complicated.
 
-If you feel so inclined, we'd love contributions toward any of the above, with broadening test coverage, deeper compatibility evaluation, or really any other opportunities you can find to help make this project better.
+If you feel so inclined, we'd love contributions toward any of the above, with broadening test coverage, deeper compatibility evaluation, or really any other opportunities you can find to help us make this project better.
 
 ## Testing strategy
 
-This project is primarily tested by comparing its behavior with other existing shells, leveraging the latter as test oracles. The integration tests implemented in this repo include [515+ test cases](brush-shell/tests/cases) run on both this shell and an oracle, comparing standard output and exit codes.
+This project is primarily tested by comparing its behavior with other existing shells, leveraging the latter as test oracles. The integration tests implemented in this repo include [525+ test cases](brush-shell/tests/cases) run on both this shell and an oracle, comparing standard output and exit codes.
 
 For more details, please consult the [reference documentation on integration testing](docs/reference/integration-testing.md).
 

brush-core/src/arithmetic.rs

@@ -19,8 +19,8 @@ pub enum EvalError {
     FailedToTokenizeExpression,
 
     /// Failed to expand an arithmetic expression.
-    #[error("failed to expand expression")]
-    FailedToExpandExpression,
+    #[error("failed to expand expression: '{0}'")]
+    FailedToExpandExpression(String),
 
     /// Failed to access an element of an array.
     #[error("failed to access array")]
@@ -71,7 +71,7 @@ pub(crate) async fn expand_and_eval(
     // Per documentation, first shell-expand it.
     let expanded_self = expansion::basic_expand_str_without_tilde(shell, expr)
         .await
-        .map_err(|_e| EvalError::FailedToExpandExpression)?;
+        .map_err(|_e| EvalError::FailedToExpandExpression(expr.to_owned()))?;
 
     // Now parse.
     let expr = brush_parser::arithmetic::parse(&expanded_self)

brush-core/src/builtins.rs

@@ -191,15 +191,18 @@ pub trait Command: Parser {
         } else {
             // N.B. clap doesn't support named options like '+x'. To work around this, we
             // establish a pattern of renaming them.
-            let args = args.into_iter().map(|arg| {
-                if arg.starts_with('+') {
-                    format!("--{arg}")
+            let mut updated_args = vec![];
+            for arg in args {
+                if let Some(plus_options) = arg.strip_prefix("+") {
+                    for c in plus_options.chars() {
+                        updated_args.push(format!("--+{c}"));
+                    }
                 } else {
-                    arg
+                    updated_args.push(arg);
                 }
-            });
+            }
 
-            Self::try_parse_from(args)
+            Self::try_parse_from(updated_args)
         }
     }
 

brush-core/src/builtins/set.rs

@@ -150,17 +150,35 @@ impl builtins::Command for SetCommand {
         // leaking into too many commands' custom parsing.
         //
 
-        // Apply the same workaround from the default implementation of Command::new to handle +
+        // Apply the same workaround from the default implementation of Command::new to handle '+'
         // args.
-        let args = args.into_iter().map(|arg| {
-            if arg.starts_with('+') {
-                format!("--{arg}")
+        let mut updated_args = vec![];
+        let mut now_parsing_positional_args = false;
+        let mut next_arg_is_option_value = false;
+        for (i, arg) in args.into_iter().enumerate() {
+            if now_parsing_positional_args || next_arg_is_option_value {
+                updated_args.push(arg);
+
+                next_arg_is_option_value = false;
+                continue;
+            }
+
+            if arg == "-" || arg == "--" || (i > 0 && !arg.starts_with(['-', '+'])) {
+                now_parsing_positional_args = true;
+            }
+
+            if let Some(plus_options) = arg.strip_prefix("+") {
+                next_arg_is_option_value = plus_options.ends_with('o');
+                for c in plus_options.chars() {
+                    updated_args.push(format!("--+{c}"));
+                }
             } else {
-                arg
+                next_arg_is_option_value = arg.starts_with('-') && arg.ends_with('o');
+                updated_args.push(arg);
             }
-        });
+        }
 
-        let (mut this, rest_args) = crate::builtins::try_parse_known::<SetCommand>(args)?;
+        let (mut this, rest_args) = crate::builtins::try_parse_known::<SetCommand>(updated_args)?;
         if let Some(args) = rest_args {
             this.positional_args.extend(args);
         }

brush-core/src/builtins/unset.rs

@@ -48,6 +48,7 @@ impl builtins::Command for UnsetCommand {
 
         let unspecified = self.name_interpretation.unspecified();
 
+        #[allow(clippy::needless_continue)]
         for name in &self.names {
             if unspecified || self.name_interpretation.shell_variables {
                 let parameter =

brush-core/src/jobs.rs

@@ -427,7 +427,7 @@ impl Job {
                         return Some(pid);
                     }
                 }
-                JobTask::Internal(_) => continue,
+                JobTask::Internal(_) => (),
             }
         }
         None

brush-shell/tests/cases/builtins/set.yaml

@@ -14,6 +14,40 @@ cases:
       set a b c d
       echo ${*}
 
+  - name: "set with options"
+    stdin: |
+      function dumpopts {
+          # Dump the options
+          echo "[Options: $1]"
+          echo "set options: " $-
+          shopt -p -o pipefail
+      }
+
+      set -e -u -o pipefail
+      dumpopts enabled
+      echo '*: ' $*
+
+      set +e +u +o pipefail
+      dumpopts disabled
+      echo '*: ' $*
+
+  - name: "set with multiple combined options"
+    stdin: |
+      function dumpopts {
+          # Dump the options
+          echo "[Options: $1]"
+          echo "set options: " $-
+          shopt -p -o pipefail
+      }
+
+      set -euo pipefail
+      dumpopts enabled
+      echo '$*: ' $*
+
+      set +euo pipefail
+      dumpopts disabled
+      echo '$*: ' $*
+
   - name: "set clearing args"
     stdin: |
       set a b c
@@ -35,7 +69,22 @@ cases:
       set --
       echo "args: " ${*}
 
-  - name: "set with option-looking tokens"
+  - name: "set with option-looking args"
     stdin: |
+      set -- a -v
+      echo ${*}
+
+      set - a -v
+      echo ${*}
+
       set a -v
       echo ${*}
+
+      set -- a +x
+      echo ${*}
+
+      set - a +x
+      echo ${*}
+
+      set a +x
+      echo ${*}