Repository: reubeno/brush (rust)

Issue:
set -- mistakenly outputs shell variables and shell functions
`set --` must clear positional parameters.

```console
$ brush --version
brush 0.2.14 (git:dd1d39c)

$ brush -c 'set --'
BASH=brush
BASH_VERSINFO=([0]="5" [1]="2" [2]="15" [3]="1" [4]="release" [5]="unknown")
BASH_VERSION=5.2.15(1)-release
COMP_WORDBREAKS='
  ︙

$ bash -c 'set --'
(nothing)
```

Pull request:
fix(builtins): correct more 'set' argument parsing
@ko1nksm noticed that #354 left behind some incorrect behavior with positional arguments to the `set` builtin that had `+` as a prefix. This PR mitigates that issue while leaving the original issue in #339 covered.

Unfortunately, this is getting us more into hand-parsing arguments for the `set` builtin, but it appears unavoidable for now. The right long-term option still seems to be working with the `clap-rs` community to properly support customizable semantics for `--`, support for `+` short-name options, etc.

Resolves #339.

Patch:
diff --git a/README.md b/README.md
index 1c22b1c..0ffa905 100644
--- a/README.md
+++ b/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).
 
diff --git a/brush-core/src/arithmetic.rs b/brush-core/src/arithmetic.rs
index c13257a..6d041cc 100644
--- a/brush-core/src/arithmetic.rs
+++ b/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)
diff --git a/brush-core/src/builtins.rs b/brush-core/src/builtins.rs
index a5c4433..fcec847 100644
--- a/brush-core/src/builtins.rs
+++ b/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)
         }
     }
 
diff --git a/brush-core/src/builtins/set.rs b/brush-core/src/builtins/set.rs
index 5aa76ce..f487e27 100644
--- a/brush-core/src/builtins/set.rs
+++ b/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);
         }
diff --git a/brush-core/src/builtins/unset.rs b/brush-core/src/builtins/unset.rs
index 4866404..52ee85a 100644
--- a/brush-core/src/builtins/unset.rs
+++ b/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 =
diff --git a/brush-core/src/jobs.rs b/brush-core/src/jobs.rs
index b997bcd..da91b46 100644
--- a/brush-core/src/jobs.rs
+++ b/brush-core/src/jobs.rs
@@ -427,7 +427,7 @@ impl Job {
                         return Some(pid);
                     }
                 }
-                JobTask::Internal(_) => continue,
+                JobTask::Internal(_) => (),
             }
         }
         None


Test patch:
diff --git a/brush-shell/tests/cases/builtins/set.yaml b/brush-shell/tests/cases/builtins/set.yaml
index 9668b6f..2a6d38b 100644
--- a/brush-shell/tests/cases/builtins/set.yaml
+++ b/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 ${*}