Rewrite builtin functions in rust

[faho]

Description

This is a rough draft of rewriting builtin functions in rust. Basically the first time it passed the tests.

It fixes two pre-existing bugs:

1. functions --details foo bar will mention whatever the last option is in the error - so functions --details --verbose foo bar will mention "--verbose". I don't believe the error is all that great anyway, but now it always uses --details because that's the "mode" that functions is in
2. The event::print code compared the enum values too deeply, and so it printed the "Event thing" header too often

Honestly the second felt extremely roundabout and awkward, and I'm pretty sure there's a nicer way.

In terms of implementation decisions I mostly decided not to make a report_function_metadata helper function because using that many mutable variables felt like going against the grain and didn't seem like it would have helped a lot.

TODOs:

• Changes to fish usage are reflected in user documentation/manpages.
• Tests have been added for regressions fixed
• User-visible changes noted in CHANGELOG.rst

[faho]

We've had this a few times, a helper method might be nice to avoid multiple "unsafe" blocks.

[henrikhorluck]

Once IO is ported, we can bump our MSRV to 1.70 so we can use is_terminal. Unless that has some behavior-difference I am not aware of, have not seen the internals of it

[faho]

Unless that has some behavior-difference I am not aware of, have not seen the internals of it

There is no behavior difference because the internals are:

https://github.com/rust-lang/rust/blob/4d7a80d48697171ed151c4667b74fc7d784c1836/library/std/src/sys/unix/io.rs#L79-L82

It does the exact same thing we already do - excluding the "out_is_redirected" check, so it would only replace the "isatty" part. (isatty is defined to only return 1 or 0, so the "==" vs "!=" makes no difference)

I would simply add a helper method to streams - streams.out_is_terminal() that encapsulates both checks.

I know that projects in the rust ecosystem like to move very fast wrt MSRV, and like to include a lot of dependencies, but that's not to my taste and I consider it to be one of the annoyances of the ecosystem. I would like to require a more compelling reason to increase MSRV or take dependencies than to replace one line with a slightly shorter one.

Of course once we have an MSRV of 1.70 for other reasons, it's fine to use is_terminal - but even then we would still keep the helper because of the "out_is_redirected" check. (unless we decide to change that behavior)

[faho]

22d92b8 should be the answer here.

[faho]

This is awkward, taken from type. This should probably be a match, and could also be a helper function.

Would that go in builtins/shared.rs?

[faho]

This seemed like the cleanest way to do this sort of "only one" check. The C++ just added the bools together, that's possible here but would require an as i32 cast for every bool.

[faho]

(this always feels extremely clunky in any programming language for what is a very common thing - "only print a newline for every following entry")

[faho]

This used to print "--verbose: expected"

[faho]

This used to print a second "Event signal" after the SIGTRAP line

[henrikhorluck]

This does not print an extra "Event signal" on LastC++11, and the output was sorted reverse of what it now prints, this appears to then 1. fix an earlier regression introduced in the Rust code, and 2. there is a behavior difference in the sorting, which is probably fine, but should perhaps be mentioned in the release notes?

I did a git bisect and it points to 9ac6cbe being the first commit that sorted it to how it is now.

This was the output of running the test after git checkout LastC++11

Testing file checks/functions.fish ... Failure:

  The CHECK on line 186 wants:
    SIGTERM term1

  which failed to match line stdout:58:
    SIGTERM term3

  Context:
    [...] from line 164 (stdout:55):
    end
    Event signal
    SIGTRAP fish_sigtrap_handler
     <= nothing to match CHECK on line 186: SIGTERM term1
     <= nothing to match CHECK on line 187: SIGTERM term2
    SIGTERM term3
    SIGTERM term2 <= no more checks
    SIGTERM term1

  when running command:
    ../test/root/bin/fish checks/functions.fish

[faho]

fix an earlier regression introduced in the Rust code

Yes, the issue was new in the rust version, caused by enum comparisons being deep.

Probably something to look out for - if we replace C++ enums with Rust enums, the C++ version only compares variants.

2. there is a behavior difference in the sorting, which is probably fine, but should perhaps be mentioned in the release notes?

I do not believe this is a thing that is expected to be stable. This isn't sorted alphabetically, it's sorted by definition order - and used to be in reverse definition (oldest last) order.

[henrikhorluck]

Mostly nits, some wrong short-options that should have tests, and the event-signal appears to have a sorting difference that should be mentioned in release-notes or reverted, otherwise LGTM

[henrikhorluck]

Nit: We use NONOPTION_CHAR_CODE elsewhere, except for abbr.rs which needs 1–3. No need to change

[faho]

Oh I quite misunderstood what NONOPTION_CHAR_CODE meant, then?

So it's used to be the char code for the one option that is long-only?

And if we have multiple options that are long-only?

Tbh I prefer naming the constant after the option.

[henrikhorluck]

It appears I might be one misunderstanding how NONOPTION_CHAR_CODE is supposed to be used then, I just assumed it should be used instead of 1 as char / '\x01' / '\u{1}', which is how I have used it in builtins/string/{escape,unescape} and builtins/path.

Looking at the commit that introduced it, it appears '\x01' has a special meaning as a non-option argument (duh), but only if the ordering is REQUIRE_ORDER | RETURN_IN_ORDER, which is set by + and - in short_opts respectively.

When the ordering is the default aka PERMUTE, \x01 does not have that special meaning.

My takeaway here is:

1. I dislike wgetoptr_t, since it mixes special meanings with legitimate return values, only configured through a char in a string. A minimal way of improving it would be to instead return an enum like:

enum WGetOptRetVal {
    LongOpt(usize), // where the usize is the "id" of the long-only option
    Opt(char),
    NonOption,
}

We could also make wopt accept using a literal number, making it result in a different enum-variant, to make this difference in usage much clearer. (This requires a IntoOption trait)

2. We should probably avoid using \x01 (regardless of whether it is as NONOPTION_CHAR_CODE or another named constant) as a short-option for long-only arguments to avoid accidentally mixing this usage, in that way your change to 2 instead of 1 as was present in the original code is good.

[faho]

I dislike wgetoptr_t, since it mixes special meanings with legitimate return values, only configured through a char in a string

Yup, it has some real crusty C heritage.

A minimal way of improving it would be to instead return an enum like:

This would return a different value for -D and --details, which is awkward. I don't think you could match like

LongOpt(id) | Opt(char) => {
}

even if you called the variables the same?

We should probably avoid using \x01 (regardless of whether it is as NONOPTION_CHAR_CODE or another named constant) as a short-option for long-only arguments to avoid accidentally mixing this usage

Agreed. Tho note that "PERMUTE" is the overwhelming majority of uses and changing from that to a different ordering is a pretty big change that is never going to happen for most uses.

[henrikhorluck]

This would return a different value for -D and --details, which is awkward. I don't think you could match like
LongOpt(id) | Opt(char)
even if you called the variables the same?

Ah, I named it a bit unspecificly, I intended LongOpt to be LongOnlyOpt, if that makes more sense? And then keep the same behaviour for Opt if the option has both long and short variants

But a move towards something like your interpretation would make it trivial to determine whether we got the argument as a short- or long-opt, just with another enum-variant.

We could also add more variants to move the optarg to this enum, avoiding all the w.optarg.unwrap()-s.

[henrikhorluck]

This does not print an extra "Event signal" on LastC++11, and the output was sorted reverse of what it now prints, this appears to then 1. fix an earlier regression introduced in the Rust code, and 2. there is a behavior difference in the sorting, which is probably fine, but should perhaps be mentioned in the release notes?

I did a git bisect and it points to 9ac6cbe being the first commit that sorted it to how it is now.

This was the output of running the test after git checkout LastC++11

Testing file checks/functions.fish ... Failure:

  The CHECK on line 186 wants:
    SIGTERM term1

  which failed to match line stdout:58:
    SIGTERM term3

  Context:
    [...] from line 164 (stdout:55):
    end
    Event signal
    SIGTRAP fish_sigtrap_handler
     <= nothing to match CHECK on line 186: SIGTERM term1
     <= nothing to match CHECK on line 187: SIGTERM term2
    SIGTERM term3
    SIGTERM term2 <= no more checks
    SIGTERM term1

  when running command:
    ../test/root/bin/fish checks/functions.fish

[faho]

Okay, reformat_for_screen is currently broken - it does not seem to ever finish. Running functions interactively let's fish spin and isn't even interruptible with ctrl-c.

I'm gonna try to find out where the issue is, if I can't find it I'm gonna remove it from functions for now.

Edit: Alright, found it. Mainly one check was the wrong way around, and another incorrectly compared to 0 instead of a variable that is initially set to 0.