From 2c17d3497117cddd4255d0ce4221265330160f6c Mon Sep 17 00:00:00 2001 From: Fabian Boehm Date: Sun, 21 Apr 2024 14:25:54 +0200 Subject: [PATCH] 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 | 13 +++++++++- doc_src/language.rst | 2 ++ src/builtins/test.rs | 42 +++++++++++++++++++++++++++--- src/flog.rs | 2 ++ src/future_feature_flags.rs | 12 +++++++++ tests/checks/status.fish | 1 + tests/checks/test-posix.fish | 48 ++++++++++++++++++++++++++++++++++ tests/checks/test.fish | 50 +++++++++++++++++++++++++++++++++++- 8 files changed, 164 insertions(+), 6 deletions(-) create mode 100644 tests/checks/test-posix.fish diff --git a/doc_src/cmds/test.rst b/doc_src/cmds/test.rst index 1194d1b5de44..84721375a7fd 100644 --- a/doc_src/cmds/test.rst +++ b/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 `, 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 ` - 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 `__. 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 9fbd30768054..9683e3236601 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)