Port bg builtin to Rust

[clemenswasser]

Description

I had to replace some job_t references to indices to avoid ownership/aliasing issues with the mutable reference to parser_t (e. g. send_to_bg). I also had to introduce some const "hacks" as cxx::SharedPtr doesn't currently allow for mutable access dtolnay/cxx#885 (I'm not really happy with this myself, so if anyone has some safer and more const correct alternatives would be great).

I could observe the following correct behavior of the ported version:

> sleep 100000000
^Zfish: Job 1, 'sleep 100000000' has stopped
> bg
Send job 1 'sleep 100000000' to background
> bg
bg: There are no suitable jobs
> bg %1
Send job 1 'sleep 100000000' to background

@mqudsi while rebasing your #9608 seems to have broken my PR.
Do you know how I can call the set_is_foreground on the Rust opaque type returned by C++?

Fixes issue #

TODOs:

• Changes to fish usage are reflected in user documentation/manpages.
• Tests have been added for regressions fixed
• User-visible changes noted in CHANGELOG.rst

[mqudsi]

I think you need to alias the type in an extern C++ section back to the underlying rust type to be able to use it interchangeably.

[clemenswasser]

I've tried to put the following above the include_cpp! invocation in ffi.rs:

#[cxx::bridge]
mod ffi {
    extern "C++" {
        type job_group_t = crate::job_group::JobGroup;
    }
}

But that results in the following error:

error[E0277]: the trait bound `job_group::JobGroup: cxx::ExternType` is not satisfied
   --> src/ffi.rs:21:14
    |
21  |         type job_group_t = crate::job_group::JobGroup;
    |              ^^^^^^^^^^^ the trait `cxx::ExternType` is not implemented for `job_group::JobGroup`
    |
    = help: the following other types implement trait `cxx::ExternType`:
              abbrs::abbrs_ffi::abbreviation_t
              abbrs::abbrs_ffi::abbrs_position_t
              abbrs::abbrs_ffi::abbrs_replacement_t
              abbrs::abbrs_ffi::abbrs_replacer_t
              autocxx::c_char16_t
              autocxx::c_int
              autocxx::c_long
              autocxx::c_longlong
            and 83 others
note: required by a bound in `cxx::private::verify_extern_type`
   --> /home/cwasser/.cargo/git/checkouts/cxx-703bae3d1c164327/24d1bac/src/extern_type.rs:185:30
    |
185 | pub fn verify_extern_type<T: ExternType<Id = Id>, Id>() {}
    |                              ^^^^^^^^^^^^^^^^^^^ required by this bound in `cxx::private::verify_extern_type`

In that trait's documentation example (https://docs.rs/cxx/latest/cxx/trait.ExternType.html) it seems like both Types must be extern "C++", but JobGroup is currently defined inside an extern "Rust" block?

[krobelus]

I think we need an explicit impl of ExternType for all types that we reuse across bridges
So

unsafe impl ExternType for JobGroup {
    type Id = type_id!("JobGroup");
    type Kind = cxx::kind::Opaque;
}

see also https://github.com/dtolnay/cxx/pull/465/files and dtolnay/cxx#942 (comment)

[clemenswasser]

Oh, thank you very much! Somehow I missed this cxx issue 😞

[clemenswasser]

This still doesn't seem to work for me. I've committed what I have tried so far. What am I missing?

[krobelus]

So the scenario is: JobGroup is a Rust object that lives in a C++ struct. We try to access it from Rust.
It has type &job_group_t which does not have the powers of the native Rust JobGroup.
I haven't encountered this scenario before (I'm porting dependencies first) so I don't have a recipe.

Maybe we can expose a raw pointer and cast that to a JobGroup (assuming the object is never moved or lives in shared_ptr or something)

Another, secondary problem is that get_jobs() returns SharedPtr which only allows read-only access.

[clemenswasser]

I've now fixed this with a unsafe cast. This should do the trick for now.

[krobelus]

    /// Mark whether we are in the foreground.
    pub fn set_is_foreground(&self, in_foreground: bool) {
        self.is_foreground.store(in_foreground, Ordering::Relaxed);
    }

How is this legal? Should be mut, no?

[mqudsi]

@krobelus it's a concept known as interior mutability. The &mut in rust is misleading. It's not "mutatable ref" vs "const ref" but rather "exclusive ref" vs "shared ref".

Interior mutability means making a method mutate the internal state in a way that's compatible with multiple instances of pointers/references to self being extant at the same time. You can't just modify whatever you need inside, instead you need to modify an object that abstracts around mutating its contents (such as Mutex, any of the Cell variants, or the atomic types).

I intentionally used internal mutability here to allow rw access to the JobGroup when multiple threads or objects hold live references to it at the same time. It's also why @clemenswasser is able to use the shared_ptr to mutate the JobGroup without needing to get a separate mutable reference.

[mqudsi]

@clemenswasser I think the reason your attempt at using the ExternType trait didn't work because you basically defined a second job_group_t in the ffi2 mod that was an alias for JobGroup but the job_group_t you were using was a different one auto-generated via the autocxx include/generate macros. I think we'd have to refrain from using autocxx for the types we need to alias back to their original rust counterparts in order for this to work, because it's using the naive cxx extern type without a way for us to specify that it needs to be aliased/pointed to the native rust type.

The only other option would be transmute from the dumb autocxx job_group_t to the "smart" cxx job_group_t but at that point you might as well transmute to the rust object and be done with it (which you did).