Introduce the internal jobs for functions

This PR is aimed at improving how job ids are assigned. In particular,
previous to this commit, a job id would be consumed by functions (and
thus aliases). Since it's usual to use functions as command wrappers
this results in awkward job id assignments.

For example if the user is like me and just made the jump from vim -> neovim
then the user might create the following alias:
```
alias vim=nvim
```
Previous to this commit if the user ran `vim` after setting up this
alias, backgrounded (^Z) and ran `jobs` then the output might be:
```
Job	Group	State	Command
2	60267	stopped	nvim  $argv
```
If the user subsequently opened another vim (nvim) session, backgrounded
and ran jobs then they might see what follows:
```
Job	Group	State	Command
4	70542	stopped	nvim  $argv
2	60267	stopped	nvim  $argv
```
These job ids feel unnatural, especially when transitioning away from
e.g. bash where job ids are sequentially incremented (and aliases/functions
don't consume a job id).

See #6053 for more details.

As @ridiculousfish pointed out in
https://github.com/fish-shell/fish-shell/issues/6053#issuecomment-559899400,
we want to elide a job's job id if it corresponds to a single function in the
foreground. This translates to the following prerequisites:

- A job must correspond to a single process (i.e. the job continuation
    must be empty)
- A job must be in the foreground (i.e. `&` wasn't appended)
- The job's single process must resolve to a function invocation

If all of these conditions are true then we should mark a job as
"internal" and somehow remove it from consideration when any
infrastructure tries to interact with jobs / job ids.

I saw two paths to implement these requirements:

- At the time of job creation calculate whether or not a job is
  "internal" and use a separate list of job ids to track their ids.
  Additionally introduce a new flag denoting that a job is internal so
  that e.g. `jobs` doesn't list internal jobs
  - I started implementing this route but quickly realized I was
    computing the same information that would be computed later on (e.g.
    "is this job a single process" and "is this jobs statement a
    function"). Specifically I was computing data that populate_job_process
    would end up computing later anyway. Additionally this added some
    weird complexities to the job system (after the change there were two
    job id lists AND an additional flag that had to be taken into
    consideration)
- Once a function is about to be executed we release the current jobs
  job id if the prerequisites are satisfied (which at this point have
  been fully computed).
  - I opted for this solution since it seems cleaner. In this
  implementation "releasing a job id" is done by both calling
  `release_job_id` and by marking the internal job_id member variable to
  -1. The former operation allows subsequent child jobs to reuse that
  same job id (so e.g. the situation described in Motivation doesn't
  occur), and the latter ensures that no other job / job id
  infrastructure will interact with these jobs because valid jobs have
  positive job ids. The second operation causes job_id to become
  non-const which leads to the list of code changes outside of `exec.c`
  (i.e. a codemod from `job_t::job_id` -> `job_t::job_id()` and moving the
   old member variable to a non-const private `job_t::job_id_`)

Note: Its very possible I missed something and setting the job id to -1
will break some other infrastructure, please let me know if so!

I tried to run `make/ninja lint`, but a bunch of non-relevant issues
appeared (e.g. `fatal error: 'config.h' file not found`). I did
successfully clang-format (`git clang-format -f`) and run tests, though.
This PR closes #6053.

src/builtin_bg.cpp

@@ -22,12 +22,12 @@ static int send_to_bg(parser_t &parser, io_streams_t &streams, job_t *j) {
     if (!j->wants_job_control()) {
         wcstring error_message = format_string(
             _(L"%ls: Can't put job %d, '%ls' to background because it is not under job control\n"),
-            L"bg", j->job_id, j->command_wcstr());
+            L"bg", j->job_id(), j->command_wcstr());
         builtin_print_help(parser, streams, L"bg", &error_message);
         return STATUS_CMD_ERROR;
     }
 
-    streams.err.append_format(_(L"Send job %d '%ls' to background\n"), j->job_id,
+    streams.err.append_format(_(L"Send job %d '%ls' to background\n"), j->job_id(),
                               j->command_wcstr());
     parser.job_promote(j);
     j->mut_flags().foreground = false;

src/builtin_disown.cpp

@@ -28,7 +28,7 @@ static int disown_job(const wchar_t *cmd, parser_t &parser, io_streams_t &stream
         killpg(j->pgid, SIGCONT);
         const wchar_t *fmt =
             _(L"%ls: job %d ('%ls') was stopped and has been signalled to continue.\n");
-        streams.err.append_format(fmt, cmd, j->job_id, j->command_wcstr());
+        streams.err.append_format(fmt, cmd, j->job_id(), j->command_wcstr());
     }
 
     // We cannot directly remove the job from the jobs() list as `disown` might be called

src/builtin_fg.cpp

@@ -91,11 +91,11 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     }
 
     if (streams.err_is_redirected) {
-        streams.err.append_format(FG_MSG, job->job_id, job->command_wcstr());
+        streams.err.append_format(FG_MSG, job->job_id(), job->command_wcstr());
     } else {
         // If we aren't redirecting, send output to real stderr, since stuff in sb_err won't get
         // printed until the command finishes.
-        std::fwprintf(stderr, FG_MSG, job->job_id, job->command_wcstr());
+        std::fwprintf(stderr, FG_MSG, job->job_id(), job->command_wcstr());
     }
 
     const wcstring ft = tok_first(job->command());

src/builtin_jobs.cpp

@@ -61,7 +61,7 @@ static void builtin_jobs_print(const job_t *j, int mode, int header, io_streams_
                 streams.out.append(_(L"State\tCommand\n"));
             }
 
-            streams.out.append_format(L"%d\t%d\t", j->job_id, j->pgid);
+            streams.out.append_format(L"%d\t%d\t", j->job_id(), j->pgid);
 
             if (have_proc_stat()) {
                 streams.out.append_format(L"%d%%\t", cpu_use(j));

src/builtin_wait.cpp

@@ -20,12 +20,12 @@
 static job_id_t get_job_id_from_pid(pid_t pid, const parser_t &parser) {
     for (const auto &j : parser.jobs()) {
         if (j->pgid == pid) {
-            return j->job_id;
+            return j->job_id();
         }
         // Check if the specified pid is a child process of the job.
         for (const process_ptr_t &p : j->processes) {
             if (p->pid == pid) {
-                return j->job_id;
+                return j->job_id();
             }
         }
     }
@@ -146,9 +146,9 @@ static bool find_job_by_name(const wchar_t *proc, std::vector<job_id_t> &ids,
         if (j->command().empty()) continue;
 
         if (match_pid(j->command(), proc)) {
-            if (!contains(ids, j->job_id)) {
+            if (!contains(ids, j->job_id())) {
                 // If pids doesn't already have the pgid, add it.
-                ids.push_back(j->job_id);
+                ids.push_back(j->job_id());
             }
             found = true;
         }
@@ -158,9 +158,9 @@ static bool find_job_by_name(const wchar_t *proc, std::vector<job_id_t> &ids,
             if (p->actual_cmd.empty()) continue;
 
             if (match_pid(p->actual_cmd, proc)) {
-                if (!contains(ids, j->job_id)) {
+                if (!contains(ids, j->job_id())) {
                     // If pids doesn't already have the pgid, add it.
-                    ids.push_back(j->job_id);
+                    ids.push_back(j->job_id());
                 }
                 found = true;
             }

src/event.cpp

@@ -157,7 +157,7 @@ wcstring event_get_desc(const event_t &evt) {
                 // In events, PGIDs are stored as negative PIDs
                 job_t *j = job_t::from_pid(-ed.param1.pid);
                 if (j) {
-                    return format_string(_(L"exit handler for job %d, '%ls'"), j->job_id,
+                    return format_string(_(L"exit handler for job %d, '%ls'"), j->job_id(),
                                          j->command_wcstr());
                 } else {
                     return format_string(_(L"exit handler for job with process group %d"),
@@ -170,7 +170,7 @@ wcstring event_get_desc(const event_t &evt) {
         case event_type_t::job_exit: {
             job_t *j = job_t::from_job_id(ed.param1.job_id);
             if (j) {
-                return format_string(_(L"exit handler for job %d, '%ls'"), j->job_id,
+                return format_string(_(L"exit handler for job %d, '%ls'"), j->job_id(),
                                      j->command_wcstr());
             } else {
                 return format_string(_(L"exit handler for job with job id %d"), ed.param1.job_id);

src/exec.cpp

@@ -511,7 +511,7 @@ static bool handle_builtin_output(parser_t &parser, const std::shared_ptr<job_t>
         p->completed = true;
         if (p->is_last_in_job) {
             FLOGF(exec_job_status, L"Set status of job %d (%ls) to %d using short circuit",
-                  j->job_id, j->preview().c_str(), p->status);
+                  j->job_id(), j->preview().c_str(), p->status);
             parser.set_last_statuses(j->get_statuses());
         }
         return true;
@@ -686,7 +686,7 @@ using proc_performer_t = std::function<proc_status_t(parser_t &parser)>;
 // exists. This is just a dumb artifact of the fact that we only capture the functions name, not its
 // properties, when creating the job; thus a race could delete the function before we fetch its
 // properties.
-static proc_performer_t get_performer_for_process(process_t *p, const job_t *job,
+static proc_performer_t get_performer_for_process(process_t *p, job_t *job,
                                                   const io_chain_t &io_chain) {
     assert((p->type == process_type_t::function || p->type == process_type_t::block_node) &&
            "Unexpected process type");
@@ -714,6 +714,13 @@ static proc_performer_t get_performer_for_process(process_t *p, const job_t *job
         };
     } else {
         assert(p->type == process_type_t::function);
+        // If this function is the only process in the current job
+        // and that job is in the foreground then mark this job as internal
+        // so it doesn't increment the job id for any jobs created within this
+        // function.
+        if (p->is_first_in_job && p->is_last_in_job && job->flags().foreground) {
+            job->mark_internal();
+        }
         auto props = function_get_properties(p->argv0());
         if (!props) {
             FLOGF(error, _(L"Unknown function '%ls'"), p->argv0());
@@ -1099,7 +1106,7 @@ bool exec_job(parser_t &parser, const shared_ptr<job_t> &j, const job_lineage_t
         }
     }
 
-    FLOGF(exec_job_exec, L"Executed job %d from command '%ls' with pgrp %d", j->job_id,
+    FLOGF(exec_job_exec, L"Executed job %d from command '%ls' with pgrp %d", j->job_id(),
           j->command_wcstr(), j->pgid);
 
     j->mark_constructed();

src/parse_execution.cpp

@@ -1287,14 +1287,14 @@ eval_result_t parse_execution_context_t::run_1_job(tnode_t<g::job> job_node,
     // the job ID here.
     auto &libdata = parser->libdata();
     const auto saved_caller_jid = libdata.caller_job_id;
-    libdata.caller_job_id = job->job_id;
+    libdata.caller_job_id = job->job_id();
 
     // Populate the job. This may fail for reasons like command_not_found. If this fails, an error
     // will have been printed.
     eval_result_t pop_result =
         this->populate_job_from_job_node(job.get(), job_node, associated_block);
 
-    assert(libdata.caller_job_id == job->job_id && "Caller job ID unexpectedly changed");
+    assert(libdata.caller_job_id == job->job_id() && "Caller job ID unexpectedly changed");
     parser->libdata().caller_job_id = saved_caller_jid;
 
     // Store time it took to 'parse' the command.

src/parser.cpp

@@ -576,7 +576,7 @@ void parser_t::job_promote(job_t *job) {
 
 job_t *parser_t::job_get(job_id_t id) {
     for (const auto &job : job_list) {
-        if (id <= 0 || job->job_id == id) return job.get();
+        if (id <= 0 || job->job_id() == id) return job.get();
     }
     return nullptr;
 }

src/postfork.cpp

@@ -75,7 +75,7 @@ bool child_set_group(job_t *j, process_t *p) {
             char command[64];
 
             format_long_safe(pid_buff, p->pid);
-            format_long_safe(job_id_buff, j->job_id);
+            format_long_safe(job_id_buff, j->job_id());
             format_long_safe(getpgid_buff, getpgid(p->pid));
             format_long_safe(job_pgid_buff, j->pgid);
             narrow_string_safe(argv0, p->argv0());

src/proc.cpp

@@ -271,10 +271,12 @@ void process_t::check_generations_before_launch() {
 
 job_t::job_t(job_id_t job_id, const properties_t &props, const job_lineage_t &lineage)
     : properties(props),
-      job_id(job_id),
+      job_id_(job_id),
       root_constructed(lineage.root_constructed ? lineage.root_constructed : this->constructed) {}
 
-job_t::~job_t() { release_job_id(job_id); }
+job_t::~job_t() {
+    if (job_id_ != -1) release_job_id(job_id_);
+}
 
 void job_t::mark_constructed() {
     assert(!is_constructed() && "Job was already constructed");
@@ -415,7 +417,7 @@ static void print_job_status(const job_t *j, job_status_t status) {
     if (status == JOB_STOPPED) msg = L"Job %d, '%ls' has stopped";
     outputter_t outp;
     outp.writestr("\r");
-    outp.writestr(format_string(_(msg), j->job_id, truncate_command(j->command()).c_str()));
+    outp.writestr(format_string(_(msg), j->job_id(), truncate_command(j->command()).c_str()));
     if (clr_eol) outp.term_puts(clr_eol, 1);
     outp.writestr(L"\n");
     fflush(stdout);
@@ -492,14 +494,14 @@ static bool try_clean_process_in_job(process_t *p, job_t *j, std::vector<event_t
             // We want to report the job number, unless it's the only job, in which case
             // we don't need to.
             const wcstring job_number_desc =
-                only_one_job ? wcstring() : format_string(_(L"Job %d, "), j->job_id);
+                only_one_job ? wcstring() : format_string(_(L"Job %d, "), j->job_id());
             std::fwprintf(stdout, _(L"%ls: %ls\'%ls\' terminated by signal %ls (%ls)"),
                           program_name, job_number_desc.c_str(),
                           truncate_command(j->command()).c_str(), sig2wcs(s.signal_code()),
                           signal_get_desc(s.signal_code()));
         } else {
             const wcstring job_number_desc =
-                only_one_job ? wcstring() : format_string(L"from job %d, ", j->job_id);
+                only_one_job ? wcstring() : format_string(L"from job %d, ", j->job_id());
             const wchar_t *fmt =
                 _(L"%ls: Process %d, \'%ls\' %ls\'%ls\' terminated by signal %ls (%ls)");
             std::fwprintf(stdout, fmt, program_name, p->pid, p->argv0(), job_number_desc.c_str(),
@@ -595,7 +597,7 @@ static bool process_clean_after_marking(parser_t &parser, bool allow_interactive
                     proc_create_event(L"JOB_EXIT", event_type_t::exit, -j->pgid, 0));
             }
             exit_events.push_back(
-                proc_create_event(L"JOB_EXIT", event_type_t::job_exit, j->job_id, 0));
+                proc_create_event(L"JOB_EXIT", event_type_t::job_exit, j->job_id(), 0));
         }
     }
 
@@ -757,8 +759,8 @@ int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) {
                 if (errno == ENOTTY) {
                     redirect_tty_output();
                 }
-                debug(1, _(L"Could not send job %d ('%ls') with pgid %d to foreground"), j->job_id,
-                      j->command_wcstr(), j->pgid);
+                debug(1, _(L"Could not send job %d ('%ls') with pgid %d to foreground"),
+                      j->job_id(), j->command_wcstr(), j->pgid);
                 wperror(L"tcsetpgrp");
                 return error;
             }
@@ -785,7 +787,7 @@ int terminal_maybe_give_to_job(const job_t *j, bool continuing_from_stopped) {
                 redirect_tty_output();
             }
 
-            debug(1, _(L"Could not send job %d ('%ls') to foreground"), j->job_id,
+            debug(1, _(L"Could not send job %d ('%ls') to foreground"), j->job_id(),
                   j->preview().c_str());
             wperror(L"tcsetattr");
             return error;
@@ -852,7 +854,7 @@ void job_t::continue_job(parser_t &parser, bool reclaim_foreground_pgrp, bool se
     mut_flags().notified = false;
 
     FLOGF(proc_job_run, L"%ls job %d, gid %d (%ls), %ls, %ls",
-          send_sigcont ? L"Continue" : L"Start", job_id, pgid, command_wcstr(),
+          send_sigcont ? L"Continue" : L"Start", job_id_, pgid, command_wcstr(),
           is_completed() ? L"COMPLETED" : L"UNCOMPLETED",
           parser.libdata().is_interactive ? L"INTERACTIVE" : L"NON-INTERACTIVE");
 

src/proc.h

@@ -315,6 +315,9 @@ class job_t {
     /// messages about job status on the terminal.
     wcstring command_str;
 
+    /// The job_id for this job.
+    job_id_t job_id_;
+
     // No copying.
     job_t(const job_t &rhs) = delete;
     void operator=(const job_t &) = delete;
@@ -378,7 +381,15 @@ class job_t {
     pid_t pgid{INVALID_PID};
 
     /// The id of this job.
-    const job_id_t job_id;
+    job_id_t job_id() const { return job_id_; }
+
+    /// Mark this job as internal. Internal jobs' job_ids are removed from the
+    /// list of jobs so that, among other things, they don't take a job_id
+    /// entry.
+    void mark_internal() {
+        release_job_id(job_id_);
+        job_id_ = -1;
+    }
 
     /// The saved terminal modes of this job. This needs to be saved so that we can restore the
     /// terminal to the same state after temporarily taking control over the terminal when a job