wopterr was a feature to allow wgetopt to emit error messages; but we do
not use this and never will. Remove its support. No functional change
expected here.
We wrongly highlight this as prefix when actually the trailing slash should
invalidate it. Turns out path normalization drops the slash, so let's
sidestep that.
Fixes#9394
The "flag" field enables an option to discover which flag it was invoked
with. However in practice none of our options use multiple flags so this
parameter was always nullptr. Remove it and fix up all the builtins to
stop passing this.
No functional change here.
I believe this should be identical to the previous code and handle the same
cases (I'm guessing going by the comment that this came from a C codebase
without `bool` types).
The problem with the previous code is that it tripped up the `clangd` analyzer
into thinking `assert()` expressions can/should be simplified via DeMorgan's to
improve readability (because it was seeing the fully expanded macro).
The tty_ownership test was sometimes failing. In this test,
`fish_test_helper` creates a child and transfers the tty to it,
"abandoning" the tty. In some cases, the child was running before the
parent; the child claims the tty. When the parent tries to transfer it to
the child, it get SIGTTIN and stops. Fix this by ignoring SIGTTIN and
SIGTTOU.
This only affects macOS and BSDs.
The stack overflow tests are too slow without this.
This is because the tests are essentially quadratic: with 500 jobs, and
each job attempts to reap all jobs.
Inside a comment we offer plain file completions (or command completions if
the comment is in command position). However these completions are broken
because they don't consider any of the surrounding characters. For example
with a command line
echo # comment
^ cursor
we suggest file completions and insert them as
echo # comsomefile ment
Providing completions inside comments does not seem useful and it can be
misleading. Let's remove the completions; this should communicate better that
we are in a free-form comment that's not subject to fish syntax.
Closes#9320
When unsetting, the scope indicates the scope that was *removed* not
set, so the warning is incorrectly triggered. If anything, the confusion
is now removed or we emit a warning that the variable is still present
in another scope (but don't do that!).
Closes#9338.
This fixes#9321
IEEE Std 1003.1-2017 Issue 6 added optional error condition
[EINVAL] for if no conversion could be performed.
Switch back to wcstoimax/wcstoumax: do not work around the old FreeBSD
8 issue.
Add a test for printf '%d %d' 1 2 3
This addresses a long-standing TODO where `complete -C` output isn't
deduplicated.
With this patch, the same deduplication and sort procedure that is run on actual
pager completions is also executed for `complete -C` completions (with a `-C`
payload specified).
This makes it possible to use `complete -C` to test what completions will
actually be generated by the completions pager instead of it displaying
something completely divorced from reality, improving the productivity of fish
completions developers.
Note that completions that wouldn't be shown in the pager are also omitted from
the results, e.g. `test/buildroot/` and `test/fish_expand_test/` are omitted
from the check matches in `checks/complete_directories.fish` because even if
they were generated, the pager wouldn't have shown them. This again makes
reasoning about and debugging completions much easier and more sane.
This reverts commit 1c92d4c5db and
reintroduces support for trivially copyable `maybe_t` impls but with a
GCC version check to disable the optimization for GNU GCC compiler
versions 9 and below.
GCC 8.3.0 armhf builds seem to have a problem with the trivially
copyable `maybe_t` impl that introduces odd heisenbugs that cause the
tests to fail. GDB reveals that `maybe_t` function parameters received
in the callee differ from what was passed-in by the caller.
This behavior appears to be (but has not been confirmed as) a
platform-specific compiler bug. Under the same system (32-bit Debian 10
armhf), compiling with clang 7.0.1 does not result in any bugs and
causes all the tests to pass while compiling with GCC 10.2 under 32-bit
Debian 11 armhf also doesn't run into any problems, so just expand the
existing GCC version check that gates support for trivially copyable
`maybe_t` impls to encompass both the troublesome GCC 8 version and the
untested GCC 9 version.
This reverts commit 9d303a74e3.
This reverts commit 0305c842e6.
9d303a7 broke 32-bit armhf builds for unknown reasons, specifically in
settings where a trivial copy of `maybe_t<int>` was performed. A caller
would pass a literal int in the place of a `maybe_t<int>` parameter and
the callee would see a populated `maybe_t` but with a value of `0`
rather than the actual value that was passed in. It was too painful to
debug to a resolution under qemu.
Fixes ommitted newline char shown after complete -n'(foo)'
Also axes the 'contains syntax errors' line before the error.
Update tests
before
> complete -n'(foo)'
complete: Condition '(foo)' contained a syntax error
complete: Command substitutions not allowed⏎
after
> complete -n'(foo)'
complete: -n '(foo)': command substitutions not allowed here
This is a salvage of the "no functional changes" part of #9221, and cherry-picks
storing completion entries in a vector instead of a linked list. The legacy
"reverse intuitive" group ordering is kept by iterating in reverse order.
Tests pass but don't actually cover group order, which needs another test.
Makes it possible to retrieve the currently executing command line as
opposed to the currently executing command (`status current-command`).
Closes#8905.
There should be no functional changes in this commit.
The global variable `$_` set in the parser variables by `reader.cpp` and
read by the `status` builtin was deprecated in fish 2.0 but kept around
internally because there's no good way to store/share/forward parser
variables.
A new enum is added that identifies the status variable and they are
stored in a private array in the parser. There is no need for
synchronization because they are only set during job init and never
thereafter. This is currently asserted via ASSERT_IS_MAIN_THREAD() but
that assert can be dropped in the interest of making the parser possible
to clone and use from worker threads.
The old `$_` global variable is still kept for backwards compatibility,
though it will be dropped in a future release.
As the user is typing an argument, fish continually checks if the input is
the prefix of a valid file path. If yes, the input is underlined.
The same prefix-logic is used for all tokens on the command line, even for
"finished" tokens. This means we highlight any token that happens to be
a prefix of a valid file path. We actually want this to only apply to the
token that the user is currently typing.
Let's use the prefix-logic only for tokens adjacent to the cursor. This should
better match user expectations (and reduce IO traffic). I don't think this is
the perfect criteria but I don't know how else we can determine if a token is
"unfinished".
When visiting the "cd" node, we mark invalid paths as error, but don't
underline valid paths. This works fine most of the time because we later
underline paths (for any command, not just "cd").
However the latter check fails to honor CDPATH. Let's correct that, which
also allows to simplify the logic.
The next commit wants to move the "Underline every valid path" logic into the
visit() methods. The logic currently polls the cancel checker before checking
each path. If that's valid, it should probably have the same behavior inside
visit(). Since we currently can't cancel an AST-visitation, the next best
thing seems to suspend all IO operations, the rest should be very fast anyway.
I'm not sure if the motivation is strong enough; a conceivable alternative
would be to stop using the cancel checker altogether for highlighting.
When passing a value of type maybe_t<size_t>, clangd complains:
Parameter 'cursor' is passed by value and only copied once; consider
moving it to avoid unnecessary copies (fix available)
We get this warning because maybe_t<size_t> is not trivially copyable
because it has a user-defined destructor and copy-constructor. Let's remove
them if the contained type is trivially copyable, to avoid such warnings.
No functional change.
The destructor is equivalent to the compiler-generated one. The user-defined
destructor prevents maybe_t<size_t> from bearing the predicate "trivially
copyable". Let's remove it. No functional change.
It seems to have originally been thought that the only possible way a stack
overflow could happen is via function calls, but there are other possibilities.
Issue #9302 reports how `eval` can be abused to recursively execute a string
substitution ad infinitum, triggering a stack overflow in fish.
This patch extends the stack overflow check to also check the current
`eval_level` against a new constant `FISH_MAX_EVAL_DEPTH`, currently set to a
conservative but hopefully still fair limit of 500. For future reference, with
the default stack size for the main/foreground thread of 8 MiB, we actually have
room for a stack depth around 2800, but that's only with extremely minimal state
stored in each stack frame.
I'm not entirely sure why we don't check `eval_depth` regardless of block type;
it can't be for performance reasons since it's just a simple integer comparison
- and a ridiculously easily one for the branch predictor handle, at that - but
maybe it's to try and support non-recursive nested execution blocks of greater
than `FISH_MAX_STACK_DEPTH`? But even without recursion, the stack can still
overflow so may be we should just bump the limit up some (to 500 like the new
`FISH_MAX_EVAL_DEPTH`?) and check it all the time?
Closes#9302.
A `block_t` instance is allocated for each live block type in memory when
executing a script or snippet of fish code. While many of the items in a
`block_t` class are specific to a particular type of block, the overhead of
`maybe_t<event_t>` that's unused except in the relatively extremely rare case of
an event block is more significant than the rest, given that 88 out of the 216
bytes of a `block_t` are set aside for this field that is rarely used.
This patch reorders the `block_t` members by order of decreasing alignment,
bringing down the size to 208 bytes, then changes `maybe_t<event_t>` to
`shared_ptr<event_t>` instead of allocating room for the event on the stack.
This brings down the runtime memory size of a `block_t` to 136 bytes for a 37%
reduction in size.
I would like to investigate using inheritance and virtual methods to have a
`block_t` only include the values that actually make sense for the block rather
than always allocating some sort of storage for them and then only sometimes
using it. In addition to further reducing the memory, I think this could also be
a safer and saner approach overall, as it would make it very clear when and
where we can expect each block_type_type_t-dependent member to be present and
hold a value.
This is a false positive as a result of disabling TLS support in LSAN due to an
incompatibility with newer versions of glibc.
Also remove the older workaround (because it didn't work).
When there are multiple screens worth of output and `history` is writing to the
pager, pressing Ctrl-C at the end of a screen doesn't exit the pager (`q` is
needed for that) but previously caused fish to emit an error ("write:
Interrupted system call) until we starting silently handling SIGINT in
`fd_output_stream_t::append()`.
This patch makes `history` detect when the `append()` call returns with an error
and causes it to end early rather than repeatedly trying (and failing) to write
to the output stream.
If EINTR caused by SIGINT is encountered while writing to the
`fd_output_stream_t` output fd, mark the output stream as errored and return
false to the caller but do not visibly complain.
Addressing the outstanding TODO notwithstanding, this is needed to avoid
littering the tty with spurious errors when the user hits Ctrl-C to abort a
long-running builtin's output (w/ the primary example being `history`).
Up to now, in normal locales \x was essentially the same as \X, except
that it errored if given a value > 0x7f.
That's kind of annoying and useless.
A subtle change is that `\xHH` now represents the character (if any)
encoded by the byte value "HH", so even for values <= 0x7f if that's
not the same as the ASCII value we would diverge.
I do not believe anyone has ever run fish on a system where that
distinction matters. It isn't a thing for UTF-8, it isn't a thing for
ASCII, it isn't a thing for UTF-16, it isn't a thing for any extended
ASCII scheme - ISO8859-X, it isn't a thing for SHIFT-JIS.
I am reasonably certain we are making that same assumption in other
places.
Fixes#1352
Closes#9240.
Squash of the following commits (in reverse-chronological order):
commit 03b5cab3dc40eca9d50a9df07a8a32524338a807
Author: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date: Sun Sep 25 15:09:04 2022 -0500
Handle differently declared posix_spawnxxx_t on macOS
On macOS, posix_spawnattr_t and posix_spawn_file_actions_t are declared as void
pointers, so we can't use maybe_t's bool operator to test if it has a value.
commit aed83b8bb308120c0f287814d108b5914593630a
Author: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date: Sun Sep 25 14:48:46 2022 -0500
Update maybe_t tests to reflect dynamic bool conversion
maybe_t<T> is now bool-convertible only if T _isn't_ already bool-convertible.
commit 2b5a12ca97b46f96b1c6b56a41aafcbdb0dfddd6
Author: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date: Sun Sep 25 14:34:03 2022 -0500
Make maybe_t a little harder to misuse
We've had a few bugs over the years stemming from accidental misuse of maybe_t
with bool-convertible types. This patch disables maybe_t's bool operator if the
type T is already bool convertible, forcing the (barely worth mentioning) need
to use maybe_t::has_value() instead.
This patch both removes maybe_t's bool conversion for bool-convertible types and
updates the existing codebase to use the explicit `has_value()` method in place
of existing implicit bool conversions.
The parent commit made the destructor of the DIR* member close it if necessary
(i.e. only if it's not null). This means that we can use the same logic in
the move constructor (where the source DIR* is null) and for move assignment
(where it might not be).
No functional change.
dir_iter_t closes its DIR* member in two places: the move assignment and
the destructor. Simplify this by closing it in the destructor of the DIR*
member which is called in both places. Use std::unique_ptr, which is shorter
than a dedicated wrapper class. Conveniently, it calls the deleter only if
the pointer is not-null. Unfortunately, std::unique_ptr requires explicit
conversion to DIR* when interacting with C APIs but it's probably still
better than a wrapper class.
This means that the noncopyable_t annotation is now implied due to the
unique_ptr member.
Additionally, we could probably remove the user-declared move constructor
and move assignment (the compiler-generated ones should be good enough). To
be safe, keep them around since they also erase the fd (though I hope we
don't rely on that behavior anywhere).
We should perhaps remove the user-declared destructor entirely but
dir_iter_t::entry_t also has one, I'm not sure why. Maybe there's a good
reason, like code size.
No functional change.
This was recently converted to a while-loop. However, we only
loop in a specific case when (by hitting "continue") so a
loop condition is not necessary.
No functional change.
We forgot to decode (i.e. turn into nice wchar_t codepoints)
"byte_literal" escape sequences. This meant that e.g.
```fish
string match ö \Xc3\Xb6
math 5 \X2b 5
```
didn't work, but `math 5 \x2b 5` did, and would print the wonderful
error:
```
math: Error: Missing operator
'5 + 5'
^
```
So, instead, we decode eagerly.
descend_unique_hierarchy is used for the cd autosuggestion: if a directory
contains exactly one subdirectory and no other entries, then propose that
as part of the cd autosuggestion.
This had a bug: if the subdirectory is a symlink to the parent, we would
chase that, going around the loop suggesting a longer path until we hit
PATH_MAX.
Fix this by using the new API which provides the inode "for free," and
track whether we've seen this inode before. This is technically too
conservative since the inode may be for a directory on a different device,
but devices are not available for free so this would incur a cost. In
practice encountering the same inode twice with different devices in a
unique hierarchy is unlikely, and should it happen the consequences are
merely cosmetic: we fail to suggest a longer path.
This introduces dir_iter_t, a new class for iterating the contents of a
directory. dir_iter_t encapsulates the logic that tries to avoid using
stat() to determine the type of a file, when possible.
While we hardcode the return values for the rest of our builtins, the `return`
builtin bubbles up whatever the user returned in their fish script, allowing
invalid return values such as negative numbers to make it into our C++ side of
things.
In creating a `proc_status_t` from the return code of a builtin, we invoke
W_EXITCODE() which is a macro that shifts left the return code by some amount,
and left-shifting a negative integer is undefined behavior.
Aside from causing us to land in UB territory, it also can cause some negative
return values to map to a "successful" exit code of 0, which was probably not
the fish script author's intention.
This patch also adds error logging to help catch any inadvertent additions of
cases where a builtin returns a negative value (should one forget that unix
return codes are always positive) and an assertion protecting against UB.
This was always the case if HAVE_TEXT wasn't defined, but if it was then we were
coercing the result of `_C()` to a `const wchar_t *` pointer, because we were
returning the address of a constant zero-length wchar_t pointer. This reserves a
local static `wcstring` variable that we can return as the "no text" sentinel
and bubbles back the `wcstring` reference rather than decomposing it into a
pointer.
This is a prerequisite for a bigger change I'm working on.
It's gone from 136 bytes to a 128 bytes by rearranging the items in order of
decreasing alignment requirements. While this reduces the memory consumption
slightly (by around 6%) for each completion we have in-memory, that translates
to only around ~8KiB of savings for a command with 1000 possible completions,
which is nice but ultimately not that big of a deal.
The bigger benefit is that a single `complete_entry_t` might now fit in a cache
line, hopefully making the process of testing completions for matches more
cache friendly (and maybe even faster).
The impact here depends on the command and how much output it
produces.
It's possible to get up to 1.5x - `string upper` being a good example,
or a no-op `string match '*'`.
But the more the command actually needs to do, the less of an effect
this has.
This basically immediately issues a "write()" if it's to a pipe or the
terminal.
That means we can reduce syscalls and improve performance, even by
doing something like
```c++
streams.out.append(somewcstring + L"\n");
```
instead of
```c++
streams.out.append(somewcstring);
streams.out.push_back(L'\n');
```
Some benchmarks of the
```fish
for i in (string repeat -n 2000 \n)
$thing
end
```
variety:
1. `set` (printing variables) sped up 1.75x
2. `builtin -n` 1.60x
3. `jobs` 1.25x (with 3 jobs)
4. `functions` 1.20x
5. `math 1 + 1` 1.1x
6. `pwd` 1.1x
Piping yields similar results, there is no real difference when
outputting to a command substitution.
This writes the output once per argument instead of once per format or
escaped char.
An egregious case:
```fish
printf (string repeat -n 200 \\x7f)%s\n (string repeat -n 2000 aaa\n)
```
Has been sped up by ~20x by reducing write() calls from 40000 to 200.
Even a simple
```fish
printf %s\n (string repeat -n 2000 aaa\n)
```
should now be ~1.2x faster by issuing 2000 instead of 4000 write
calls (the `\n` was written separately!).
This at least halves the number of "write()" calls we do if it goes to
a pipe or the terminal, or reduces them by 75% if there is a
description.
This makes
```fish
complete -c foo -xa "(seq 50000)"
complete -C"foo "
```
faster by 1.33x.
This uses wreaddir_resolving, which tries to use the dirent d_type
field if it exists. In that way, it can skip the `stat` to determine
if the given file is a directory.
This allows `cd` completions to skip stat in most cases:
```fish
strace -Ce newfstatat fish --no-config -c 'complete -C"cd /tmp/completion_test/"' >/dev/null
```
prints before:
```
% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
100,00 0,002627 2 1033 4 newfstatat
```
after:
```
% time seconds usecs/call calls errors syscall
------ ----------- ----------- --------- --------- ----------------
100,00 0,000054 1 31 3 newfstatat
```
for a directory with 1000 subdirectories.
(just `fish --no-config -c exit` does 26 newfstatat)
This should improve the situation with slow filesystems like fuse or
network fsen.
In case we have no d_type, we use `stat`, which would yield about the
same results.
The worst case is that we need directories *and* descriptions or the
"executable" flag (which we don't currently check for cd, if I read
this right?).
This flag determines whether or not more shortopt switches will be offered up as
potential completions (vs only the payload for the last-parsed shortopt switch).
Previously, it was being stomped before it was determined whether or not two
`complete` rules with different `result_mode.requires_param` values were
actually resolved against the current command line or not, and the last
evaluated completion rule would win out.
There are two changes here:
* `last_option_requires_param` is only assigned if all associated conditions for
a potential completion are also met, and
* If already assigned by a conflicting rule (which can only be user/developer
error), `last_option_requires_param` is allowed to change from true to false
but not the other way around (i.e. in case of a conflict, generate both
payloads and other shortopt completions)
The first change is immediately noticeable and affects many of our own
completions, see the discussion in #9221 for an example regarding `git` where
`-c` has any of about a million different possible meanings depending on which
completion preconditions have been met. The second change should only happen if
a dev/user mistakenly enters a `complete -c ...` rule for the same shortopt more
than once, both with conditions matching, sometimes requiring an argument and
not sometimes not. It should be a rare occurence.
This reverts commit 3d8f98c395.
In addition to the issues mentioned on the GitHub page for this commit,
it also broke the CentOS 7 build.
Note one can locally test the CentOS 7 build via:
./docker/docker_run_tests.sh ./docker/centos7.Dockerfile
