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

fix: handle callbacks from functions without a return type in a special way #738

Merged
merged 8 commits into from
Mar 4, 2022

Conversation

itegulov
Copy link
Contributor

Fixes #729

I fixed this in a very straightforward way by extracting the first generic type for all #[callback_result] function parameters and checking if it is unit or not. This also adds a nice benefit of a compile time check that all #[callback_result] parameters have the right type.

But I am not super happy with how it turned out in the end. Since we do not have access to semantic information in macros there is no certain way to check whether the given type is actually the kind of Result we want. If a user was to type a parameter as core::result::Result or via a Result type alias, the macro would panic even though it shouldn't. If the user had something like

use anyhow::Result;

fn foo(#[callback_result] a: Result<String, PromiseError>)

it won't panic even though it should.

The other alternative would be to change our serialization logic to make all functions that don't have an explicit return type (which is a much easier check) to return "null" as their response, but I am not sure if it is a good idea to change how all already existing contracts behave and I will be introducing maybe a miniscule, but still overhead. So what do you guys think?

@itegulov itegulov force-pushed the daniyar/callback-unit-result branch from 3497ecd to a22248e Compare February 21, 2022 10:28
// that the byte array is either empty or contains the string "null".
syn::Type::Tuple(type_tuple) if type_tuple.elems.is_empty() => {
quote! {
near_sdk::PromiseResult::Successful(data) if data.is_empty() || data == vec!(110, 117, 108, 108) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
near_sdk::PromiseResult::Successful(data) if data.is_empty() || data == vec!(110, 117, 108, 108) =>
near_sdk::PromiseResult::Successful(data) if data.is_empty() || &data == b"null" =>

vec! will allocate, and we don't need this to check equality.

Also, null will be deserialized into (), so this code path isn't really necessary. Also, I'm pretty sure this will fail when using another serialization protocol, try it with borsh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good points. I made so that the flow goes through the regular deserialization process if the array is not empty, this should resolve all the weirdness you have pointed out.

@itegulov itegulov force-pushed the daniyar/callback-unit-result branch from 82a38a4 to 33e619b Compare February 28, 2022 06:41
@itegulov itegulov requested a review from austinabell March 2, 2022 21:58
Comment on lines 196 to 197
panic!("Function parameters marked with #[callback_result] should have \
type Result<T, PromiseError>")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that this doesn't create a compiler error with span info, so the error line will be on #[near_bindgen] like:

24 | #[near_bindgen]
   | ^^^^^^^^^^^^^^^
   |
   = help: message: Function parameters marked with #[callback_result] should have type Result<T, PromiseError>

but you can have this error be placed on the violating line with something like https://docs.rs/syn/latest/syn/struct.Error.html and converting it into a compile error.

Can be done later, not a big deal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, didn't know there were proper diagnostics in Rust macros

@itegulov itegulov merged commit 865c02c into master Mar 4, 2022
@itegulov itegulov deleted the daniyar/callback-unit-result branch March 4, 2022 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to gracefully handle a callback that doesn't have return value
2 participants