The translation is fairly direct though it adds some duplication, for example
there are multiple "match" statements that mimic function overloading.
Rust has no overloading, and we cannot have generic methods in the Node trait
(due to a Rust limitation, the error is like "cannot be made into an object")
so we include the type name in method names.
Give clients like "indent_visitor_t" a Rust companion ("IndentVisitor")
that takes care of the AST traversal while the AST consumption remains
in C++ for now. In future, "IndentVisitor" should absorb the entirety of
"indent_visitor_t". This pattern requires that "fish_indent" be exposed
includable header to the CXX bridge.
Alternatively, we could define FFI wrappers for recursive AST traversal.
Rust requires we separate the AST visitors for "mut" and "const"
scenarios. Take this opportunity to concretize both visitors:
The only client that requires mutable access is the populator. To match the
structure of the C++ populator which makes heavy use of function overloading,
we need to add a bunch of functions to the trait. Since there is no other
mutable visit, this seems acceptable.
The "const" visitors never use "will_visit_fields_of()" or
"did_visit_fields_of()", so remove them (though this is debatable).
Like in the C++ implementation, the AST nodes themselves are largely defined
via macros. Union fields like "Statement" and "ArgumentOrRedirection"
do currently not use macros but may in future.
This commit also introduces a precedent for a type that is defined in one
CXX bridge and used in another one - "ParseErrorList". To make this work
we need to manually define "ExternType".
There is one annoyance with CXX: functions that take explicit lifetime
parameters require to be marked as unsafe. This makes little sense
because functions that return `&Foo` with implicit lifetime can be
misused the same way on the C++ side.
One notable change is that we cannot directly port "find_block_open_keyword()"
(which is used to compute an error) because it relies on the stack of visited
nodes. We cannot modify a stack of node references while we do the "mut"
walk. Happily, an idiomatic solution is easy: we can tell the AST visitor
to backtrack to the parent node and create the error there.
Since "node_t::accept_base" is no longer a template we don't need the
"node_visitation_t" trampoline anymore.
The added copying at the FFI boundary makes things slower (memcpy dominates
the profile) but it's not unusable, which is good news:
$ hyperfine ./fish.{old,new}" -c 'source ../share/completions/git.fish'"
Benchmark 1: ./fish.old -c 'source ../share/completions/git.fish'
Time (mean ± σ): 195.5 ms ± 2.9 ms [User: 190.1 ms, System: 4.4 ms]
Range (min … max): 193.2 ms … 205.1 ms 15 runs
Benchmark 2: ./fish.new -c 'source ../share/completions/git.fish'
Time (mean ± σ): 677.5 ms ± 62.0 ms [User: 665.4 ms, System: 10.0 ms]
Range (min … max): 611.7 ms … 805.5 ms 10 runs
Summary
'./fish.old -c 'source ../share/completions/git.fish'' ran
3.47 ± 0.32 times faster than './fish.new -c 'source ../share/completions/git.fish''
Leftovers:
- Enum variants are still snakecase; I didn't get around to changing this yet.
- "ast_type_to_string()" still returns a snakecase name. This could be
changed since it's not user visible.
More ugliness with types that cxx bridge can't recognize as being POD. Using
pointers to get/set `termios` values with an assert to make sure we're using
identical definitions on both sides (in cpp from the system headers and in rust
from the libc crate as exported).
I don't know why cxx bridge doesn't allow `SharedPtr<OpaqueRustType>` but we can
work around it in C++ by converting a `Box<T>` to a `shared_ptr<T>` then convert
it back when it needs to be destructed. I can't find a clean way of doing it
from the cxx bridge wrapper so for now it needs to be done manually in the C++
code.
Types/values that are drop-in ready over ffi are renamed to match the old cpp
names but for types that now differ due to ffi difficulties I've left the `_ffi`
in the function names to indicate that this isn't the "correct" way of using the
types/methods.
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.
Let's hope this doesn't causes build failures for e.g. musl: I just
know it's good on macOS and our Linux CI.
It's been a long time.
One fix this brings, is I discovered we #include assert.h or cassert
in a lot of places. If those ever happen to be in a file that doesn't
include common.h, or we are before common.h gets included, we're
unawaringly working with the system 'assert' macro again, which
may get disabled for debug builds or at least has different
behavior on crash. We undef 'assert' and redefine it in common.h.
Those were all eliminated, except in one catch-22 spot for
maybe.h: it can't include common.h. A fix might be to
make a fish_assert.h that *usually* common.h exports.
The wait status value, which we also use internally, is read by a
bunch of macros.
Unfortunately because we want to *create* such a value, and some
systems lack the "W_EXITCODE" macro to do that, we need to figure out
how it's encoded.
So we simply check a specific value, and assume the encoding from
that.
On Haiku the return status is in the lower byte, on other systems it's
typically the upper byte.
TODO: Test on musl (that's the other system without W_EXITCODE).
Fixes#9067
c4fb857dac (in 3.4.1) introduced a regression where process_exit
events would only fire once the job itself is complete. Allow
process_exit events to fire before that. Fixes#8914.
If we ever need any of these... they're in this commit:
fish_wcswidth_visible()
status_cmd_opts_t::feature_name
completion_t::is_naturally_less_than()
parser_t::set_empty_var_and_fire()
parser_t::get_block_desc()
parser_keywords_skip_arguments()
parser_keywords_is_block()
job_t::has_internal_proc()
fish_wcswidth_visible()
We need special handling when reporting backtraces for commands run
during startup, i.e. config.fish. Previously we had a global variable;
make it local to the parser to eliminate a global.
No functional change here.
This is a big cleanup to how tty transfer works. Recall that when job
control is active, we transfer the tty to jobs via tcsetpgrp().
Previously, transferring was done "as needed" in continue_job. That is, if
we are running a job, and the job wants the terminal and does not have it,
we will transfer the tty at that point.
This got pretty weird when running mixed pipelines. For example:
cmd1 | func1 | cmd2
Here we would run `func1` before calling continue_job. Thus the tty
would be transferred by the nested function invocation, and also restored
by that invocation, potentially racing with tty manipulation from cmd1 or
cmd2.
In the new model, migrate the tty transfer responsibility outside of
continue_job. The caller of continue_job is then responsible for setting up
the tty. There's two places where this gets done:
1. In `exec_job`, where we run a job for the first time.
2. In `builtin_fg` where we continue a stopped job in the foreground.
Fixes#8699
This is a cleanup of job groups, rationalizing a bunch of stuff. Some
notable changes (none user-visible hopefully):
1. Previously, if a job group wanted a pgid, then we would assign it to the
first process to run in the job group. Now we deliberately mark which
process will own the pgroup, via a new `leads_pgrp` flag in process_t. This
eliminates a source of ambiguity.
2. Previously, if a job were run inside fish's pgroup, we would set fish's
pgroup as the group of the job. But this meant we had to check if the job
had fish's pgroup in lots of places, for example when calling tcsetpgrp.
Now a job group only has a pgrp if that pgrp is external (i.e. the job is
under job control).
"not not return 34" exits with 34, not 1. This behavior is pretty
surprising but benign. I think it's very unlikely that anyone relies
on the opposite behavior, because using two "not" decorators in one
job is weird, and code that compares not's raw exit code is rare.
The behavior doesn't match our docs, but it's not worth changing the
docs because that would confuse newcomers. Add a test to cement the
behavior and a comment to explain this is intentional.
I considered adding the comment at
parse_execution_context_t::populate_not_process where this behavior
is implemented but the field defintion seems even better, because I
expect programmers to read that first.
Closes#8377
This untangles some of the complicated logic and loops around posting
job exit events, and invoking the fish_job_summary function. No
functional change here (hopefully).
Exited processes generate event_t::process_exit if they exit with a
nonzero status. Prior to this change, to avoid sending duplicate events,
we would clear the status. This is ugly since we're lying about the
process exit status. Use a real flag to prevent sending duplicate
notifications.
Prior to this change, a function with an on-job-exit event handler must be
added with the pgid of the job. But sometimes the pgid of the job is fish
itself (if job control is disabled) and the previous commit made last_pid
an actual pid from the job, instead of its pgroup.
Switch on-job-exit to accept any pid from the job (except fish itself).
This allows it to be used directly with $last_pid, except that it now
works if job control is off. This is implemented by "resolving" the pid to
the internal job id at the point the event handler is added.
Also switch to passing the last pid of the job, rather than its pgroup.
This aligns better with $last_pid.
When a job is placed in the background, fish will set the `$last_pid`
variable. Prior to this change, `$last_pid` was set to the process group
leader of the job. However this caussed problems when the job ran in
fish's process group, because then fish itself would be the process group
leader and commands like `wait` would not work.
Switch `$last_pid` to be the actual last pid of the pipeline. This brings
it in line with the `$!` variable from zsh and bash.
This is technically a breaking change, but it is unlikely to cause
problems, because `$last_pid` was already rather broken.
Fixes#5036Fixes#5832Fixes#7721
In preparation for using wait handles in --on-process-exit events, factor
wait handles into their own wait handle store. Also switch them to
per-process instead of per-job, which is a simplification.
This switches builtin_wait from waiting on jobs in the active job list, to
waiting on the wait handles. The wait handles may be either derived from
the job list itself, or from saved wait handles from jobs that exited in
the background.
Fixes#7210
This is preparing to address the problem where fish cannot wait on a
reaped job, because it only looks at the active job list. Introduce the
idea of a "wait handle," which is a thing that `wait` can use to check if
a job is finished. A job may produce its wait handle on demand, and
parser_t will save the wait handle from wait-able jobs at the point they
are reaped.
This change merely introduces the idea; the next change makes builtin_wait
start using it.
Several functions including wgetopt and execve operate on null-terminated
arrays of nul-terminated pointers: a list of pointers to C strings where
the last pointer is null. Prior to this change, each process_t stored its
argv in such an array. This had two problems:
1. It was awkward to work with this type, instead of using std::vector,
etc.
2. The process's arguments would be rearranged by builtins which is
surprising
Our null terminated arrays were built around a fancy type that would copy
input strings and also generate an array of pointers to them, in one big
allocation.
Switch to a new model where we construct an array of pointers over
existing strings. So you can supply a `vector<string>` and now
`null_terminated_array_t` will just make a list of pointers to them. Now
processes can just store their argv in a familiar wcstring_list_t.
Prior to this change, builtins would take their arguments as `wchar_t **`.
This implies that the order of the arguments may be changed (which is
true, `wgetopter` does so) but also that the strings themselves may be
changed, which no builtin should do.
Switch them all to take `const wchar_t **` instead: now the arguments may
be rearranged but their contents may no longer be modified.
After commit 6dd6a57c60, 3 remaining
builtins were affected by uint8_t overflow: `exit`, `return`, and
`functions --query`.
This commit:
- Moves the overflow check from `builtin_set_query` to `builtin_run`.
- Removes a conflicting int -> uint8_t conversion in `builtin_return`.
- Adds tests for the 3 remaining affected builtins.
- Simplifies the wording for the documentation for `set --query`.
- Does not change documentation for `functions --query`, because it does
not state the exit code in its API.
- Updates the CHANGELOG to reflect the change to all builtins.
Prior to this change, the functions in exec.cpp would return true or false
and it was not clear what significance that value had.
Switch to an enum to make this more explicit. In particular we have the
idea of a "pipeline breaking" error which should us to skip processes
which have not yet launched; if no process launches then we can bail out
to a different path which avoids reaping processes.
Prior to this change, if fish were launched connected to a tty but not as
pgroup leader, it would attempt to become pgroup leader only if
--interactive is explicitly passed. But bash will unconditionally attempt
to become pgroup leader if launched interactively. This can result in
scenarios where fish is running interactively but in another pgroup. The
most obvious impact is that control-C will result in the pgroup leader
(not fish) exiting and make fish orphaned.
Switch to matching the bash behavior here - we will always try to become
pgroup leader if interactive.
Fixes#7060.
The 'time' prefix may come about either because the job itself is marked
with time, or because of the "inside out" weirdness of 'not time...'.
Factor this logic together and precompute it for a job.
This moves us slightly closer towards fish code in the background. The idea is
that a background job may still have "foreground" sub-jobs, example:
begin ; sleep 5 ; end &
The begin/end job runs in the background but should wait for `sleep`.
Prior to this fix, fish would see the overall job group is in the background
and not wait for any of its processes. With this change we detach waiting from
is_foreground.
This changes how fish attempts to protect itself from calling tcsetpgrp() too
aggressively. Recall that tcsetpgrp() will "force" itself, if SIGTTOU is
ignored (which it is in fish when job control is enabled).
Prior to this fix, we avoided SIGTTINs by only transferring the tty ownership
if fish was already the owner. This dated from a time before we had really
nailed down how pgroups should be assigned. Now we more deliberately assign a
job's pgroup so we don't need this conservative check.
However we still need logic to avoid transferring the tty if fish is not the
owner. The bad case is when job control is enabled while fish is running in the
background - here fish would transfer the tty and "steal" from the foreground
process.
So retain the checks of the current tty owner but migrate them to the point of
calling tcsetpgrp() itself.
add_disowned_pgid skipped jobs that have a PGID equal to the running
process. However, this includes processes started in config.fish or when
job control is turned off, so they never get waited on.
Instead, refactor this function to add_disowned_job, and add either the PGID or
all the PIDs of the job to the list of disowned PIDs/PGIDs.
Fixes#7183.
