Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve type function #1540

Merged
merged 12 commits into from Aug 22, 2014
Merged

Improve type function #1540

merged 12 commits into from Aug 22, 2014

Conversation

@lilyball
Copy link
Member

@lilyball lilyball commented Jul 10, 2014

Multiple fixes to the type function to make it run much faster and behave better.

This includes adding a -s flag to the command builtin, so we can avoid invoking which in the common case for type.

Fixes #54.

@lilyball lilyball changed the title Make type function better Improve type function Jul 10, 2014
@zanchey
Copy link
Member

@zanchey zanchey commented Jul 10, 2014

You might have seen, but this makes the tests fail:

Error output differs for file test7.in. Diff follows:
1d0
< type: Could not find “fish_test_type_zzz”

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 10, 2014

Oh huh, I didn't actually realize there were tests for the functions >_>

I'll look into that right now, thanks.

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 10, 2014

Ah hah, it's because it uses >/dev/null to quiet the error, which is now ^/dev/null. I only looked at the other functions for usages of type (because I didn't think it could be used anywhere else).

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 10, 2014

Test fixed, thanks @zanchey.

@mrak
Copy link
Contributor

@mrak mrak commented Jul 10, 2014

Should we model the behaviour after the POSIX command?

Right now this implementation uses the -p flag in the way that the traditional command uses -v. The traditional use of -p tells command to use a default value for PATH when looking up programs

source: man 1 command

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 10, 2014

@mrak Hrm, maybe that's a good idea. I picked -p because I made the assumption that command -v was a bash thing, but you're right, it actually does appear to be specified by POSIX. Here's The Open Group description. Furthermore, it exists as an executable at /usr/bin/command (at least on my system), which is another point in favor of using the same flags.

At this point, perhaps we should also support the -p flag to use the default PATH as well.

Although now that I'm looking at command a bit closer, it actually isn't doing what I thought it did. I thought that it would do a PATH search for command, ignoring functions, etc. But that's not actually what it does. command -v foo (where foo is a function) actually prints the details of the function foo instead of doing a PATH search. This is very surprising to me. Furthermore, command foo itself actually doesn't necessarily do a PATH search either; it suppresses functions/aliases, but it very explicitly supports builtins.

So I guess the question is, does Fish have any desire to match the POSIX behavior for shell builtins? Note that without this PR, Fish's command is still in violation of the POSIX spec for multiple reasons. Not only does it not have the -p flag, but it also suppresses all builtin lookup as well; according to POSIX, command cd foo should behave like cd foo, but in Fish it behaves like /usr/bin/cd foo (which naturally does nothing, being a child process and unable to affect the cwd of the shell).

Given the fact that Fish already diverges from POSIX, I think it's safe to say we don't need to follow POSIX except as where it makes things easier for the user (e.g. test, AFAIK, is based on POSIX with a few exceptions). That said, the POSIX command -p behavior is actually potentially useful, which suggests that a) we should implement it, and b) my command -p in this PR should be command -v, although I'm not really all that keen on the name "verbose" for this behavior.

@mrak
Copy link
Contributor

@mrak mrak commented Jul 10, 2014

I know it is frequently suggested that command -v is the de facto way to check for the existence of a program in a script regardless of system or shell dialect

command -p is very useful for locally-defined scripts that mask the system scripts. For instance, I have my own tmux on an extended PATH that calls the system-level tmux with specific flags:

$HOME/bin/tmux:

#!/bin/sh

command -p tmux -f "$XDG_CONFIG_HOME/tmux/tmux.conf" "$@"

Since command -p uses a default PATH, it doesn't cause infinite recursion by calling my tmux script over and over again.

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 10, 2014

@mrak That's a good reason for command -p to exist in some form. I've always used type (with various flags) to test for the presence of a command, rather than command -v, but of course I'm usually scripting for sh/bash rather than for arbitrary shell dialects (arbitrary shell dialects is potentially problematic anyway, because of the other differences in the shells). My motivation for adding command -p in this PR was to provide some builtin way to do a PATH search so I could get type to stop using which (except it still needs it for the -a functionality because fish's PATH search implementation doesn't have a way to request all paths, and I didn't feel like extending it that far, though I certainly could). I don't really care how the PATH search is invoked, only that it's possible.

So I guess the real question with having this functionality be in command in fish is whether the behavior in this PR is correct or whether it should follow POSIX and return information about e.g. functions/builtins. I gather that the POSIX command -v foo is meant to tell you what would happen if you just ran foo by itself, whereas the functionality implemented in this PR instead tells you what would happen if you ran command foo. Given the fact that Fish's command foo actually already differs from POSIX command foo, I think it's probably safe to keep the current behavior (with the possible adjustment of flag choice).

@mrak
Copy link
Contributor

@mrak mrak commented Jul 10, 2014

I do like the idea of type not having to delegate to which. I've actually made this mistake with a PR in another library thinking that type wouldn't have to leave the fish environment to find a command.

👍 to the functionality

The expectation that fish isn't POSIX could invalidate my argument for -v versus -p.

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 10, 2014

@mrak I am partial to -p meaning "path" (and also being consistent with type -p), but I am leaning towards implementing the POSIX command -p functionality in some form (where it searches the default $PATH).

After reflecting upon various flag names, I'm inclined towards renaming my -p/--path to -s/--search, and then adding -p/--default-path (-p here just for the nod towards POSIX command, but --default-path because --path is ambiguous) to search just the default $PATH.

@zanchey
Copy link
Member

@zanchey zanchey commented Jul 11, 2014

That sounds good. Matching at least a subset of SUS is helpful and helps avoid astonishment. You could add -v as an alias for -s, and possibly make -V call type?

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 11, 2014

@zanchey I could make -v as an alias for -s, although I'm not aware of any precedent in other Fish builtins/functions for having flag aliases, but I suppose it wouldn't hurt.

I don't think it makes sense to try and make -V call type. It could possibly work if type was a builtin, but it's a function, one that theoretically could be rewritten by the user. Also, I have no idea how to even begin having a builtin call a function.

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 11, 2014

I spent some time looking into what it would take to implement the --default-path flag, and it turns out to be entirely non-trivial. The only mechanism right now through which jobs are invoked is the parser, and I can't easily ask it to run the command that was passed as arguments to the command builtin, unless I want to try and construct a new fish script string that represents the already-expanded values that the command builtin sees (and I would rather not do that if possible).

I think someone who's already familiar with how jobs are executed (which I've never looked at before tonight) should consider dealing with this. I've already filed an issue #1540 about things like builtin command foo not working right, and the solution there is to teach the builtin/command/exec builtins how to do their job when invoked as builtins instead of handled by the parser. Once that's done, adding a --default-path flag should be much simpler.

So in the meantime, I think this PR needs to stand on its own without the --default-path flag.

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 14, 2014

I don't know how I missed it before, but the pure-fish option parser doesn't allow combined short flags, e.g. -qf. That needs to be fixed.

lilyball added 9 commits Jul 14, 2014
Give the `command` builtin a single flag, -p/--path, that causes it to
print the full path that would be executed for the given command.
Stop using getopt to parse flags. It's far more expensive than
necessary, and results in long flags not being parsed on OS X. This also
allows args starting with - after the options list to be properly
interpreted as a value to test.

Print the error message to stderr as is appropriate.

Use the new `command -p` functionality when the -a flag has not been
provided (`command` does not have any equivalent to the -a flag),
instead of using `which`. This is faster and also avoids any possible
disagreement between `which` and what fish thinks is valid.

Stop testing every path to see if it's executable, that test has already
been done by `which` or `command -p`.

The end result is `type -P ls` is roughly 250% faster, according to
profiling, on my OS X machine.
Track whether -a and -f have been supplied separately. That way both
`type -a -f command` and `type -f -a command` behaves correctly, as does
`type -a -f foo` where there are multiple executables named `foo` in the
$PATH.
The --quiet flag is useful when only the exit status matters.

Fix the documentation for the -t flag to no longer claim that `type` can
print "keyword", as it never does that.

Stop printing a blank line for functions/builtins when the -p flag has
been passed. It's just not useful.
Use `functions -q` instead of searching the `functiosn -na` list for the
provided word. This may result in an automatically-loaded function being
sourced, but that happens anyway with the default output.

This change means the results of `test -q foo` can be relied upon to
indicate whether `foo` can actually be invoked. Previosly, if `foo` was
the name of an automatically-loaded function file but did not actually
define a function `foo`, and there was no execuable `foo`, then `type -q
foo` would lie and say `foo` can be invoked when it can't.
One of the tests was using `>/dev/null` to suppress the `type` output.
That needs to be `^/dev/null` now, but instead just go ahead and use the
new `-q` flag.
@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Jul 14, 2014

What's the pure-fish options parser? Do you mean wgetopt(), or the getopt command used in type?

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 14, 2014

@ridiculousfish 5f7d9ce (part of this PR) ditches the getopt command and just parses the flags itself. This is faster, and it also means long option names work on OS X. But as my comment indicates, I didn't properly handle combined short flags such as -qf.

Right now I'm looking into perhaps defining a function __fish_getopt to do option parsing for fish functions, so we can drop the getopt usage in other places as well, and get long option support across the board.

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Jul 14, 2014

Thanks for the clarification. FYI, I've been working on a docopt implementation suitable for fish (#478) that is intended to become the option parser used internally for functions. I hope to have it testable by the end of the month.

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 14, 2014

@ridiculousfish Never heard of docopt before. I'll read the linked issue shortly, but in the meantime, I guess that means I shouldn't bother writing a __fish_getopt function. Instead I'll just fix the type impl here to handle combined short flags correctly.

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 14, 2014

@ridiculousfish Ok after skimming the wiki page it looks like docopt adds quite a lot of verbosity. That could be perfectly fine for bundled functions, but there's still a need for simple option-parsing for third-party functions. So perhaps I should go ahead and add a __fish_getopt impl anyway that can be used by anyone who doesn't want to opt in to full docopt support.

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Jul 14, 2014

@kballard That's a great point, that it may be too heavyweight for user defined functions. I'd be willing to modify the syntax to support more lightweight modes. For user defined functions, it might be as simple as:

function myfunc --signature '-f -g'
  ...

The benefit to the function author is that arguments are parsed and variables populated automatically, e.g.:

   if set -q arg_f:
       ....
   end
   if set -q arg_g
      ...
   end

and tab completions / autosuggestions work too.

@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 14, 2014

@ridiculousfish A more lightweight mode would be nice, but figuring out a proper syntax that's lightweight while still allowing short/long option pairs, possibly with arguments, might take a bit of work.

One option might be to simply support full getopts functionality, e.g.

function myfunc --shortopts fg: --longopts foo,grape:
    # $argv is modified into full getopt form
    ...
end

although I'm not really a fan of that as an official way of declaring functions, because e.g. it doesn't give you any niceties to actually test flags directly. You still have to process $argv.

lilyball added 2 commits Jul 14, 2014
Enhance the `read` builtin to support creating an array with the --array
flag. With --array, only a single variable name is allowed and the
entire input is tokenized and placed into that variable as an array.

Also add custom behavior if IFS is empty or unset. In that event, split
the input on every character, instead of the previous behavior of doing
no splitting at all.
When running `make test` we want to use the local function definitions,
not the ones installed on the system.

The system config.fish will still insert the system definitions at the
end, but at least ours will take precedence.
@xfix
Copy link
Member

@xfix xfix commented Jul 14, 2014

By the way, in Perl 6, sub MAIN takes arguments. For example, the contains function from fish shell can look like this.

#!/usr/bin/env perl6
sub MAIN($key, *@values, Bool :i(:$index)) {
    for @values.kv -> $i, $value {
        if $key eq $value {
            say $i + 1 if $index;
            exit 0;
        }
    }
    exit 1;
}

When called incorrectly, and usage is not specified (using USAGE function), Perl 6 automatically generates usable usage message.

Usage:
  ./contains [-i|--index] <key> [<values> ...]

The syntax is too ugly for fish shell (it's Perl 6 after all), but I guess something like this could be done when not using verbose syntax to do so (not everyone would want to use docstrings for short scripts).

Fix the parsing of `type` flags to handle combined short flags as
appropriate, e.g. `type -qf ls`.
@lilyball
Copy link
Member Author

@lilyball lilyball commented Jul 14, 2014

@ridiculousfish I just pushed new commits that restore the combined short flags parsing. This required making changes to the read builtin to support character splitting. I added in array support at the same time, because I was going to use that for my __fish_getopts, which I implemented but didn't include in this PR because it's actually more expensive than the rest of type combined.

I also fixed make test while I was at it so it can test the functions from the repo instead of the functions installed on-disk.

@lilyball
Copy link
Member Author

@lilyball lilyball commented Aug 21, 2014

@ridiculousfish Any more feedback on this?

@ridiculousfish ridiculousfish added this to the next-minor milestone Aug 22, 2014
@ridiculousfish ridiculousfish merged commit 16e50c2 into fish-shell:master Aug 22, 2014
1 check passed
1 check passed
@ridiculousfish
continuous-integration/travis-ci The Travis CI build passed
Details
@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Aug 22, 2014

Looks good to me. Thanks, this was a big set of changes! Merged as 033373f

@lilyball lilyball deleted the make_type_better branch Aug 22, 2014
@siteshwar
Copy link
Member

@siteshwar siteshwar commented Aug 22, 2014

These changes caused build failures for utopic and trusty on launchpad ( https://launchpad.net/~fish-shell/+archive/ubuntu/nightly-master/+packages) :

g++ -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -D_LARGEFILE_SOURCE=1 -D_FILE_OFFSET_BITS=64 -fno-exceptions -Wall -Wno-sign-compare -D_GNU_SOURCE=1 -D_ISO99_SOURCE=1  -DLOCALEDIR=\"/usr/share/locale\" -DPREFIX=L\"/usr\" -DDATADIR=L\"/usr/share\" -DSYSCONFDIR=L\"/etc\" -DBINDIR=L\"/usr/bin\" -DDOCDIR=L\"/usr/share/doc/fish\"  -D_FORTIFY_SOURCE=2 -DFISH_BUILD_VERSION=\"2.1.0-755-gf549ada\"  -c -o function.o function.cpp
g++ -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -D_LARGEFILE_SOURCE=1 -D_FILE_OFFSET_BITS=64 -fno-exceptions -Wall -Wno-sign-compare -D_GNU_SOURCE=1 -D_ISO99_SOURCE=1  -DLOCALEDIR=\"/usr/share/locale\" -DPREFIX=L\"/usr\" -DDATADIR=L\"/usr/share\" -DSYSCONFDIR=L\"/etc\" -DBINDIR=L\"/usr/bin\" -DDOCDIR=L\"/usr/share/doc/fish\"  -D_FORTIFY_SOURCE=2 -DFISH_BUILD_VERSION=\"2.1.0-755-gf549ada\"  -c -o builtin.o builtin.cpp
builtin.cpp: In function 'int builtin_read(parser_t&, wchar_t**)':
builtin.cpp:2639:79: error: taking address of temporary array
                     env_set(argv[i], j < bufflen ? (wchar_t[2]){buff[j], 0} : L"", place);
                                                                               ^
<builtin>: recipe for target 'builtin.o' failed
make[1]: *** [builtin.o] Error 1
make[1]: Leaving directory '/build/buildd/fish-2.1.0-755-gf549ada'
dh_auto_build: make -j1 returned exit code 2
debian/rules:13: recipe for target 'build' failed
make: *** [build] Error 2
dpkg-buildpackage: error: debian/rules build gave error exit status 2

@lilyball
Copy link
Member Author

@lilyball lilyball commented Aug 22, 2014

Fixed by 1f3a93a

simnalamburt added a commit to simnalamburt/shellder that referenced this issue Oct 28, 2016
This reverts commit 6199f40.

I reverted this commit to support fish 2.0.0 properly, since `-q` option
of `type` function was introduced in fish 2.2.0.

See also:
  fish-shell/fish-shell@6f7a745
  fish-shell/fish-shell#1540
  fish-shell/fish-shell#54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants