diff --git a/doc_src/cmds/test.rst b/doc_src/cmds/test.rst index 506f863233ed..1d4111122c16 100644 --- a/doc_src/cmds/test.rst +++ b/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 `, 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 ` - 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 `__. 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``. diff --git a/doc_src/language.rst b/doc_src/language.rst index 7228a49ed8a1..243a57c426da 100644 --- a/doc_src/language.rst +++ b/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 `'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 ` 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:: diff --git a/src/builtins/test.rs b/src/builtins/test.rs index f180ca58b1c9..e18235843f2c 100644 --- a/src/builtins/test.rs +++ b/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> { - // 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 diff --git a/src/flog.rs b/src/flog.rs index af3763ba5e56..fed77dddd3c0 100644 --- a/src/flog.rs +++ b/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"); diff --git a/src/future_feature_flags.rs b/src/future_feature_flags.rs index 776bf81c8068..429baa8c969c 100644 --- a/src/future_feature_flags.rs +++ b/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), ], } } diff --git a/tests/checks/status.fish b/tests/checks/status.fish index 733dd22604cd..0a1089fae9f4 100644 --- a/tests/checks/status.fish +++ b/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 diff --git a/tests/checks/test-posix.fish b/tests/checks/test-posix.fish new file mode 100644 index 000000000000..c9fcd74c6d90 --- /dev/null +++ b/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: ^ + diff --git a/tests/checks/test.fish b/tests/checks/test.fish index 5a1cb09b4feb..036b8b025d2c 100644 --- a/tests/checks/test.fish +++ b/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)