From 91a16bb0e853aa5b81e16ba5cd698ac2ba606436 Mon Sep 17 00:00:00 2001 From: Fabian Boehm Date: Fri, 12 Apr 2024 15:30:36 +0200 Subject: [PATCH 1/3] Introduce test-require-arg feature flag This 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 --- src/builtins/test.rs | 43 ++++++++++++++++++++++--------- src/future_feature_flags.rs | 12 +++++++++ tests/checks/status.fish | 1 + tests/checks/test-posix.fish | 27 +++++++++++++++++++ tests/checks/test.fish | 50 +++++++++++++++++++++++++++++++++++- 5 files changed, 120 insertions(+), 13 deletions(-) create mode 100644 tests/checks/test-posix.fish diff --git a/src/builtins/test.rs b/src/builtins/test.rs index f180ca58b1c9..6b0d546afcaa 100644 --- a/src/builtins/test.rs +++ b/src/builtins/test.rs @@ -1,5 +1,6 @@ use super::prelude::*; use crate::common; +use crate::future_feature_flags::{feature_test, FeatureFlag}; mod test_expressions { use super::*; @@ -552,6 +553,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 +723,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,15 +1021,32 @@ pub fn test(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Opt .collect(); let args: &[WString] = &args; - if argc == 0 { - return STATUS_INVALID_ARGS; // Per 1003.1, exit false. - } else if argc == 1 { - // Per 1003.1, exit true if the arg is non-empty. - return if args[0].is_empty() { - STATUS_CMD_ERROR - } else { - STATUS_CMD_OK - }; + 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 { + return STATUS_INVALID_ARGS; // Per 1003.1, exit false. + } else if argc == 1 { + // Per 1003.1, exit true if the arg is non-empty. + return if args[0].is_empty() { + STATUS_CMD_ERROR + } else { + STATUS_CMD_OK + }; + } } // Try parsing 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..9852e266e97d --- /dev/null +++ b/tests/checks/test-posix.fish @@ -0,0 +1,27 @@ +# 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 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) From 3dbffccd875c04f9aa10a2d5c3e374cbd7b93215 Mon Sep 17 00:00:00 2001 From: Fabian Boehm Date: Sat, 23 Mar 2024 12:15:30 +0100 Subject: [PATCH 2/3] Add deprecation warning FLOG category "deprecated-test", run `fish -d deprecated-test` and it will show any test call that would change in future. --- src/builtins/test.rs | 35 +++++++++++++++++++++++++---------- src/flog.rs | 2 ++ tests/checks/test-posix.fish | 21 +++++++++++++++++++++ 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/src/builtins/test.rs b/src/builtins/test.rs index 6b0d546afcaa..e18235843f2c 100644 --- a/src/builtins/test.rs +++ b/src/builtins/test.rs @@ -1,6 +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::*; @@ -1036,17 +1037,31 @@ pub fn test(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Opt return STATUS_CMD_OK; } } - } else { - if argc == 0 { - return STATUS_INVALID_ARGS; // Per 1003.1, exit false. - } else if argc == 1 { - // Per 1003.1, exit true if the arg is non-empty. - return if args[0].is_empty() { - STATUS_CMD_ERROR - } else { - 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 + } else { + STATUS_CMD_OK + }; } // Try parsing 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/tests/checks/test-posix.fish b/tests/checks/test-posix.fish index 9852e266e97d..c9fcd74c6d90 100644 --- a/tests/checks/test-posix.fish +++ b/tests/checks/test-posix.fish @@ -25,3 +25,24 @@ echo $status 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: ^ + From 6a4b289ca3b5521bd1f25be004eaadb2e858f957 Mon Sep 17 00:00:00 2001 From: Fabian Boehm Date: Sat, 13 Apr 2024 11:45:16 +0200 Subject: [PATCH 3/3] document test-require-arg --- doc_src/cmds/test.rst | 13 ++++++++++++- doc_src/language.rst | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-) 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::