From 91a16bb0e853aa5b81e16ba5cd698ac2ba606436 Mon Sep 17 00:00:00 2001 From: Fabian Boehm Date: Fri, 12 Apr 2024 15:30:36 +0200 Subject: [PATCH] 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)