Deprecate builtin test's one- and zero-argument modes

doc_src/cmds/test.rst

@@ -26,7 +26,15 @@ The first form (``test``) is preferred. For compatibility with other shells, the
 
 When using a variable or command substitution as an argument with ``test`` you should almost always enclose it in double-quotes, as variables expanding to zero or more than one argument will most likely interact badly with ``test``.
 
-For historical reasons, ``test`` supports the one-argument form (``test foo``), and this will also be triggered by e.g. ``test -n $foo`` if $foo is unset. We recommend you don't use the one-argument form and quote all variables or command substitutions used with ``test``.
+.. warning::
+
+          For historical reasons, ``test`` supports the one-argument form (``test foo``), and this will also be triggered by e.g. ``test -n $foo`` if $foo is unset. We recommend you don't use the one-argument form and quote all variables or command substitutions used with ``test``.
+
+          This confusing misfeature will be removed in future. ``test -n`` without any additional argument will be false, ``test -z`` will be true and any other invocation with exactly one or zero arguments, including ``test -d`` and ``test "foo"`` will be an error.
+
+          The same goes for ``[``, e.g. ``[ "foo" ]`` and ``[ -d ]`` will be errors.
+
+          This can be turned on already via the ``test-require-arg`` :ref:`feature flag <featureflags>`, and will eventually become the default and then only option.
 
 Operators for files and directories
 -----------------------------------
@@ -185,6 +193,8 @@ Be careful with unquoted variables::
         echo $MANPATH
     end
 
+This will change in a future release of fish, or already with the ``test-require-arg`` :ref:`feature flag <featureflags>` - if $MANPATH is unset, ``if test -n $MANPATH`` will be false.
+
 Parentheses and the ``-o`` and ``-a`` operators can be combined to produce more complicated expressions. In this example, success is printed if there is a ``/foo`` or ``/bar`` file as well as a ``/baz`` or ``/bat`` file.
 
 ::
@@ -234,6 +244,7 @@ Standards
 Unlike many things in fish, ``test`` implements a subset of the `IEEE Std 1003.1-2008 (POSIX.1) standard <https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html>`__. The following exceptions apply:
 
 - The ``<`` and ``>`` operators for comparing strings are not implemented.
+- With ``test-require-arg``, the zero- and one-argument modes will behave differently.
 
  In cases such as this, one can use ``command`` ``test`` to explicitly use the system's standalone ``test`` rather than this ``builtin`` ``test``.
 

doc_src/language.rst

@@ -2010,6 +2010,7 @@ You can see the current list of features via ``status features``::
     regex-easyesc           on  3.1 string replace -r needs fewer \\'s
     ampersand-nobg-in-token on  3.4 & only backgrounds if followed by a separating character
     remove-percent-self     off 3.8 %self is no longer expanded (use $fish_pid)
+    test-require-arg        off 3.8 builtin test requires an argument
 
 Here is what they mean:
 
@@ -2018,6 +2019,7 @@ Here is what they mean:
 - ``regex-easyesc`` was introduced in 3.1. It makes it so the replacement expression in ``string replace -r`` does one fewer round of escaping. Before, to escape a backslash you would have to use ``string replace -ra '([ab])' '\\\\\\\\$1'``. After, just ``'\\\\$1'`` is enough. Check your ``string replace`` calls if you use this anywhere.
 - ``ampersand-nobg-in-token`` was introduced in fish 3.4. It makes it so a ``&`` i no longer interpreted as the backgrounding operator in the middle of a token, so dealing with URLs becomes easier. Either put spaces or a semicolon after the ``&``. This is recommended formatting anyway, and ``fish_indent`` will have done it for you already.
 - ``remove-percent-self`` turns off the special ``%self`` expansion. It was introduced in 3.8. To get fish's pid, you can use the :envvar:`fish_pid` variable.
+- ``test-require-arg`` removes :doc:`builtin test <cmds/test>`'s one-argument form (``test "string"``. It was introduced in 3.8. To test if a string is non-empty, use ``test -n "string"``. If disabled, any call to ``test`` that would change sends a :ref:`debug message <debugging-fish>` of category "deprecated-test", so starting fish with ``fish --debug=deprecated-test`` can be used to find offending calls.
 
 
 These changes are introduced off by default. They can be enabled on a per session basis::

src/builtins/test.rs

@@ -1,5 +1,7 @@
 use super::prelude::*;
 use crate::common;
+use crate::future_feature_flags::{feature_test, FeatureFlag};
+use crate::should_flog;
 
 mod test_expressions {
     use super::*;
@@ -552,6 +554,10 @@ mod test_expressions {
                 );
             }
 
+            if feature_test(FeatureFlag::test_require_arg) {
+                return self.error(start, sprintf!("Unknown option at index %u", start));
+            }
+
             return JustAString {
                 arg: self.arg(start).to_owned(),
                 range: start..start + 1,
@@ -718,9 +724,6 @@ mod test_expressions {
             err: &mut WString,
             program_name: &wstr,
         ) -> Option<Box<dyn Expression>> {
-            // Empty list and one-arg list should be handled by caller.
-            assert!(args.len() > 1);
-
             let mut parser = TestParser {
                 strings: args,
                 errors: Vec::new(),
@@ -1019,9 +1022,40 @@ pub fn test(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Opt
         .collect();
     let args: &[WString] = &args;
 
-    if argc == 0 {
+    if feature_test(FeatureFlag::test_require_arg) {
+        if argc == 0 {
+            streams.err.appendln(wgettext_fmt!(
+                "%ls: Expected at least one argument",
+                program_name
+            ));
+            builtin_print_error_trailer(parser, streams.err, program_name);
+            return STATUS_INVALID_ARGS;
+        } else if argc == 1 {
+            if args[0] == "-n" {
+                return STATUS_CMD_ERROR;
+            } else if args[0] == "-z" {
+                return STATUS_CMD_OK;
+            }
+        }
+    } else if argc == 0 {
+        if should_flog!(deprecated_test) {
+            streams.err.appendln(wgettext_fmt!(
+                "%ls: called with no arguments. This will be an error in future.",
+                program_name
+            ));
+            streams.err.append(parser.current_line());
+        }
         return STATUS_INVALID_ARGS; // Per 1003.1, exit false.
     } else if argc == 1 {
+        if should_flog!(deprecated_test) {
+            if args[0] != "-z" {
+                streams.err.appendln(wgettext_fmt!(
+                    "%ls: called with one argument. This will return false in future.",
+                    program_name
+                ));
+                streams.err.append(parser.current_line());
+            }
+        }
         // Per 1003.1, exit true if the arg is non-empty.
         return if args[0].is_empty() {
             STATUS_CMD_ERROR

src/flog.rs

@@ -79,6 +79,8 @@ pub mod categories {
 
         (warning_path, "warning-path", "Warnings about unusable paths for config/history (on by default)", true);
 
+        (deprecated_test, "deprecated-test", "Warning about using test's zero- or one-argument modes (`test -d $foo`), which will be changed in future.");
+
         (config, "config", "Finding and reading configuration");
 
         (event, "event", "Firing events");

src/future_feature_flags.rs

@@ -24,6 +24,9 @@ pub enum FeatureFlag {
     ampersand_nobg_in_token,
     /// Whether "%self" is expanded to fish's pid
     remove_percent_self,
+
+    /// Remove `test`'s one and zero arg mode (make `test -n` return false etc)
+    test_require_arg,
 }
 
 struct Features {
@@ -96,6 +99,14 @@ pub const METADATA: &[FeatureMetadata] = &[
         default_value: false,
         read_only: false,
     },
+    FeatureMetadata {
+        flag: FeatureFlag::test_require_arg,
+        name: L!("test-require-arg"),
+        groups: L!("3.8"),
+        description: L!("builtin test requires an argument"),
+        default_value: false,
+        read_only: false,
+    },
 ];
 
 thread_local!(
@@ -156,6 +167,7 @@ impl Features {
                 AtomicBool::new(METADATA[2].default_value),
                 AtomicBool::new(METADATA[3].default_value),
                 AtomicBool::new(METADATA[4].default_value),
+                AtomicBool::new(METADATA[5].default_value),
             ],
         }
     }

tests/checks/status.fish

@@ -58,6 +58,7 @@ status features
 #CHECK: regex-easyesc           on  3.1 string replace -r needs fewer \'s
 #CHECK: ampersand-nobg-in-token on  3.4 & only backgrounds if followed by a separator
 #CHECK: remove-percent-self     off 3.8 %self is no longer expanded (use $fish_pid)
+#CHECK: test-require-arg        off 3.8 builtin test requires an argument
 status test-feature stderr-nocaret
 echo $status
 #CHECK: 0

tests/checks/test-posix.fish

@@ -0,0 +1,48 @@
+# RUN: %fish %s
+#
+# Tests for the posix-mandated zero and one argument modes for the `test` builtin, aka `[`.
+
+test -n
+echo $status
+#CHECK: 0
+
+test -z
+echo $status
+#CHECK: 0
+
+test -d
+echo $status
+#CHECK: 0
+
+test "foo"
+echo $status
+#CHECK: 0
+
+test ""
+echo $status
+#CHECK: 1
+
+test -z "" -a foo
+echo $status
+#CHECK: 0
+
+set -l fish (status fish-path)
+echo 'test foo; test; test -z; test -n; test -d; echo oops' | $fish -d 'deprecated-*' >/dev/null
+#CHECKERR: test: called with one argument. This will return false in future.
+#CHECKERR: Standard input (line 1):
+#CHECKERR: test foo; test; test -z; test -n; test -d; echo oops
+#CHECKERR: ^
+#CHECKERR: test: called with no arguments. This will be an error in future.
+#CHECKERR: Standard input (line 1):
+#CHECKERR: test foo; test; test -z; test -n; test -d; echo oops
+#CHECKERR:           ^
+#CHECKERR: test: called with one argument. This will return false in future.
+# (yes, `test -z` is skipped because it would behave the same)
+#CHECKERR: Standard input (line 1):
+#CHECKERR: test foo; test; test -z; test -n; test -d; echo oops
+#CHECKERR:                          ^
+#CHECKERR: test: called with one argument. This will return false in future.
+#CHECKERR: Standard input (line 1):
+#CHECKERR: test foo; test; test -z; test -n; test -d; echo oops
+#CHECKERR: ^
+

tests/checks/test.fish

@@ -1,4 +1,4 @@
-# RUN: %fish %s
+# RUN: %fish --features test-require-arg %s
 #
 # Tests for the `test` builtin, aka `[`.
 test inf -gt 0
@@ -94,3 +94,51 @@ test epoch -ef old && echo bad ef || echo good ef
 #CHECK: good ef
 
 rm -f epoch old newest epochlink
+
+test -n
+echo -- -n $status
+#CHECK: -n 1
+test -z
+echo -- -z $status
+#CHECK: -z 0
+
+test -d
+#CHECKERR: test: Missing argument at index 2
+#CHECKERR: -d
+#CHECKERR: ^
+#CHECKERR: {{.*}}test.fish (line {{\d+}}): 
+#CHECKERR: test -d
+#CHECKERR: ^
+
+test "foo"
+#CHECKERR: test: Missing argument at index 2
+#CHECKERR: foo
+#CHECKERR: ^
+#CHECKERR: {{.*}}test.fish (line {{\d+}}): 
+#CHECKERR: test "foo"
+#CHECKERR: ^
+
+test ""
+#CHECKERR: test: Missing argument at index 2
+#CHECKERR: ^
+#CHECKERR: {{.*}}test.fish (line {{\d+}}): 
+#CHECKERR: test ""
+#CHECKERR: ^
+
+test -z "" -a foo
+#CHECKERR: test: Missing argument at index 5
+#CHECKERR: -z  -a foo
+#CHECKERR: ^
+#CHECKERR: {{.*}}test.fish (line {{\d+}}): 
+#CHECKERR: test -z "" -a foo
+#CHECKERR: ^
+
+echo $status
+#CHECK: 1
+
+test
+#CHECKERR: test: Expected at least one argument
+#CHECKERR: {{.*}}test.fish (line {{\d+}}): 
+#CHECKERR: test
+#CHECKERR: ^
+#CHECKERR: (Type 'help test' for related documentation)