fix: cleanup transient completion variables

brush-core/src/builtins/unset.rs

@@ -57,7 +57,7 @@ impl builtins::Command for UnsetCommand {
                     brush_parser::word::Parameter::Positional(_) => continue,
                     brush_parser::word::Parameter::Special(_) => continue,
                     brush_parser::word::Parameter::Named(name) => {
-                        context.shell.env.unset(name.as_str())?
+                        context.shell.env.unset(name.as_str())?.is_some()
                     }
                     brush_parser::word::Parameter::NamedWithIndex { name, index } => {
                         // First evaluate the index expression.

brush-core/src/completion.rs

@@ -10,7 +10,7 @@ use std::{
 use crate::{
     env, error, jobs, namedoptions, patterns,
     sys::{self, users},
-    trace_categories, traps,
+    trace_categories, traps, variables,
     variables::ShellValueLiteral,
     Shell,
 };
@@ -515,6 +515,7 @@ impl Spec {
             ("COMP_CWORD", context.token_index.to_string().into()),
         ];
 
+        let mut vars_to_remove = vec![];
         for (var, value) in vars_and_values {
             shell.env.update_or_add(
                 var,
@@ -523,6 +524,8 @@ impl Spec {
                 env::EnvironmentLookup::Anywhere,
                 env::EnvironmentScope::Global,
             )?;
+
+            vars_to_remove.push(var);
         }
 
         let mut args = vec![
@@ -543,27 +546,40 @@ impl Spec {
 
         tracing::debug!(target: trace_categories::COMPLETION, "[called completion func '{function_name}' => {result}]");
 
+        // Unset any of the temporary variables.
+        for var_name in vars_to_remove {
+            shell.env.unset(var_name)?;
+        }
+
         // When the function returns the special value 124, then it's a request
         // for us to restart the completion process.
         if result == 124 {
             Ok(Answer::RestartCompletionProcess)
         } else {
-            if let Some((_, reply)) = shell.env.get("COMPREPLY") {
+            if let Some(reply) = shell.env.unset("COMPREPLY")? {
                 tracing::debug!(target: trace_categories::COMPLETION, "[completion function yielded: {reply:?}]");
 
                 match reply.value() {
-                    crate::variables::ShellValue::IndexedArray(values) => Ok(Answer::Candidates(
-                        values.values().map(|v| v.to_owned()).collect(),
-                        ProcessingOptions::default(),
-                    )),
-                    _ => error::unimp("unexpected COMPREPLY value type"),
+                    variables::ShellValue::IndexedArray(values) => {
+                        return Ok(Answer::Candidates(
+                            values.values().map(|v| v.to_owned()).collect(),
+                            ProcessingOptions::default(),
+                        ));
+                    }
+                    variables::ShellValue::String(s) => {
+                        let mut candidates = IndexSet::new();
+                        candidates.insert(s.to_owned());
+
+                        return Ok(Answer::Candidates(candidates, ProcessingOptions::default()));
+                    }
+                    _ => (),
                 }
-            } else {
-                Ok(Answer::Candidates(
-                    IndexSet::new(),
-                    ProcessingOptions::default(),
-                ))
             }
+
+            Ok(Answer::Candidates(
+                IndexSet::new(),
+                ProcessingOptions::default(),
+            ))
         }
     }
 }

brush-core/src/env.rs

@@ -204,14 +204,16 @@ impl ShellEnvironment {
     /// # Arguments
     ///
     /// * `name` - The name of the variable to unset.
-    pub fn unset(&mut self, name: &str) -> Result<bool, error::Error> {
+    pub fn unset(&mut self, name: &str) -> Result<Option<ShellVariable>, error::Error> {
         let mut local_count = 0;
         for (scope_type, map) in self.scopes.iter_mut().rev() {
             if matches!(scope_type, EnvironmentScope::Local) {
                 local_count += 1;
             }
 
-            if Self::try_unset_in_map(map, name)? {
+            let unset_result = Self::try_unset_in_map(map, name)?;
+
+            if unset_result.is_some() {
                 // If we end up finding a local in the top-most local frame, then we replace
                 // it with a placeholder.
                 if matches!(scope_type, EnvironmentScope::Local) && local_count == 1 {
@@ -221,11 +223,11 @@ impl ShellEnvironment {
                     );
                 }
 
-                return Ok(true);
+                return Ok(unset_result);
             }
         }
 
-        Ok(false)
+        Ok(None)
     }
 
     /// Tries to unset an array element from the environment, using the given name and
@@ -243,11 +245,14 @@ impl ShellEnvironment {
         }
     }
 
-    fn try_unset_in_map(map: &mut ShellVariableMap, name: &str) -> Result<bool, error::Error> {
+    fn try_unset_in_map(
+        map: &mut ShellVariableMap,
+        name: &str,
+    ) -> Result<Option<ShellVariable>, error::Error> {
         match map.get(name).map(|v| v.is_readonly()) {
             Some(true) => Err(error::Error::ReadonlyVariable),
             Some(false) => Ok(map.unset(name)),
-            None => Ok(false),
+            None => Ok(None),
         }
     }
 
@@ -506,14 +511,14 @@ impl ShellVariableMap {
     // Setters
     //
 
-    /// Tries to unset the variable with the given name, returning whether or not
-    /// such a variable existed.
+    /// Tries to unset the variable with the given name, returning the removed
+    /// variable or None if it was not already set.
     ///
     /// # Arguments
     ///
     /// * `name` - The name of the variable to unset.
-    pub fn unset(&mut self, name: &str) -> bool {
-        self.variables.remove(name).is_some()
+    pub fn unset(&mut self, name: &str) -> Option<ShellVariable> {
+        self.variables.remove(name)
     }
 
     /// Sets a variable in the map.