Many Context methods return types with unnecessarily short lifetimes

[programmerjake]

In:

inkwell/src/context.rs

Line 950 in f768691

pub fn const_string(&self, string: &[u8], null_terminated: bool) -> VectorValue {

the lifetimes are left implicit, which unnecessarily shortens the lifetime of the return value. Making the lifetimes explicit:

impl<'ctx> Context<'ctx> {
    pub fn const_string<'a>(&'a self, string: &'a [u8], null_terminated: bool) -> VectorValue<'a> {
        ...
    }
}

The problem is that VectorValue<'a> should actually live as long as 'ctx (or at least as long as self), instead of being shortened to the lifetime of string, since string is internally cloned rather than borrowed.

Example:

fn f<'a, 'ctx>(ctx: &'a Context<'ctx>, v: i32) -> VectorValue<'a> {
    ctx.const_string(v.to_string().as_bytes(), false) // should compile, but currently doesn't due to the above problem
}

There are several other functions with similar lifetime issues, such as Context::ptr_sized_int_type

[TheDan64]

impl<'ctx> Context<'ctx> {
    pub fn const_string<'a>(&'a self, string: &'a [u8], null_terminated: bool) -> VectorValue<'a> {
        ...
    }
}

Context<'ctx> isn't a thing. Is this a typo?

I think fn const_string<'a>(&'a self, string: &'a [u8], null_terminated: bool) -> VectorValue<'a> is identical to fn const_string(&self, string: &[u8], null_terminated: bool) -> VectorValue via lifetime elision (edit: nvm, this was your point)

You may have actually meant fn const_string<'ctx>(&'ctx self, string: &[u8], null_terminated: bool) -> VectorValue<'ctx>? This might actually disentangle 'ctx from the lifetime of the input slice, which I think is what you're getting at?

[programmerjake]

Context<'ctx> isn't a thing. Is this a typo?

Yeah, oops.

You may have actually meant fn const_string<'ctx>(&'ctx self, string: &[u8], null_terminated: bool) -> VectorValue<'ctx>? This might actually disentangle 'ctx from the lifetime of the input slice, which I think is what you're getting at?

Yup!

[TheDan64]

Ok, then I am in favor of this change. Good catch

[TheDan64]

@programmerjake

fn f<'ctx>(ctx: &'ctx Context, v: i32) -> VectorValue<'ctx> {
    ctx.const_string(v.to_string().as_bytes(), false)
}

fn f<'ctx>(ctx: &'ctx Context, v: &[u8]) -> VectorValue<'ctx> {
    ctx.const_string(v, false)
}

fn f<'a, 'ctx>(ctx: &'ctx Context, v: &'a [u8]) -> VectorValue<'ctx> {
    ctx.const_string(v, false)
}

all compile fine as is. Do you have an actual non-compiling example I can verify changes against?

[programmerjake]

ah, turns out i misunderstood the lifetime elision rules:
https://doc.rust-lang.org/nomicon/lifetime-elision.html

Elision rules are as follows:

• Each elided lifetime in input position becomes a distinct lifetime
  parameter.

• If there is exactly one input lifetime position (elided or not), that lifetime
  is assigned to all elided output lifetimes.

• If there are multiple input lifetime positions, but one of them is &self or
  &mut self, the lifetime of self is assigned to all elided output lifetimes.

• Otherwise, it is an error to elide an output lifetime.

The first and third rules mean the function signature actually becomes:

fn const_string<'a, 'b>(&'a self, string: &'b [u8], null_terminated: bool) -> VectorValue<'a>