Deprecate builtin test's one- and zero-argument modes (#10365)

This introduces a feature flag, "test-require-arg", that removes builtin test's zero and one argument special modes.

That means:

- `test -n` returns false
- `test -z` returns true
- `test -x` with any other option errors out with "missing argument"
- `test foo` errors out as expecting an option

`test -n` returning true is a frequent source of confusion, and so we are breaking with posix in this regard.

As always the flag defaults to off and can be turned on. In future it will default to on and then eventually be made read-only.

There is a new FLOG category "deprecated-test", run `fish -d deprecated-test` and it will show any test call that would change in future.

doc_src/cmds/test.rst

@@ -25,7 +25,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
 -----------------------------------
@@ -184,6 +192,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.
 
 ::
@@ -233,6 +243,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)