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

faho · 2024-03-11T15:33:37Z

This introduces the "test-require-arg" feature flag.

When enabled, it removes builtin test's zero and one argument special modes.

That means:

test -n returns false
test -z returns true
test -x, test foo and test "" error out

Currently, all of these return true, except for the last which returns false, all silently.

This is a recognition that this is a frequent cause of problems. While we could (and still should) introduce a better replacement for test, this will fix a frequently recurring question, and remove awkward explanations we need to give.

Nobody needs zero- or one-argument test, and few people want it as a shortcut.

Fixes #2037.

TODOs:

Is this the right fix or do we e.g. error out for all one-argument calls?
Documentation
Do we add a deprecation FLOG for these if the feature flag is disabled? Or do we give people a mostly-correct regex to grep their code instead?
Tests have been added for regressions fixed
CHANGELOG

faho · 2024-03-11T15:45:08Z

(I'm pretty sure this will conflict with #10357, that's fine, this is easy to redo)

mqudsi · 2024-03-11T23:07:02Z

For posterity: this breaks with posix?

faho · 2024-03-12T07:46:45Z

For posterity: this breaks with posix?

Yes. Posix explicitly demands the zero- and one-argument behavior as we implement it currently.

This is a regular source of confusion and so I believe the break is warranted.

mqudsi · 2024-03-16T04:29:05Z

After mulling it over, I'm more open to this change. The "error out" behavior is easier to stomach vs the more "silent breakage" changes, but we've seen so many of these issues that I think it's overall warranted.

Funnily enough, the part I hate the most about this is "oh no, not another feature flag!"

krobelus · 2024-03-17T05:36:17Z

src/builtins/test.rs

+                program_name
+            ));
+            builtin_print_error_trailer(parser, streams.err, program_name);
+            return STATUS_INVALID_ARGS;


Good change.

One alternative is to keep returning the same exit code as today, but only print the error. This way we wouldn't really need a feature flag. I'm fine with either.

That's not a bad idea. That could be our "depreciation phase".

The way this is used, errors and warnings are often already breaking - if your prompt or worse a binding suddenly prints a warning that makes you want to avoid it, and so you would want to fix it immediately.

This isn't e.g. python, where you can add warnings that, for the most part, will be logged away with a script's output. The context of interactive use makes this different. (and for scripted use, stderr needs to be redirected and read pretty often, it's not separated enough)

That means printing an error strikes me as a bad idea without a feature flag.

Additionally "the same exit code as today" means that test -z returns true. This would mean that test -z returns true while printing an error.

Of course erroring out for all single-argument calls instead of allowing test -n and test -z is a possibility, but I would want to have it behind a feature flag.

correction, we'd still want a feature flag

faho · 2024-03-20T09:59:24Z

Alright, summarizing, I think we're agreed that a change is good. I think it's not possible to do it without a feature flag.

I also think we're agreed that test and test foo (where "foo" is anything that's not a valid operator) should error out. This also applies to test -n "..." -a foo. The implicit string test is horrible and should be removed.

So, the remaining cases are:

test -n and test -z with no further argument - currently these are both true (because it's test -n "-n"), my change here makes test -n false. The alternative is to make this error out.
test -d with no further argument (where -d is any valid operator that isn't -n or -z). These currently are all true, this makes them error out. The alternative is to make them return false ("the empty filename isn't a directory").

My idea is that this proposal allows you to do test -n $foo, with a variable $foo that you know is either zero or one elements (or do specifically test -n $foo[1]), and it will do the right thing for if it is empty.

The counter-argument is that that will still break if $foo contains more than one element and that you should quote it. The counter to that is that test is fundamentally not suited for testing lists - test -d "$foo" will not do the right thing if $foo has multiple elements no matter what you do.

krobelus · 2024-03-20T10:28:06Z

The counter-argument is that that will still break if $foo contains more than one element

right so sounds like we don't want to approve of any one-argument invocation -- knowing this, I'd keep the current exit code, to prevent disatrous breakage (but do print an error)

faho · 2024-03-20T11:00:18Z

right so sounds like we don't want to approve of any one-argument invocation

Well, no. We don't approve of test "foo", but test -n and test -z I can see being okay - that's the question I was posing.

knowing this, I'd keep the current exit code, to prevent disatrous breakage (but do print an error)

I still do not believe this is helpful - you would have to silence the error to prevent breakage anyway, and if you are touching the line anyway you can just quote the variable instead.

An error message in a prompt or keybinding, or when you redirect stderr (because many things print on stderr! often useful output!) is already breakage.

We could add a FLOG message to any one-argument test invocation that you can turn on? So you would do fish -d deprecated-test, and it would show any deprecated invocation (with location).

krobelus · 2024-03-20T11:39:29Z

I understand that extra output is breakage, sorry if I gave the wrong impression.
I was only talking about disastrous breakage like test $dir && rm -rf "$dir/"

faho · 2024-03-20T12:31:19Z

I was only talking about disastrous breakage like test $dir && rm -rf "$dir/"

test $dir would change from true to false even if $dir is non-empty (and would stay false if it is empty), so this wouldn't remove the directory. This would never run rm -rf /.

What would break is something like test -d $dir || rm -rf "$dir/", which would print an error and run rm -rf /.

Personally, I feel like having that behind a feature flag is already a lot. We could go further by adding FLOG messages. It would also be quite cool to have some form of static checking for this (unfortunately writing a good grep invocation here is pretty hard).

It would also be possible to have an in-between step where we warn about the change, but I would not want to have that as the endpoint. I would like to end up without errors where it returns true.

So how about this:

We add the feature flag (default off) and a FLOG category (default off) that logs affected test invocations
Some releases later: We turn on the FLOG category by default (but only if the feature flag is off)
Another release or two later: We turn on the feature flag by default

Note that FLOG isn't redirectable via stderr - it'll go to fish's --debug-output. We could make it so it only fires if test's stderr isn't redirected (or we make it an stderr message that is only printed if the FLOG category is enabled).

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

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

faho · 2024-04-13T10:20:25Z

Alright, I've added the FLOG message (in category "deprecated-test") and documented it.

Since this is a feature flag that would introduce a backwards-incompatible change, I'm going to wait for a sort of "final call" for comments before merging.

krobelus · 2024-04-13T10:59:14Z

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.");


Fine by me.

Given what I know I'm fine with enabling this warning now already
(and I'd probably not change the exit code but I'm not gonna fight)
In the light of that I'd also rephrase "will change" to "may change"

To be clear: As this PR stands, by default, it will not do anything.

If the warning is enabled, it will show the warning, the exit code will remain.

If the feature flag is enabled, the exit code will change.

The end goal is to have the behavior that I consider best, which is that test -n and test -z are fine with no additional arguments, anything else is an error.

The plan is to stage this carefully:

People can, once this is merged, enable the warning. Once they've fixed their code, they can set the feature flag

The next step is to enable the warning, but keep the flag off by default, so the exit codes stay

The step after that is to enable the warning and the flag, but allow disabling them

The step after that is to make the flag read-only and remove the warning - now we have the behavior we would want from test

That means we have a step where there's a warning but no change in the exit code (by default), but it's only a step. I do not consider "warn about bad behavior but keep it" to be a good final outcome, I would like to go further and end at "okay behavior".

But I also believe that giving people some time where they know the change is coming without already throwing warnings at them - which is, as we have discussed, itself a breaking change for many uses, even if it is easy to revert. That's why I would like to have that first step.

I think everything that can be said has been said already. You go ahead.
I don't fully agree but my expertise in this area is quite limited.

The plan is to stage this carefully:

People can, once this is merged, enable the warning. Once they've fixed their code, they can set the feature flag

yeah you're right that announcing things in release notes before we change anything is the proper way to do it.
Then plugin authors can react to it. Hard to tell if it matters in this case but it's good practice

faho added enhancement RFC labels Mar 11, 2024

faho added this to the fish next-3.x milestone Mar 11, 2024

krobelus reviewed Mar 17, 2024

View reviewed changes

krobelus approved these changes Mar 17, 2024

View reviewed changes

faho mentioned this pull request Mar 19, 2024

Un-hide documentation of non-fish shell builtins #10376

Closed

faho force-pushed the test-deposixify branch 2 times, most recently from 5f08123 to 3dbffcc Compare April 12, 2024 13:29

faho added 2 commits April 12, 2024 15:30

Introduce test-require-arg feature flag

91a16bb

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

Add deprecation warning

3dbffcc

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

faho force-pushed the test-deposixify branch from fa92875 to 6a4b289 Compare April 13, 2024 10:18

document test-require-arg

6a4b289

krobelus reviewed Apr 13, 2024

View reviewed changes

faho merged commit 2c17d34 into fish-shell:master Apr 21, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

faho commented Mar 11, 2024 •

edited

faho commented Mar 11, 2024

mqudsi commented Mar 11, 2024

faho commented Mar 12, 2024

mqudsi commented Mar 16, 2024

krobelus Mar 17, 2024

mqudsi Mar 18, 2024

faho Mar 19, 2024

krobelus Mar 19, 2024

faho commented Mar 20, 2024

krobelus commented Mar 20, 2024

faho commented Mar 20, 2024

krobelus commented Mar 20, 2024

faho commented Mar 20, 2024

faho commented Apr 13, 2024

krobelus Apr 13, 2024

faho Apr 13, 2024

krobelus Apr 13, 2024

krobelus Apr 13, 2024

		@@ -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.");

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

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

Conversation

faho commented Mar 11, 2024 • edited

TODOs:

faho commented Mar 11, 2024

mqudsi commented Mar 11, 2024

faho commented Mar 12, 2024

mqudsi commented Mar 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faho commented Mar 20, 2024

krobelus commented Mar 20, 2024

faho commented Mar 20, 2024

krobelus commented Mar 20, 2024

faho commented Mar 20, 2024

faho commented Apr 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faho commented Mar 11, 2024 •

edited