Escape : and = in file completions

This is similar to f7dac82ed (Escape separators (colon and equals) to improve
completion, 2019-08-23) except we only escape : and = if they are the result of
file completions.  This way we avoid issues with custom completions like dd.
This also means that it won't work for things like __fish_complete_suffix
[*] but that can be fixed later, once we can serialize the DONT_ESCAPE flag.

By moving the escaping step earlier, this causes some unit test changes
which should not result in actual behavior change.

See also #6099

[*]: The new \: and \= does not leak from "complete -C" because that command
unescapes its output  -- unless --escape is given.

CHANGELOG.rst

@@ -99,6 +99,10 @@ Interactive improvements
 - Command-specific tab completions may now offer results whose first character is a period. For example, it is now possible to tab-complete ``git add`` for files with leading periods. The default file completions hide these files, unless the token itself has a leading period (:issue:`3707`).
 - Option completion now uses fuzzy subsequence filtering, as non-option completion does. This means that ``--fb`` may be completed to ``--foobar`` if there is no better match.
 - Completions that insert an entire token now use quotes instead of backslashes to escape special characters (:issue:`5433`).
+- File name completion usually starts at the last ``:``  or ``=`` within a token.
+  If these characters are actually part of the filename, they will be escaped as ``\:`` and ``\=``,
+  and no longer get this special treatment.
+  This matches Bash's behavior.
 - Autosuggestions were sometimes not shown after recalling a line from history, which has been fixed (:issue:`10287`).
 - Nonprintable ASCII control characters are now rendered using symbols from Unicode's Control Pictures block (:issue:`5274`).
 - Up-arrow search matches are no longer highlighted with low contrast.

src/common.rs

@@ -120,6 +120,8 @@ bitflags! {
         const NO_TILDE = 1 << 2;
         /// Replace non-printable control characters with Unicode symbols.
         const SYMBOLIC = 1 << 3;
+        /// Escape : and =
+        const SEPARATORS = 1 << 4;
     }
 }
 
@@ -180,6 +182,7 @@ pub fn escape_string(s: &wstr, style: EscapeStringStyle) -> WString {
 /// Escape a string in a fashion suitable for using in fish script.
 fn escape_string_script(input: &wstr, flags: EscapeFlags) -> WString {
     let escape_printables = !flags.contains(EscapeFlags::NO_PRINTABLES);
+    let escape_separators = flags.contains(EscapeFlags::SEPARATORS);
     let no_quoted = flags.contains(EscapeFlags::NO_QUOTED);
     let no_tilde = flags.contains(EscapeFlags::NO_TILDE);
     let no_qmark = feature_test(FeatureFlag::qmark_noglob);
@@ -290,6 +293,13 @@ fn escape_string_script(input: &wstr, flags: EscapeFlags) -> WString {
             ANY_STRING_RECURSIVE => {
                 out += L!("**");
             }
+            ':' | '=' => {
+                if escape_separators {
+                    need_escape = true;
+                    out.push('\\');
+                }
+                out.push(c);
+            }
 
             '&' | '$' | ' ' | '#' | '<' | '>' | '(' | ')' | '[' | ']' | '{' | '}' | '?' | '*'
             | '|' | ';' | '"' | '%' | '~' => {

src/complete.rs

@@ -10,7 +10,7 @@ use std::{
 };
 
 use crate::{
-    common::charptr2wcstring,
+    common::{charptr2wcstring, escape_string, EscapeFlags, EscapeStringStyle},
     reader::{get_quote, is_backslashed},
     util::wcsfilecmp,
 };
@@ -727,11 +727,12 @@ impl<'ctx> Completer<'ctx> {
 
         if cmd_tok.location_in_or_at_end_of_source_range(cursor_pos) {
             let equals_sign_pos = variable_assignment_equals_pos(current_token);
-            if equals_sign_pos.is_some() {
+            if let Some(pos) = equals_sign_pos {
                 self.complete_param_expand(
-                    current_token,
+                    &current_token[..pos + 1],
-                    true,  /* do_file */
+                    &current_token[pos + 1..],
-                    false, /* handle_as_special_cd */
+                    /*do_file=*/ true,
+                    /*handle_as_special_cd=*/ false,
                 );
                 return;
             }
@@ -831,10 +832,7 @@ impl<'ctx> Completer<'ctx> {
         }
 
         // This function wants the unescaped string.
-        self.complete_param_expand(current_argument, do_file, handle_as_special_cd);
+        self.complete_param_expand(L!(""), current_argument, do_file, handle_as_special_cd);
-
-        // Escape '[' in the argument before completing it.
-        self.escape_opening_brackets(current_argument);
 
         // Lastly mark any completions that appear to already be present in arguments.
         self.mark_completions_duplicating_arguments(&cmdline, current_token, tokens);
@@ -1501,7 +1499,13 @@ impl<'ctx> Completer<'ctx> {
     }
 
     /// Perform generic (not command-specific) expansions on the specified string.
-    fn complete_param_expand(&mut self, s: &wstr, do_file: bool, handle_as_special_cd: bool) {
+    fn complete_param_expand(
+        &mut self,
+        variable_override_prefix: &wstr,
+        s: &wstr,
+        do_file: bool,
+        handle_as_special_cd: bool,
+    ) {
         if self.ctx.check_cancel() {
             return;
         }
@@ -1534,7 +1538,8 @@ impl<'ctx> Completer<'ctx> {
         // foo=bar => expand the whole thing, and also just bar
         //
         // We also support colon separator (#2178). If there's more than one, prefer the last one.
-        let sep_index = if get_quote(s, s.len()).is_some() {
+        let quoted = get_quote(s, s.len()).is_some();
+        let sep_index = if quoted {
             None
         } else {
             let mut end = s.len();
@@ -1570,6 +1575,14 @@ impl<'ctx> Completer<'ctx> {
 
             // Any COMPLETE_REPLACES_TOKEN will also stomp the separator. We need to "repair" them by
             // inserting our separator and prefix.
+            Self::escape_opening_brackets(&mut local_completions, s);
+            Self::escape_separators(
+                &mut local_completions,
+                variable_override_prefix,
+                self.flags.autosuggestion,
+                true,
+                quoted,
+            );
             let prefix_with_sep = s.as_char_slice()[..sep_index + 1].into();
             for comp in &mut local_completions {
                 comp.prepend_token_prefix(prefix_with_sep);
@@ -1586,14 +1599,56 @@ impl<'ctx> Completer<'ctx> {
                 flags -= ExpandFlags::FUZZY_MATCH;
             }
 
+            let first = self.completions.len();
             if expand_to_receiver(s.to_owned(), &mut self.completions, flags, self.ctx, None).result
                 == ExpandResultCode::error
             {
                 FLOGF!(complete, "Error while expanding string '%ls'", s);
             }
+            Self::escape_opening_brackets(&mut self.completions[first..], s);
+            let have_token = !s.is_empty();
+            Self::escape_separators(
+                &mut self.completions[first..],
+                variable_override_prefix,
+                self.flags.autosuggestion,
+                have_token,
+                quoted,
+            );
         }
     }
 
+    fn escape_separators(
+        completions: &mut [Completion],
+        variable_override_prefix: &wstr,
+        append_only: bool,
+        have_token: bool,
+        is_quoted: bool,
+    ) {
+        for c in completions {
+            if is_quoted && !c.replaces_token() {
+                continue;
+            }
+            // clone of completion_apply_to_command_line
+            let add_space = !c.flags.contains(CompleteFlags::NO_SPACE);
+            let no_tilde = c.flags.contains(CompleteFlags::DONT_ESCAPE_TILDES);
+            let mut escape_flags = EscapeFlags::SEPARATORS;
+            if append_only || !add_space || (!c.replaces_token() && have_token) {
+                escape_flags.insert(EscapeFlags::NO_QUOTED);
+            }
+            if no_tilde {
+                escape_flags.insert(EscapeFlags::NO_TILDE);
+            }
+            if c.replaces_token() {
+                c.completion = variable_override_prefix.to_owned()
+                    + &escape_string(&c.completion, EscapeStringStyle::Script(escape_flags))[..];
+            } else {
+                c.completion =
+                    escape_string(&c.completion, EscapeStringStyle::Script(escape_flags));
+            }
+            assert!(!c.flags.contains(CompleteFlags::DONT_ESCAPE));
+            c.flags |= CompleteFlags::DONT_ESCAPE;
+        }
+    }
     /// Complete the specified string as an environment variable.
     /// Returns `true` if this was a variable, so we should stop completion.
     fn complete_variable(&mut self, s: &wstr, start_offset: usize) -> bool {
@@ -2008,7 +2063,7 @@ impl<'ctx> Completer<'ctx> {
     ///
     /// Check if there is any unescaped, unquoted '['; if yes, make the completions replace the entire
     /// argument instead of appending, so '[' will be escaped.
-    fn escape_opening_brackets(&mut self, argument: &wstr) {
+    fn escape_opening_brackets(completions: &mut [Completion], argument: &wstr) {
         let mut have_unquoted_unescaped_bracket = false;
         let mut quote = None;
         let mut escaped = false;
@@ -2040,7 +2095,7 @@ impl<'ctx> Completer<'ctx> {
         ) else {
             return;
         };
-        for comp in self.completions.get_list_mut() {
+        for comp in completions {
             if comp.flags.contains(CompleteFlags::REPLACES_TOKEN) {
                 continue;
             }

src/tests/complete.rs

@@ -95,8 +95,13 @@ fn test_complete() {
     std::fs::create_dir_all("test/complete_test").unwrap();
     std::fs::write("test/complete_test/has space", []).unwrap();
     std::fs::write("test/complete_test/bracket[abc]", []).unwrap();
-    #[cfg(not(windows))] // Square brackets are not legal path characters on WIN32/CYGWIN
     std::fs::write(r"test/complete_test/gnarlybracket\[abc]", []).unwrap();
+    #[cfg(not(windows))] // Square brackets are not legal path characters on WIN32/CYGWIN
+    {
+        std::fs::write(r"test/complete_test/gnarlybracket\[abc]", []).unwrap();
+        std::fs::write(r"test/complete_test/colon:abc", []).unwrap();
+    }
+    std::fs::write(r"test/complete_test/equal=abc", []).unwrap();
     std::fs::write("test/complete_test/testfile", []).unwrap();
     let testfile = CString::new("test/complete_test/testfile").unwrap();
     assert_eq!(unsafe { libc::chmod(testfile.as_ptr(), 0o700,) }, 0);
@@ -158,21 +163,45 @@ fn test_complete() {
     // Brackets - see #5831
     unique_completion_applies_as!(
         "touch test/complete_test/bracket[",
-        "test/complete_test/bracket[abc]",
+        r"'test/complete_test/bracket[abc]'",
         "touch 'test/complete_test/bracket[abc]' ",
     );
     unique_completion_applies_as!(
         "echo (ls test/complete_test/bracket[",
-        "test/complete_test/bracket[abc]",
+        r"'test/complete_test/bracket[abc]'",
         "echo (ls 'test/complete_test/bracket[abc]' ",
     );
     #[cfg(not(windows))] // Square brackets are not legal path characters on WIN32/CYGWIN
     {
         unique_completion_applies_as!(
             r"touch test/complete_test/gnarlybracket\\[",
-            r"test/complete_test/gnarlybracket\[abc]",
+            r"'test/complete_test/gnarlybracket\\[abc]'",
             r"touch 'test/complete_test/gnarlybracket\\[abc]' ",
         );
+        unique_completion_applies_as!(
+            r"a=test/complete_test/bracket[",
+            r"a='test/complete_test/bracket[abc]'",
+            r"a='test/complete_test/bracket[abc]' ",
+        );
+    }
+
+    #[cfg(not(windows))]
+    {
+        unique_completion_applies_as!(
+            r"touch test/complete_test/colon",
+            r"\:abc",
+            r"touch test/complete_test/colon\:abc ",
+        );
+        unique_completion_applies_as!(
+            r"touch test/complete_test/colon\:",
+            r"abc",
+            r"touch test/complete_test/colon\:abc ",
+        );
+        unique_completion_applies_as!(
+            r#"touch "test/complete_test/colon:"#,
+            r"abc",
+            r#"touch "test/complete_test/colon:abc" "#,
+        );
     }
 
     // #8820
@@ -427,8 +456,9 @@ fn test_autosuggest_suggest_special() {
     let _cleanup = test_init();
     macro_rules! perform_one_autosuggestion_cd_test {
         ($command:literal, $expected:literal, $vars:expr) => {
+            let command = L!($command);
             let mut comps = complete(
-                L!($command),
+                command,
                 CompletionRequestOptions::autosuggest(),
                 &OperationContext::background($vars, EXPANSION_LIMIT_BACKGROUND),
             )
@@ -440,7 +470,15 @@ fn test_autosuggest_suggest_special() {
             if !expects_error {
                 sort_and_prioritize(&mut comps, CompletionRequestOptions::default());
                 let suggestion = &comps[0];
-                assert_eq!(suggestion.completion, L!($expected));
+                let mut cursor = command.len();
+                let newcmdline = completion_apply_to_command_line(
+                    &suggestion.completion,
+                    suggestion.flags,
+                    command,
+                    &mut cursor,
+                    /*append_only=*/ true,
+                );
+                assert_eq!(newcmdline.strip_prefix(command).unwrap(), L!($expected));
             }
         };
     }
@@ -512,24 +550,24 @@ fn test_autosuggest_suggest_special() {
     perform_one_autosuggestion_cd_test!("cd test/autosuggest_test/0", "foobar/", &vars);
     perform_one_autosuggestion_cd_test!("cd \"test/autosuggest_test/0", "foobar/", &vars);
     perform_one_autosuggestion_cd_test!("cd 'test/autosuggest_test/0", "foobar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd test/autosuggest_test/1", "foo bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd test/autosuggest_test/1", r"foo\ bar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd \"test/autosuggest_test/1", "foo bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd \"test/autosuggest_test/1", r"foo bar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd 'test/autosuggest_test/1", "foo bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd 'test/autosuggest_test/1", r"foo bar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd test/autosuggest_test/2", "foo  bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd test/autosuggest_test/2", r"foo\ \ bar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd \"test/autosuggest_test/2", "foo  bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd \"test/autosuggest_test/2", r"foo  bar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd 'test/autosuggest_test/2", "foo  bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd 'test/autosuggest_test/2", r"foo  bar/", &vars);
     #[cfg(not(windows))]
     {
-        perform_one_autosuggestion_cd_test!("cd test/autosuggest_test/3", "foo\\bar/", &vars);
+        perform_one_autosuggestion_cd_test!("cd test/autosuggest_test/3", r"foo\\bar/", &vars);
-        perform_one_autosuggestion_cd_test!("cd \"test/autosuggest_test/3", "foo\\bar/", &vars);
+        perform_one_autosuggestion_cd_test!("cd \"test/autosuggest_test/3", r"foo\\bar/", &vars);
-        perform_one_autosuggestion_cd_test!("cd 'test/autosuggest_test/3", "foo\\bar/", &vars);
+        perform_one_autosuggestion_cd_test!("cd 'test/autosuggest_test/3", r"foo\\bar/", &vars);
     }
-    perform_one_autosuggestion_cd_test!("cd test/autosuggest_test/4", "foo'bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd test/autosuggest_test/4", r"foo\'bar/", &vars);
     perform_one_autosuggestion_cd_test!("cd \"test/autosuggest_test/4", "foo'bar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd 'test/autosuggest_test/4", "foo'bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd 'test/autosuggest_test/4", r"foo\'bar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd test/autosuggest_test/5", "foo\"bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd test/autosuggest_test/5", r#"foo\"bar/"#, &vars);
-    perform_one_autosuggestion_cd_test!("cd \"test/autosuggest_test/5", "foo\"bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd \"test/autosuggest_test/5", r#"foo\"bar/"#, &vars);
-    perform_one_autosuggestion_cd_test!("cd 'test/autosuggest_test/5", "foo\"bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd 'test/autosuggest_test/5", r#"foo"bar/"#, &vars);
 
     vars.parent
         .vars
@@ -549,24 +587,24 @@ fn test_autosuggest_suggest_special() {
     perform_one_autosuggestion_cd_test!("cd 0", "foobar/", &vars);
     perform_one_autosuggestion_cd_test!("cd \"0", "foobar/", &vars);
     perform_one_autosuggestion_cd_test!("cd '0", "foobar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd 1", "foo bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd 1", r"foo\ bar/", &vars);
     perform_one_autosuggestion_cd_test!("cd \"1", "foo bar/", &vars);
     perform_one_autosuggestion_cd_test!("cd '1", "foo bar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd 2", "foo  bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd 2", r"foo\ \ bar/", &vars);
     perform_one_autosuggestion_cd_test!("cd \"2", "foo  bar/", &vars);
     perform_one_autosuggestion_cd_test!("cd '2", "foo  bar/", &vars);
     #[cfg(not(windows))]
     {
-        perform_one_autosuggestion_cd_test!("cd 3", "foo\\bar/", &vars);
+        perform_one_autosuggestion_cd_test!("cd 3", r"foo\\bar/", &vars);
-        perform_one_autosuggestion_cd_test!("cd \"3", "foo\\bar/", &vars);
+        perform_one_autosuggestion_cd_test!("cd \"3", r"foo\\bar/", &vars);
-        perform_one_autosuggestion_cd_test!("cd '3", "foo\\bar/", &vars);
+        perform_one_autosuggestion_cd_test!("cd '3", r"foo\\bar/", &vars);
     }
-    perform_one_autosuggestion_cd_test!("cd 4", "foo'bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd 4", r"foo\'bar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd \"4", "foo'bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd \"4", r"foo'bar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd '4", "foo'bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd '4", r"foo\'bar/", &vars);
-    perform_one_autosuggestion_cd_test!("cd 5", "foo\"bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd 5", r#"foo\"bar/"#, &vars);
-    perform_one_autosuggestion_cd_test!("cd \"5", "foo\"bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd \"5", r#"foo\"bar/"#, &vars);
-    perform_one_autosuggestion_cd_test!("cd '5", "foo\"bar/", &vars);
+    perform_one_autosuggestion_cd_test!("cd '5", r#"foo"bar/"#, &vars);
 
     // A single quote should defeat tilde expansion.
     perform_one_autosuggestion_cd_test!("cd '~/test_autosuggest_suggest_specia'", "<error>", &vars);