CodSpeed Performance Report

Merging #2943 will not alter performance

_{Comparing branchseer:lifetime_variance (3938ab0) with main (c250b28)}

Summary

✅ 30 untouched benchmarks

Are you working on something that requires borrow_* from ouroboros, or is this a soundness change?

I'm asking because I don't understand this topic, and need to know more about the context of this change.

Are you working on something that requires borrow_* from ouroboros, or is this a soundness change?

I'm asking because I don't understand this topic, and need to know more about the context of this change.

@Boshen Thank you for your time reviewing.

I was working on a side project using oxc and bumped¹ into this restriction. I just think borrow_* would be nice to have, although it's not essential for my case and I don't think this is a soundness change.

Also on other day I saw rolldown is also creating self-referencing ast types, so maybe this can make their life a little easier as well.

I am no expert to unsafe rust. But seeing both std's and allocator_api2's Box and Vec are lifetime covariant gives me some confidence about this change.

Thank you for working on this.

Merging because tests, rustc and miri all passed.

&mut T is invariant over T because it is borrowed and when borrow ends T would be observed in it original form.
If T could be coerced to shorter lifetime and overwritten, original value would contain value with shorter lifetime than type's.

Example.

fn break_it<'a, 'b: 'a>(a: &mut &'b Vec<i32>, b: &'a Vec<i32>) {
  let coerced_a: &mut &'a Vec<i32> = a; // Would be valid if `&mut T` covariant over `T`.
  *coerced_a = b;
}

let x = vec![1, 2, 3];
let mut y: &Vec<i32> = &x;
break_it(&mut y, &vec![]);
println!("{y:?}"); // use after free 💥

This is not an issue with Box because it is owned value, you can't observe its content when Box over type with shortened lifetime drops.

fn break_it<'a, 'b: 'a>(a: Box<&'b Vec<i32>>, b: &'a Vec<i32>) {
  let coerced_a: Box<&'a Vec<i32>> = a; // Would be valid if `&mut T` covariant over `T`.
  *coerced_a = b;
}

let x = vec![1, 2, 3];
let mut y: &Vec<i32> = &x;
break_it(Box::new(y), &vec![]);
println!("{y:?}"); // Doesn't compile, `y` is no more.

I'd like to replace Allocator copy in allocator_api2::boxed::Box with Deallocator that would be ZST for bumpallo case.
But it requires existence of Deallocator and my proposal for it left unattended :)

Weird, this is causing a stack over only in the code coverage run https://github.com/oxc-project/oxc/actions/runs/8660479790/job/23749372302

Weird, this is causing a stack over only in the code coverage run https://github.com/oxc-project/oxc/actions/runs/8660479790/job/23749372302

For the record, this PR increased the stack size just over the edge.

A RUST_MIN_STACK=104857600 cargo codecov fixes the issue.

The underlying cause is oxc_prettier recursively printing assignment expressions.

@branchseer Sorry to resurrect a long-dead issue, but can you please help me with something?

What is it that makes allocator_api2::vec::Vec covariant, whereas bumpalo::collections::Vec isn't? Looking at the source code of both, the only difference I can see is that the alloc field of RawVec is &'a Bump in bumpalo, whereas it's a generic A: Allocator in allocator_api2.

But in our case, the Allocator we use with allocator_api2::vec::Vec is a &'a Bump anyway. So what's the difference?

https://docs.rs/allocator-api2/0.2.21/src/allocator_api2/stable/raw_vec.rs.html#116-120
https://docs.rs/bumpalo/latest/src/bumpalo/collections/raw_vec.rs.html#53-57

And weirdly, neither of them includes a PhantomData<T> which std::vec::Vec does.

Are you able to shed any light?

@overlookmotel

What is it that makes allocator_api2::vec::Vec covariant, whereas bumpalo::collections::Vec isn't?

In branchseer@8bebc89 I swapped allocator_api2::vec::Vec with bumpalo::collections::Vec. The covariant test assertion passes. So I was wrong and bumpalo::collections::Vec is indeed covariant. Sorry for the confusion:)

And weirdly, neither of them includes a PhantomData which std::vec::Vec does.

std::vet::Vec doesn't use T in its pointer type Unique<u8>, so PhantomData<T> is mandatory. allocator_api2 and bumpalo's Vec already uses T in their pointer type NonNull<T>.

In branchseer@8bebc89 I swapped allocator_api2::vec::Vec with bumpalo::collections::Vec. The covariant test assertion passes. So I was wrong and bumpalo::collections::Vec is indeed covariant. Sorry for the confusion:)

OK great. Thanks for investigating, and resolving the mystery. Maybe you were right at the time and bumpalo changed something since last year. 🤷

std::vet::Vec doesn't use T in its pointer type Unique<u8>, so PhantomData<T> is mandatory. allocator_api2 and bumpalo's Vec already uses T in their pointer type NonNull<T>.

Ah ha. Yes, of course. I did not spot the difference NonNull<T> vs Unique<u8>.