Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ABI generation fails with lifetimes #974

Closed
miraclx opened this issue Nov 24, 2022 · 7 comments · Fixed by #982
Closed

ABI generation fails with lifetimes #974

miraclx opened this issue Nov 24, 2022 · 7 comments · Fixed by #982
Labels
abi ABI related issues

Comments

@miraclx
Copy link
Contributor

miraclx commented Nov 24, 2022

Similar to #972.

#[near_bindgen]
impl MyType {
    pub fn slice<'a>(data: &'a String) -> &'a str { ... }
}
$ cargo build --features near-sdk/__abi-generate
   Compiling adder v0.1.0 (/Users/miraclx/git/near/near-sdk-rs/examples/adder)
error[E0261]: use of undeclared lifetime name `'a`
  --> src/lib.rs:3:17
   |
 3 |     pub fn slice<'a>(data: &'a String) -> &'a str {
   |                 -                          ^^ undeclared lifetime
   |                 |
   |                 help: consider introducing lifetime `'a` here: `<'a>`

For more information about this error, try `rustc --explain E0261`.```
@miraclx miraclx added the abi ABI related issues label Nov 24, 2022
@itegulov
Copy link
Contributor

It seems like support for lifetimes is intentional (https://github.com/near/near-sdk-rs/blob/master/near-sdk/compilation_tests/lifetime_method.rs). Not sure if there are legitimate patterns for it though. The best I could come up with is something like this:

pub fn foo<'a>(&self, data: &'a [u8; 32]) -> &'a [u8; 32] {
    data
}

@austinabell
Copy link
Contributor

It seems like support for lifetimes is intentional (master/near-sdk/compilation_tests/lifetime_method.rs). Not sure if there are legitimate patterns for it though. The best I could come up with is something like this:

pub fn foo<'a>(&self, data: &'a [u8; 32]) -> &'a [u8; 32] {
    data
}

yes, that's exactly the pattern. I think it's very niche, but if there is deserialized data that is very large and not ideal to move around, but you want to return a reference to part of it to serialize as the return, that would be valid.

An argument could definitely be made to remove this, though, but I don't see a reason to limit this functionality.

So it sounds like it's just the lifetime that causes issues, not using references for parameters or the return in any case?

@itegulov
Copy link
Contributor

itegulov commented Nov 29, 2022

Just a note that the existing lifetime support is limited to top-level lifetimes. For example this does not compile:

pub fn slice<'a>(data: Cow<'a, [u8; 32]>) -> Cow<'a, [u8; 32]> {
    data
}
$ cargo build --target wasm32-unknown-unknown --release
error[E0401]: can't use generic parameters from outer item
  --> src/lib.rs:46:32
   |
22 | #[near_bindgen]
   | --------------- lifetime `'a` is missing in item created through this procedural macro
...
46 |     pub fn slice<'a>(data: Cow<'a, [u8; 32]>) -> Cow<'a, [u8; 32]> {
   |                  --            ^^ use of generic parameter from outer item
   |                  |
   |                  lifetime parameter from outer item

@miraclx
Copy link
Contributor Author

miraclx commented Jan 21, 2023

I think lifetimes in argument position is pretty much invalid form. As part of the return type, which is more reasonable, it compiles just fine. So we can sanitize generics out of arguments and just report an error. But still, the question remains how to reconcile this with ABI generation.

@itegulov
Copy link
Contributor

@miraclx

I think lifetimes in argument position is pretty much invalid form.

Can you elaborate on why you think it is invalid form? The foo example above seems reasonable to me.

@miraclx
Copy link
Contributor Author

miraclx commented Jan 23, 2023

Well, really, my thought was: in the context of the code-generated contract API, reference to data that was deserialized just for you doesn't seem very useful. But yeah, the scope extends larger than this. Enough to be generalized, so it could technically be called by another method in the contract that only has a reference to pass to it, so that makes sense. You can ignore the original comment.

But, in that case, we should make an effort to resolve the issue you highlighted in #974 (comment).

Two options I've experimented with locally use TokenStream sanitization as an alternative to #982, like in #1001;

  • The first approach strips all lifetime patterns from type signatures.
    • &'a Cow<'b, [&'c u8; 32]> -> &Cow<[&u8; 32]>
  • While the second approach replaces all lifetime patterms with the implicit lifetime.
    • &'a Cow<'b, [&'c u8; 32]> -> &'_ Cow<'_, [&'_ u8; 32]>

.. both of which are equivalent to the compiler, and are sufficient for ABI generation. I'll submit both PRs for evaluation in a sec.

@miraclx
Copy link
Contributor Author

miraclx commented Jan 23, 2023

But yeah, the scope extends larger than this. Enough to be generalized, so it could technically be called by another method in the contract that only has a reference to pass to it, so that makes sense. You can ignore the original comment.

Honestly, I take that back. I think deserialized inputs to methods of the exported contract API should be received with full ownership. And if you need a method that you can either call from inside the contract or outside of it; you can keep the main impl private and just export a proxy that consumes ownership and passes a reference to the main impl.

#[near_bindgen]
impl Contract {
    fn _do_something(&self, value: &Value) -> Answer { }

    pub fn do_something(&self, value: Value) -> Answer {
        self._do_something(&value)
    }

    pub fn other_method_path(&self, other: OtherValue) -> OtherAnswer {
        self._do_something(&other.value()).other()
    }
}

We're already doing this implicitly, and it works for the trivial cases, but reconciling a complete solution to this issue with that implementation would be difficult.

This, for example, is how the input deserializer looks from the exported contract API:

  • fn method(&self, data: MyType)
  • fn method<'a>(&self, data: &'a MyType)
#[serde(Deserialize)]
struct Input {
    data: MyType,
}

simple enough for simple types, and one-level of lifetimes are elided

but for anything else, example: fn method<'a, 'b>(&self, data: Cow<'a, [&'b str; 32]>). You get this:

#[serde(Deserialize)]
struct Input {
    data: Cow<'a, [&'b str; 32]>,
}

And aside the fact that this is invalid because the lifetimes are undeclared, serde also doesn't know how to reconcile lifetimes in derives by default. So lifetime-bound fields would need to be annotated with #[serde(borrow)]

and that's just for the exported contract api, the extern helpers *Ext would also need to go through some transformation of its own, and then at ABI generation, we'll need to sanitize the lifetimes to allow ABI generation.

IMO, the best resolution is to treat contract API inputs as owned by default. And disallow the use of generics in argument position. All that's left would then be to sanitize lifetimes from the return type for ABI generation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi ABI related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants