lang: Remove 3 lifetime definitions from Context - #3340
Conversation
|
@acheroncrypto is attempting to deploy a commit to the coral-xyz Team on Vercel. A member of the Team first needs to authorize it. |
|
It basically fixes the problem. It still requires a lifetime annotation, but we should be able to handle this automically in the
Yeah, but the alternative is worse (declaring another lifetime as explained in the "Details" section of #2770).
Does that even matter here? I don't think the Rust compiler would allow you to overwrite the data with a reference declared inside the instruction handler. |
With a normal struct, it is possible to change the lifetime to a smaller one by re-borrowing struct X<'a> {
s: &'a str,
}
fn cast<'a, 'b>(x: X<'a>) -> X<'b> where 'a: 'b {
X { s: &*x.s }
// or just `return x;`, rust can cast automatically
}but we can't do this with
|
Yeah, I'm aware of that, but I'm not sure how that answers my initial comment about the Rust compiler not allowing you to overwrite the data with a reference declared inside the instruction handler.
Since you're using "they" when referring to the Metaplex team, I'm assuming you're not from the Metaplex team, and you just want to interact with the If my assumptions are correct, then trying to fully upgrade that program is completely unnecessary for your use case, because you don't need the program's internal logic to be able to interact with a program (you only need implementation signatures). If you want to use the program's crate as a CPI client or an off-chain client, you can safely remove all its instruction handler logic, including the parts where it uses As a side note, the Furthermore, you don't even need to upgrade anything to interact with older programs if you're using the latest version (v0.30.1). In fact, you don't even need to add programs as a dependency. Here are some useful links: |
|
I am new to learning Anchor. How do you currently handle this issue? I am encountering same problem when trying to use |
|
This is a fantastic proposal, and a huge win for the developer experience in Anchor. I was initially cautious about the introduction of Removing the need for developers to manually juggle multiple lifetimes in the This is a high-impact, positive change. Excellent work, and I fully support merging this. |
|
Just some considerations: However, it seems the use of transmute to an anonymous lifetime '_ provides a strong mitigation here. The borrow checker should, in most safe code, correctly identify the shorter lifetime and prevent it from being assigned to a longer-lived variable. A developer would likely need to use another unsafe block to force such a leak, at which point they are already bypassing safety guarantees. Are there any other scenarios, perhaps involving complex CPIs, where this assumption could be violated in a less obvious way? From my perspective, this looks robust for the vast majority of use cases. |
|
@deanmlittle for second look |
febo
left a comment
There was a problem hiding this comment.
Looks good to me. Tried different ways to not use the unsafe block, but I could not find another way to do it – it is technically sound. I suggested to update the comment to be more clear about the transmute.
Co-authored-by: Fernando Otero <febo@anza.xyz>
9ba45fa to
3ba7588
Compare
|
The lifetime assumption during the transmute causes compile errors if the instruction doesn't have any accounts: I see two potential solutions here:
I'd be happy to know if I'm missing something and there is a better way to get around this problem. I think the latter option makes more sense because I don't feel comfortable letting the compiler do a blind transmute. The vast majority of instructions have accounts anyway. |
|
We can just introduce a lifetime-only transmute helper: unsafe fn transmute_lifetime<'to, T>(value: &mut T) -> &'to mut T {
::core::mem::transmute(value)
}in order to work generically. This is safe as T will not change, but could also be implemented as unsafe {
&mut *::core::ptr::from_mut(value)
}To extend the lifetime via pointer round tripping |
Wouldn't this just extend the lifetime of the mutable borrow (to
Isn't this essentially the same as the blind transmute option? |
Looking into this again - this actually works by laundering the lifetimes such that
No, there's no casting of the pointer. This just reborrows the same pointer as the same type but selects a new lifetime, disconnected from the original (so the same effect) |
|
@acheroncrypto we've decided to hold off on this change until 2.0. |
@jamie-osec That still sounds like they're essentially doing the same thing to me. I wanted to confirm this by looking at the generated binary and confirmed this PR (with both variants) has no effect on the binary (at least in the programs I tested). I don't see a reason why it would anyway, considering lifetimes literally don't exist in runtime. Did you mention "casting of the pointer" because you thought the compiler wouldn't be able to tell that this is actually the same value and same type and consequently optimize by omitting all pointer logic? If so, Rust seems to have grown up and not be as dumb anymore.
Not sure I follow. The return type
looks like it would still make it get recognized as mutably borrowed.
@jacobcreech looking at real world usage (e.g. This is a massive devex improvement with seemingly no impact on program binaries/runtime. If we're not comfortable including this because of the |
Yeah, the difference here is that it's a compile-time guard that ensures that we can't change the type accidentally, only the lifetime. The current method is probably fine, but I would personally be more comfortable constraining the types as much as possible when transmuting in macro-generated code.
(Highly UB example but just illustrative) The transmute helper returns a reference with a lifetime that is disjoint of the input, meaning we can mutate |
|
I added the lifetime-specific helper as an extra-paranoid guard, and added a small note on variance to the safety comment to make it clear why this shrinking isn't done automatically; we'll merge this for V1 |
e67fe7d to
045f0af
Compare
* lang: Remove 3 lifetime definitions from `Context` * tests: Fix remaining accounts * lang: Update the safety comment Co-authored-by: Fernando Otero <febo@anza.xyz> * tests: Remove extra lifetimes * lang: Remove the type annotations of the `transmute` * tests: Fix `auction-house` * chore: Use lifetime transmute helper and variance note --------- Co-authored-by: Fernando Otero <febo@anza.xyz> Co-authored-by: Jamie Hill-Daniel <jamie@osec.io>
| unsafe fn __shrink_lifetime<'from, 'to, T>(value: &'from mut T) -> &'to mut T { | ||
| unsafe { ::core::mem::transmute(value) } | ||
| } |
There was a problem hiding this comment.
I added the lifetime-specific helper as an extra-paranoid guard, and added a small note on variance to the safety comment to make it clear why this shrinking isn't done automatically
@jamie-osec I think the helper function clearly extends the mutable borrow lifetime instead of shortening the inner lifetime. The impl and its documentation is misleading currently.
What we want instead is something like this:
unsafe fn __shorten_invariant_lifetime<'a, 'info: 'a>(
value: &'a mut #accounts_struct_name<'info>,
) -> &'a mut #accounts_struct_name<'a> {
unsafe { ::core::mem::transmute(value) }
}* lang: Remove 3 lifetime definitions from `Context` * tests: Fix remaining accounts * lang: Update the safety comment Co-authored-by: Fernando Otero <febo@anza.xyz> * tests: Remove extra lifetimes * lang: Remove the type annotations of the `transmute` * tests: Fix `auction-house` * chore: Use lifetime transmute helper and variance note --------- Co-authored-by: Fernando Otero <febo@anza.xyz> Co-authored-by: Jamie Hill-Daniel <jamie@osec.io>
Problem
Contextstruct definition includes 4 lifetimes:https://github.com/coral-xyz/anchor/blob/ebbad72fc431fb726004d68c56270e88c869428d/lang/src/context.rs#L24
While this is technically correct, and it's also what Rust does by default, it results in a poor developer experience because the lifetimes leak to the Anchor users in various cases. For example, remaining accounts usage requires annotating the instruction handler with lifetimes, which is quite difficult to figure out for people who're less experienced with lifetimes (not to mention this is completely unnecessary).
Summary of changes
Remove 3 (out of 4) lifetimes from the
Contextstruct. In other words, make all references have the same lifetime. This makes it so much easier for Anchor users to to handle places thatContextis used. For example, remaining accounts usage:https://github.com/coral-xyz/anchor/blob/ebbad72fc431fb726004d68c56270e88c869428d/tests/misc/programs/remaining-accounts/src/lib.rs#L25-L27
simply becomes: