Commit graph

5234 commits

Author SHA1 Message Date
Johannes Altmanninger
de7f39d627 builtin bind: make function keys lowercase (f1 instead of F1)
All other key names are lowercase so this inconsistency is weird.
2024-05-22 22:38:06 +02:00
Johannes Altmanninger
2fa98ec20c Fix deadlock when importing universal LC_* variable
The C++ version of this code simply copied the entire uvar table.
Today we take a reference. It's not clear which one is better.

Removal of locale variables like LC_ALL triggers variable change handlers
which call EnvStackImpl::get. This deadlocks because we still hold the lock
to protect the reference to all uvars.  Work around this.

Closes #10513
2024-05-21 23:11:06 +02:00
Mahmoud Al-Qudsi
0b5e41b268 Rework path_normalize_for_cd() to be less allocation trigger happy
Lots of resizing, splicing, and full-on allocating going on here.
2024-05-21 12:54:52 -05:00
Mahmoud Al-Qudsi
d14d8d5733 Remove wcstringutil::split_string()
It is short and simple enough to write yourself if you need it and it encourages
bad behavior by a) always returning owned strings, b) always allocating them in
a vector. If/where possible, it is better to a) use &wstr, b) use an iterator.

In rust, it's an anti-pattern to unnecessarily abstract over allocating
operations. Some of the call sites even called split_string(..).into_iter().
2024-05-21 12:54:52 -05:00
Mahmoud Al-Qudsi
96b979077c Add unit tests for path_normalize_for_cd() 2024-05-21 12:54:51 -05:00
Mahmoud Al-Qudsi
8c62f733b3 Extend certain WSL workarounds to WSLv2
This updates is_windows_subsystem_for_linux() to take a WSL version to test for
(any, v1, or v2) and returns the boolean result depending on the system. I've
benchmarked and when running on regular Linux, this is still just as fast as the
previous binary check; it's only when it's WSL that this takes about 20ns
longer to figure out which variant.

Note that older WSLv2 kernels had a `-microsoft-standard` suffix while newer
ones appear to have a `-microsoft-standard-WSL2` suffix, so we make sure to test
for the least common denominator. (It doesn't matter to us, but note that newer
WSLv2 kernels have four dots in the version string!)

WSL workarounds pertaining to the default Windows terminal or executable
behavior of win32 binaries under a WSL shell are extended to WSLv2 while those
specific to oddities in kernel behavior are confined to WSLv1 only. (It
technically wouldn't hurt to extend them to WSLv2 but there's no good reason to
do so, either.)
2024-05-20 14:14:25 -05:00
Mahmoud Al-Qudsi
3374692b91
Work around $PATH issues under WSL (#10506)
A common complaint has been the massive amount of directories Windows appends to
$PATH slowing down fish when it attempts to find a non-existent binary (which it
does a lot more often than someone not in the know might think). The typical
workaround suggested is to trim unneeded entries from $PATH, but this a) has
considerable friction, b) breaks resolution of Windows binaries (you can no
longer use `clip.exe`, `cmd.exe`, etc).

This patch introduces a two-PATH workaround. If the cmd we are executing does
not contain a period (i.e. has no extension) it by definition cannot be a
Windows executable. In this case, we skip searching for it in any of the
auto-mounted, auto-PATH-appended directories like `/mnt/c/Windows/` or
`/mnt/c/Program Files`, but we *do* include those directories if what we're
searching for could be a Windows executable. (For now, instead of hard-coding a
list of known Windows executable extensions like .bat, .cmd, .exe, etc, we just
depend on the presence of an extension at all).

e.g. this is what starting up fish prints with logging enabled (that has been
removed):

    bypassing 100 dirs for lookup of kill
    bypassing 100 dirs for lookup of zoxide
    bypassing 100 dirs for lookup of zoxide
    bypassing 100 dirs for lookup of fd
    not bypassing dirs for lookup of open.exe
    not bypassing dirs for lookup of git.exe

This has resulted in a massive speedup of common fish functions, especially
anywhere we internally use or perform the equivalent of `if command -q foo`.

Note that the `is_windows_subsystem_for_linux()` check will need to be patched to
extend this workaround to WSLv2, but I'll do that separately.

Under WSL:
* Benchmark `external_cmds` improves by 10%
* Benchmark `load_completions` improves by an incredible 77%
2024-05-20 10:29:32 -05:00
ridiculousfish
42f8672f34 Remove an errant {} from a FLOG 2024-05-19 10:27:45 -07:00
Johannes Altmanninger
d40d2b786f Work around wants_terminal not begin set inside eval
On this binding we fail to disable CSI u

    bind c-t '
        begin
            set -lx FZF_DEFAULT_OPTS --height 40% --bind=ctrl-z:ignore
            eval fzf | while read -l r; echo read $r; end
        end
    '

because for "fzf", ParseExecutionContext::setup_group() returns early with the
parent process group (which should be fish's own) , hence "wants_terminal"
is false. This seems questionable, I don't think the eval should make a
difference here.

For now, don't touch it; use the more accurate way of detecting whether
a process may read keyboard input. In many of such cases "wants_terminal"
is false, like

    echo (echo 1\n2\n3 | fzf)

Fixes #10504
2024-05-18 20:55:06 +02:00
Mahmoud Al-Qudsi
d8e9a17c1f Inline extract_prefix_and_unescape_yaml()
We sometimes call it but discard half its results, so force it to be inlined to
make sure we don't perform work we then throw away.
2024-05-17 16:11:46 -05:00
Mahmoud Al-Qudsi
bcb1e2ed85 Further optimize unescape_yaml_fish_2_0()
This hot function dominates the flamegraphs for the completions thread, and any
optimizations are worthwhile.

A variety of different approaches were tested and benchmarked against real-world
fish-history file inputs and this is the one that won out across all rustc
target-cpu variations tried.

Benchmarks and code at https://github.com/mqudsi/fish-yaml-unescape-benchmark
2024-05-17 16:06:08 -05:00
Mahmoud Al-Qudsi
3d648e6e04 Remove Arc from environment::globals() (take 2)
We don't forward this variable for storage in any structs, so there's no reason
to go through an Arc instead of returning the `&'static EnvStack` directly.

NB: This particular change was safe, and passes all tests on its own.
2024-05-16 21:17:15 -05:00
Mahmoud Al-Qudsi
45e249dd94 Revert removal of Arc from principal() and global()
This reverts commit c6d3bde0c6.
This reverts commit 4ce13f0adb.
2024-05-16 21:08:06 -05:00
Mahmoud Al-Qudsi
c6d3bde0c6 Remove Arc from environment::globals()
We don't forward this variable for storage in any structs, so there's no reason
to go through an Arc instead of returning the `&'static EnvStack` directly.
2024-05-16 20:50:23 -05:00
Mahmoud Al-Qudsi
4ce13f0adb Simplify lifetime of environment::principal()
It's clearer that using it with `Rc::from_raw()` is safe since we don't have to
go through an `Arc<T>`.
2024-05-16 20:46:28 -05:00
Mahmoud Al-Qudsi
ec1bf60941 Add note about possible safety issue with GLOBAL_NODE 2024-05-16 20:33:43 -05:00
Mahmoud Al-Qudsi
66618d64cb Convert some lazy_static to OnceCell/OnceLock
These have clearer sync/unsync semantics and now ship with rust itself.
They don't paper over any possible cross-thread issues, and we can specifically
choose which we want for the purpose.
2024-05-16 20:33:43 -05:00
Mahmoud Al-Qudsi
0f18480559 Simplify Parser and EnvStack singletons and clarify thread semantics
`Parser` is a single-threaded `!Send`, `!Sync` type and does not need to use
`Arc` for anything. We were using it because that's all we had for the parser's
`EnvStack`, but though that is *technically* protected internally by a mutex
(shared with global EnvStack), there's nothing to say that other parsers with a
narrower scope/lifetime on other threads will be necessarily using the same
backing mutex.

We can safely marshal the existing `Arc<EnvStack>` we get from
`environment::principal()` into an `Rc<EnvStack>` since the underlying reference
is always valid. To prove this point, we could have PRINCIPAL_STACK be a static
`EnvStack` and have `environment::principal()` use `Arc::from_raw()` to turn
that into an `Arc<EnvStack>`, but there's no need to factorize this process.
2024-05-16 20:33:39 -05:00
Mahmoud Al-Qudsi
e4282f3798 Remove all locking from principal_parser()
By inverting the order of storage, we can use an `OnceCell`/`unsync::Lazy`
inside the Send/Sync `MainThread<T>` and remove the need for a lock altogether.
2024-05-16 14:23:25 -05:00
Johannes Altmanninger
29f2da8d18 Toggle terminal protocols lazily
Closes #10494
2024-05-16 12:26:47 +02:00
Johannes Altmanninger
6f9d5cf44c Address clippy lint 2024-05-16 12:25:29 +02:00
Mahmoud Al-Qudsi
b82170aded Change extract_prefix_and_unescape_yaml() assert!() to debug_assert!()
It's reasonable since this is only checking to see that the history file
contains the expected format and if it's corrupted but we at least got what we
expect to be the correct key/value pairs, then that's all we can do.

Of course the real motivation is to speed up this very hot function in any way
possible!
2024-05-15 22:42:19 -05:00
Mahmoud Al-Qudsi
51275525c1 Speed up extract_prefix_and_unescape_yaml
On the completions and history thread, the parent function
HistoryFileContents::decode_item() is responsible for ~60% of the CPU time, and
extract_prefix_and_unescape_yaml() alone comprising 14% (of the total).

This change removes allocations in the event that the history item is either
fully or partially plain yaml with no escapes to begin with, and brings down the
execution time of this function to only 7% of the total execution time.

The bulk of the remaining time is spent in wcs2string(), which is called
unconditionally and is naturally alloc-heavy.
2024-05-15 22:42:19 -05:00
ridiculousfish
caef202551 Fix some more clippies and enable more warnings 2024-05-15 20:05:35 -07:00
Fabian Boehm
f1e19884fb Add set --no-event
This allows running `set` without triggering any event handlers.

That is useful, for example, if you want to set a variable in an event
handler for that variable - we could do it, for example, in the
fish_user_path or fish_key_bindings handlers.

This is something the `block` builtin was supposed to be for, but it
never really worked because it only allows suppressing the event for
the duration, they would fire later. See #9030.

Because it is possible to abuse this, we only have a long-option so
that people see what is up.
2024-05-14 17:31:47 +02:00
Fabian Boehm
403920e9d6 input: Remove all_mappings_cache
Replace it with a chained iterator.

This not only simplifies the code, but also removes a RefCell.
2024-05-14 16:14:02 +02:00
Johannes Altmanninger
a19ff4989a Prevent out-of-order execution following repaint
Commit a583fe723 ("commandline -f foo" to skip queue and execute immediately,
2024-04-08) fixed the execution order of some bindings but was partially
backed out in 5ba21cd29 (Send repaint requests through the input queue again,
2024-04-19) because repainting outside toplevel yields surprising results
(wrong $status etc).

Transient prompts wants to first repaint and then execute some more readline
commands, all within a single binding.  This was broken by the second commit
because that one defers the repaint until after the binding has finished.

Work around this problem by deferring input events again while a readline
event was queued. This is closest to the historical behavior.

The implementation feels hacky; we might find odd situations.
For example,

    commandline -f repaint end-of-line
    set token (commandline -t)

sets the wrong token.
Probably not a very important case. We could throw an error or make it work
by letting "commandline -t" drain the input queue.

That seems too complicated, better change repaints to not use the input queue
(and fake $status etc). Let's try to do that in future.

Closes #10492
2024-05-13 10:44:12 +02:00
ridiculousfish
d6e231af0d Fix some clippies 2024-05-12 14:55:35 -07:00
ridiculousfish
172fb22692 Clean up and correct O_SEARCH definitions
Correct the FreeBSD constant and simplify some of the cfg() logic.
2024-05-11 13:20:34 -07:00
Jonathan Krebs
4606b02d44 rustfmt 2024-05-11 11:12:29 -07:00
Jonathan Krebs
a148760963 cd: open directory with O_SEARCH or O_PATH, when the platform supports it 2024-05-11 11:12:29 -07:00
Jonathan Krebs
2ecbdb9ae7 cleanup: fds::open_dir - remove mode argument
[w]open_dir does not pass O_CREAT, so the mode argument to open is never used.
also, O_CREAT | O_DIRECTORY could not be used (portably) to create a directory.
(on POSIX does not specify what should happen, on Linux it is EINVAL.)
2024-05-11 11:12:29 -07:00
Mahmoud Al-Qudsi
35a16e3713 Rename FISH_TSAN_WORKAROUNDS and add feature to Cargo.toml
rustc 1.80 now complains about features not declared in Cargo.toml and cfg
keys/values not declared by build.rs to protect against typos or misuse (you
think you're using the right condition but you're not). See
rust-lang/cargo#10554 and rust-lang/rust#82450.

(We're not actually using TSAN under CI at this time, but I do want to re-enable
it at some point — especially if we get multithreaded execution going — using
the rust-native TSAN configuration.)

I'll be updating the `rsconf` crate and patching `build.rs` accordingly to also
handle the warnings about unknown cfg values, but tsan is a feature and not a
cfg and these can be dealt with in `Cargo.toml` directly.
2024-05-09 13:58:42 -05:00
Mahmoud Al-Qudsi
059b842d1b is_wsl(): Move .copied() earlier in the iter pipeline
This is a best practice that usually helps with auto vectorization, as llvm has
a hard time seeing through the references.
2024-05-09 13:33:50 -05:00
Mahmoud Al-Qudsi
5dcc5955fb Don't allocate a vector twice in wcs2zstring()
We were passing a slice (and not a vec) to `CString::new()`, meaning it would
allocate a new Vec internally to hold the bytes.

Also document that the resulting CString will be silently truncated at the first
interior NUL.
2024-05-09 13:19:06 -05:00
Mahmoud Al-Qudsi
883bcbad83 Remove possible panic from wperror() 2024-05-09 11:20:09 -05:00
Mahmoud Al-Qudsi
84be043f31 Clean up src/libc.rs some
Update the CVAR!() macro and use it more.
2024-05-08 22:34:10 -05:00
Mahmoud Al-Qudsi
92eee61fb5 Convert colors::Flags to a bitflags enum
We use accessors and setters for all operations, so there's no benefit to
storing the modifiers as separate boolean fields.
2024-05-07 14:03:38 -05:00
Fabian Boehm
41a0fe2b1d history: Check for cmd key earlier
This shows up hot in `perf record ./fish` style profiles.

This assumes that "- cmd" can't be escaped.
2024-05-07 20:23:32 +02:00
Mahmoud Al-Qudsi
f709795a3a Reserve vector capacity upfront in colors::named_color_names()
Only because why not.
2024-05-07 13:09:40 -05:00
Mahmoud Al-Qudsi
fd2ea3ff0f Add more tests for color::try_parse_rgb() 2024-05-07 13:00:11 -05:00
Mahmoud Al-Qudsi
5b79f267d6 Optimize color::try_parse_rgb()
The function was repeatedly calling `s.char_at(n)` which is O(1) only for UTF-32
strings (so not a problem at the moment). But it was also calling `hex_digit(n)`
twice for each `n` in the 3-digit case, causing unnecessary repeated parsing of
individual characters into their radix-16 numeric equivalents, which could be
avoided just by reusing the already calculated result.
2024-05-07 12:53:44 -05:00
Fabian Boehm
b9d44407b3 tokenizer: Stop copying the start string 2024-05-07 16:59:35 +02:00
Mahmoud Al-Qudsi
5f8f799cf7 Replace C++ doc \return with "Return"
quick_replace '\\\\return(s)? ' 'Return$1 ' src/

Filtered to only lines beginning with //
2024-05-06 14:59:36 -05:00
Mahmoud Al-Qudsi
589639a87d Replace C++-style \p with Markdown backticks
quick_replace '\\\\p ([a-zA-Z0-9_]+)' '`$1`' src/

Filtered to only lines beginning with //
2024-05-06 14:59:23 -05:00
Mahmoud Al-Qudsi
a35925b3ed Don't use $HOME under cargo test harness
We will continue to use the "normal" fish base directory detection when using
the CMake test harness which properly sets up a sandboxed $HOME for fish to use,
but when running source code tests with a bare `cargo test` we don't want to
write to the actual user's profile.

This also works around test failures when running `cargo test` under CI with a
locked-down $HOME directory (see #10474).
2024-05-06 14:31:51 -05:00
Fabian Boehm
2a121ef1aa function: Check if --argument-names gets a valid variable name
These were accepted but then ineffective because the only way these
are used is to set a variable.
2024-05-06 17:00:45 +02:00
Fabian Boehm
39b996332e functions: Fix --argument-names display
Mentioned in #10465
2024-05-06 17:00:45 +02:00
Mahmoud Al-Qudsi
2d2b75003d Fix format string presumably broken under i686
%ld expects a 32-bit param under x86, change it to %lld instead.
2024-05-04 22:57:55 -05:00
Mahmoud Al-Qudsi
476b360eb8 Remove rust test dependency on cmake output
The test_history_formats test was reading from build/tests/ which is an artifact
of the cmake test runner. The source code tests should not depend on the cmake
test harness at all, so this is changed to read from the original test source in
the ./tests/ directory instead.
2024-05-04 20:49:49 -05:00