Port types returning &'static references is unsound

[prokopyl]

As of now, all port types return &'static references to their contents, which allows them to be stored outside of the run() function, at which point the host could invalidate them (the following code compiles fine today):

struct Amp {
    foo: Option<&'static [f32]>
}

struct Ports { input: InputPort<Audio> }

impl Plugin for Amp {
    fn run(&mut self, ports: &mut Ports, _features: &mut (), _: u32) {
        self.foo.replace(&*ports.input); // Uh-oh
    }
}

While this example is rather obvious in that it's doing something very odd, issues can be viciously subtle with more complex port types such as Atom: one could want to store a deserialized value.

I haven't made any in-depth testing, but I see two solutions to this:

The first would be to make PortType generic over a 'a lifetime:

pub trait PortType<'a> {
    type InputPortType: Sized + 'a;
    type OutputPortType: Sized + 'a;
}

impl<'a> PortType<'a> for Audio<'a> {
    type InputPortType = &'a [f32];
    type OutputPortType = &'a mut [f32];
}

However, this has a cascading effect, which would force all downstream users of port types to specify their lifetimes:

#[derive(PortCollection)]
struct Ports<'a> {
    gain: InputPort<'a, Control<'a>>,
    input: InputPort<'a, Audio<'a>>,
    output: OutputPort<'a, Audio<'a>>,
}

The second option would be to make the associated types themselves generic:

pub trait PortType {
    type InputPortType<'a>: Sized;
    type OutputPortType<'a>: Sized;
}

impl PortType for Audio {
    type InputPortType<'a> = &'a [f32];
    type OutputPortType<'a> = &'a mut [f32];
}

However, this would require Generic Associated Types being stabilized, but while it seems to be making steady progress, there doesn't seem to be any deadline coming soon.

Both options are breaking changes however.

I think we could potentially use the first solution now, and move to GATs when they are stabilized, making two breaking changes.

[prokopyl]

Oh, and of course, the day right after I open this issue, the Rust Team wants to push GATs for stabilization. :upside_down_face:

Jokes aside, that's actually pretty good news. I think we could keep the unsoundness temporarily (it's been there for a while after all), and then push a breaking change using GATs when they are stabilized.

Perhaps we could also start a nightly branch that uses GATs to try them out right now?

[PieterPenninckx]

rsynth has a very similar problem. The current version of rsynth solves this by not giving the API user the option to choose the buffer type (i.e.: it is not generic over the buffer type). I had to develop the vecstorage crate for this purpose, however.

LV2's current approach is much more ergonomic, however. I've been thinking about how one could solve this problem (in the context of rsynth) and I think I've found a solution. I'm copy-pasting what I have found:

// This struct should be auto-generated with a macro, based on the buffer data type that the API user wants.
struct MyStereoBuilder {
    in_left: Port<AudioIn>,
    in_right: Port<AudioIn>,
    out_left: Port<AudioOut>,
    out_right: Port<AudioOut>,
}

// This struct should be auto-generated with a macro, based on the buffer data type that the API user wants.
struct MyStereoInputOutput<'a> {
    in_left: &'a [f32],
    in_right: &'a [f32],
    out_left: &'a mut [f32],
    out_right: &'a mut [f32],
}

// This impl block should be auto-generated with a macro, based on the buffer data type that the API user wants.
// Some context: `Client` is a data type exposed by the `jack` crate.
impl<'c> TryFrom<&'c Client> for MyStereoBuilder {
    type Error = Error;

    fn try_from(client: &'c Client) -> Result<Self, Self::Error> {
        todo!();
    }
}

// This is the trait that the plugin (the API user needs to implement).
// This is part of `rsynth`.
pub trait NewContextualAudioRenderer<B, Context> {
    fn render_buffer(&mut self, buffer: B, context: &mut Context);
}

// This should be auto-generated by a macro.
// The definition of `DelegateHandling` is omitted.
impl<'a, P> DelegateHandling<P, (&'a Client, &'a ProcessScope)> for MyStereoBuilder
where
    for<'b, 'c, 'mp, 'mw> P:
        NewContextualAudioRenderer<MyStereoInputOutput<'b>, JackHost<'c, 'mp, 'mw>>,
{
    type Output = Control;

    fn delegate_handling(
        &mut self,
        plugin: &mut P,
        (client, process_scope): (&'a Client, &'a ProcessScope),
    ) -> Self::Output {
        let mut jack_host: JackHost = todo!();

        let buffer : StereoInputOutput  = todo!();
        plugin.render_buffer(buffer, &mut jack_host);
        jack_host.control
    }
}

// Generic code, part of `rsynth`.
struct DemoJackHandler<B, P> {
    builder: B,
    plugin: P,
}

// Generic code, part of `rsynth`.
// Context: ProcessHandler is the trait that the `jack` crate requires to implement.
impl<B, P> ProcessHandler for DemoJackHandler<B, P>
where
    for<'a> B: DelegateHandling<P, (&'a Client, &'a ProcessScope), Output = Control>,
    B: Send,
    P: Send,
{
    fn process(&mut self, client: &Client, process_scope: &ProcessScope) -> Control {
        self.builder
            .delegate_handling(&mut self.plugin, (client, process_scope))
    }
}

// Generic code, part of `rsynth`.
pub fn run2<P, B>(mut plugin: P) -> Result<P, jack::Error>
where
    P: CommonPluginMeta + AudioHandler + Send + Sync + 'static,
    for<'b, 'c, 'mp, 'mw> P:
        NewContextualAudioRenderer<StereoInputOutput<'b>, JackHost<'c, 'mp, 'mw>>,
    for<'a> B: DelegateHandling<P, (&'a Client, &'a ProcessScope), Output = Control>,
    for<'a> B: TryFrom<&'a Client, Error = jack::Error>,
    B: Send + 'static,
{
    let client: Client = todo!();

    let stereo_builder = B::try_from(&client)?;

    let demo_handler = DemoJackHandler {
        plugin: plugin,
        builder: stereo_builder,
    };

    let active_client = client.activate_async((), demo_handler)?;

    todo!();
}

The idea is that there are two types associated to the buffer:

1. The buffer type itself (MyStereoInputOutput above)
2. A "builder" type (that has a static lifetime) (MyStereoBuilder above)

For each audio chunk to be rendered, the builder type is asked to do the rendering. The builder type first constructs an instance of the buffer type, based on the data exposed by the host. The builder type then passes this buffer to the plugin to do the actual rendering.

I'm currently writing macros-by-example to generate the code that should be auto-generated. I hope that when this is finished, it can serve as an example for lv2.

[PieterPenninckx]

Ok, the comment above is probably not 100% clear (I wrote it in a rush). The gist is that rsynth has a similar problem and that I've found a way to solve it (and that doesn't require GAT's). I'm thinking of the following approach:

1. I finish the implementation in rsynth and check if it works.
2. I try to more clearly explains how it works; then we can check if this approach would fit for LV2 as well.
3. If it works and seems to be a good fit for LV2, we can see how we can implement it for LV2 as well.

[YruamaLairba]

Lol, i tried to look how to bind a port to a lifetime and I just realized i partially addressed this issue when i rewritten Dereferentation of ports. It seems to prevent all this kind of pattern, but sometime it's seems to be prevented through a side effect, which is bad.

The correct solution would be to bind the appropriate lifetime to a port, but i think the cascading effects may even worst that one already mentioned, even GAT may not help.