Types like Environment are sometimes imported using their FQN and
sometimes via their re-export in the parent module; it seems better
to have one canonical name. I think we usually use the re-exports
already, so use them always.
It't not as trivial to enforce this in child modules (such as "mod
tests" which) but that's not a big loss.
While at it, make some other consistency improvements to imports.
This parameter is used to suppress certain verbose errors that are
expected during tests. It was awkwardly threaded through multiple call
sites. Just set it (test only) on Parser.
As described in
https://github.com/fish-shell/fish-shell/pull/9990#discussion_r1382494440,
prior to 77aeb6a2a8 (Port execution, 2023-10-08), "Parser" was
passed by mutable reference ("parser_t&"), even though operation
context was passed as "const operation_context_t &". This worked
because C++ doesn't propagate const to pointers by default (see
https://en.cppreference.com/cpp/experimental/propagate_const).
class operation_context_t {
std::shared_ptr<parser_t> parser;
...
};
So "*ctx->parser" was a "parser_t&", not "const parser_t&".
Rust has stricter const propagation rules which means that const
operation context can't simply hand out a non-const reference to parser.
To be able to port code without changing its structure,
77aeb6a2a8 passed "Parser" by shared reference, using interior
mutability (RefCell) to modify parser fields. This is a bit ugly
(c.f. https://doc.rust-lang.org/std/cell/index.html "interior mutability
is something of a last resort") and means that some borrowing conflicts
are not found at compile time but runtime.
Pass both parser and operation context by exclusive reference, and
remove the interior mutability wrappers from parser's fields.
Since "libdata" is no longer inside a "RefCell", add a "ScopedRefCell"
around "transient_commandline".
The downside is that "ScopeGuard" use can become more intrusive
when we pass "Parser" or "OperationContext" as context (especially
when we use "zelf" since we can't shadow "self"), see
* 2930466d53 (Introduce ScopedCell and ScopedRefCell, 2025-03-15)
* 29ae571afa (Make scoped_push nicer, 2024-12-28)
Avoid this in some cases, specifically when using "ScopedCell" or
"ScopedRefCell". Since "&mut Parser" prevents the "ScopeCell"'s
"ScopeGuard" from holding a shared reference, use an "Rc" to capture
a dynamically-checked reference to the Cell. We could also use raw
pointers instead.
Change "Completer::apply_var_assignments" to return a block ID, to
avoid the need to return a "zelf" "ScopeGuard". In future, we could
probably untangle completer and get away with returning a "ScopeGuard"
called "ctx".
Closes#12694
In C++ we can't have a field and method sharing a name,
but in Rust we can.
For some structs, most getters don't have a "get_", so it's weird
that some do. Remove the "get_" prefix where it's obvious enough.
While at it, give some related getters better names.
The ScopedRefCell wrapping from library_data
is used for two things
1. to allow mutating library_data from a &Parser (for this, a RefCell would be enough)
2. to replace "current_filename" for a scope
A following commit wants to pass parser as "&mut Parser", which
voids reason 1. It will also remove the ScopedRefCell wrapping
from LibraryData because reason 2 alone is not strong enough. Move
"current_filename" outside of that, next to "current_node" which is
already a ScopedRefCell. In future we could maybe consolidate them
into one field, like (or even merging with) ScopedData.
In Rust, "safety" is usually used in the context of unsafe functions,
which have documented preconditions. Our async-signal-safe functions
are different; they offer extra safety properties. Rename them to
reduce confusion.
Ref: https://github.com/fish-shell/fish-shell/pull/12625#discussion_r3067819966
The decoding functions for our widestrings are already in the
`fish_widestring` crate, so by symmetry, it makes sense to put the
encoding functions there as well. This also makes it easier to depend on
these functions, giving more options when it comes to further code
extraction.
Part of #12625
These functions don't depend on `wcstringutil` functionality, so there
is no need for them to be there. The advantage of putting them into our
`widestring` crate is that quite a lot of code depends on it, and
extracting some of that code would result in crate dependency cycles if
the functions stayed in the `wcstringutil` crate. Our `widestring` crate
does not depend on any of our other crates, so there won't be any cyclic
dependency issues with code in it.
Part of #12625
Not reexporting means that imports have to change to directly import
from `fish_common`. This makes it easier to see which dependencies on
`src/common.rs` actually remain, which helps with identifying candidates
for extraction.
While at it, group some imports.
Part of #12625
Similar to `perror_io`, we don't need to make a libc call for `nix`
results, since the error variant contains the errno, from which a static
mapping to an error message exists. Avoid using `perror` and instead use
`perror_io` or `perror_nix` as appropriate where possible.
The `perror_io` and `perror_nix` functions could be combined by
implementing `fish_printf::ToArg` for `nix::errno::Errno`, but such a
function would violate type safety, as it would allow passing any
formattable argument, not necessarily limited to functions with a `%s`
formatting.
Part of #12502
This time, functions for decoding `wstr` into various types and the
`ToCString` trait are extracted.
Part of the wider goal of slimming down the main library to improve
incremental build performance and reduce dependency cycles.
Part of #12492
We sometimes use explicit reclaim() and sometimes rely on the drop
implementation. This adds an unnecesary step to reading all uses of
this code. Make this consistent. Use drop everywhere though we could
use explicit reclaim too.
Rust has this annoying design where all of the syscall conveniences on
File assume that it owns its fd; in particular this means that we can't
easily construct File from stdin, a raw file descriptor, etc.
The usual workarounds are to construct a File and then mem::forget it
(this is apparently idiomatic Rust!). But this has problems of its own:
for example it can't easily be used in Drop.
Introduce BorrowedFdFile which wraps File with ManuallyDrop and then
never drops the file (i.e. it's always forgotten). Replace some raw FDs
with BorrowedFdFile.
Commit 7996637db5 (Make fish immediately show color changes again,
2025-12-01) repaints unnecessarily when a local unexported color
variable changes. Also, it repaints when the change comes from
fish_prompt, causing an easy infinite loop. Same when changing TERM,
COLORTERM and others.
This feature is relevant when using a color-theme aware theme, so
try to keep it. Repaint only on global/universal changes.
Also ignore changes if already repainting fish prompt.
This change may be at odds with concurrent execution (parser should
not care about whether we are repainting) but that's intentional
because of 1. time constraints and 2. I'm not sure what the solution
will look like; we could use the event infrastructure. But a lot of
existing variable listeners don't use that.
Extract a context object we pass whenever we mutate the environment; While
at it, use it to pass EnvMode::USER, to reduce EnvMode responsibilities.
Fixes#12233
This command
echo $(/bin/echo -n 1; echo -n 2)
sometimes outputs "21" because we implement this as
let bufferfill = IoBufferfill::create_opts(...);
...
let eval_res = parser.eval_with(...);
let buffer = IoBufferfill::finish(bufferfill);
i.e. /bin/echo and builtin echo both output to the same buffer; the
builtin does inside parser.eval_with(), and the external process may
or may not output before that, depending on when the FD monitor thread
gets scheduled (to run item_callback).
(Unrelated to that we make sure to consume all available input in
"IoBufferfill::finish(bufferfill)" but that doesn't help with
ordering.)
Fix this by reading all available data from stdout after the child
process has exited.
This means we need to pass the BufferFill down to
process_mark_finished_children().
We don't need to do this for builtins like "fg" or "wait",
because commands that buffer output do not get job control, see
2ca66cff53 (Disable job control inside command substitutions,
2021-07-26).
We also don't need to do it when reaping from reader because there
should be no buffering(?).
fish still deviates from other shells in that it doesn't wait for
it's child's stdout to be closed, meaning that this will behave
non-deterministically.
fish -c '
echo -n $(
sh -c " ( for i in \$(seq 10000); do printf .; done ) & "
)
' | wc -c
We should fix that later.
Closes#12018
Having the prelude in wchar is not great. The wchar module was empty
except for the prelude, and its prelude included things from wutil.
Having a top-level prelude module in the main crate resolves this. It
allows us to completely remove the wchar module, and a top-level prelude
module makes more sense conceptually. Putting non-wchar things into the
prelude also becomes more sensible, if we ever want to do that.
Closes#12182
Another reduction in size of the main crate. Also allows other crates to
depend on the new wchar crate.
The original `src/wchar.rs` file is kept around for now to keep the
prelude imports working.
Part of #12182
The logic added by 2dbaf10c36 (Also refresh TTY timestamps
after external commands from bindings, 2024-10-21) is obsoleted
by TtyHandoff. That module is also responsible for calling
reader_save_screen_state after it writes to the TTY, so we don't
actually need to check if it wrote anything.
Not sure if this will be useful but the fact that we use very
few Unicode characters, suggests that we are insecure about
this. Having some kind of central and explicit listing might help
future decision-making. Obviously, completions and translations use
more characters, but those are not as central.
This merges changes that make thread pools instanced. We no longer have
a single global thread pool. This results in significant simplifications
especially in the reader (no more "canary").
"cargo test" captures stdout by default but not stderr.
So it's probably still useful to suppress test output like
in function 'recursive1'
in function 'recursive2'
[repeats many times]
This was done by should_suppress_stderr_for_tests() which has been
broken. Fix that, but only for the relevant cases instead of setting
a global.
Sometimes we need to spawn threads to service internal processes. Make
this use a separate thread pool from the pool used for interactive tasks
(like detecting which arguments are files for syntax highlighting).
Some string handling functions deal with `Vec<u8>` or `&[u8]`, which
have been referred to as `string` or `str` in the function names. This
is confusing, since they don't deal with Rust's `String` type. Use
`bytes` in the function names instead to reduce confusion.
Closes#11969
This commit adds `style_edition = "2024"` as a rustfmt config setting.
All other changes are automatically generated by `cargo fmt`.
The 2024 style edition fixes several bugs and changes some defaults.
https://doc.rust-lang.org/edition-guide/rust-2024/rustfmt-style-edition.html
Most of the changes made to our code result from a different sorting
method for `use` statements, improved ability to split long lines, and
contraction of short trailing expressions into single-line expressions.
While our MSRV is still 1.70, we use more recent toolchains for
development, so we can already benefit from the improvements of the new
style edition. Formatting is not require for building fish, so builds
with Rust 1.70 are not affected by this change.
More context can be found at
https://github.com/fish-shell/fish-shell/issues/11630#issuecomment-3406937077Closes#11959
Previously, if you called a function parameter 'argv', within the body
of the function, argv would be set to *all* the arguments to the
function, and not the one indicated by the parameter name.
The same behaviour happened if you inherited a variable named 'argv'.
Both behaviours were quite surprising, so this commit makes things more
obvious, although they could alternatively simply be made errors.
Part of #11780
Length modifiers are useless. This simplifies the code a bit, results in
more consistency, and allows removing a few PO messages which only
differed in the use of length modifiers.
Closes#11878
