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

Add a macro attribute for auto-unwrapping vals #9

Closed
wants to merge 22 commits into from
Closed

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Apr 10, 2022

What

Add a macro attribute for auto-unwrapping and rewrapping vals.

Why

So that we can write contracts that look like this:

#[contractfn]
pub fn add(a: i32, b: i32) -> i32 {
    return a + b;
}

That end up being converted into:

pub fn add(a : i32, b : i32) -> i32 {
    return a + b ;
}
#[no_mangle]
#[link_name="$add"]
fn contractfn_add(a : Val, b : Val) -> Val {
    return Val::try_from(add(a.try_into().or_abort(), b.try_into().or_abort())).or_abort();
}

cc @graydon
cc @paulbellamy

@leighmcculloch leighmcculloch marked this pull request as ready for review April 11, 2022 04:51
edition = "2021"

[lib]
proc-macro = true
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to cause rust-lang/rust#93988 on macos, but probably an insignificant issue.

@graydon
Copy link
Contributor

graydon commented Apr 11, 2022

Ah, I wish I'd caught this earlier. Apologies for slow uptake.

In general I've a fairly strong desire not to provide any macros in the SDK, especially procedural ones. Almost all the Rust SDKs for other smart contract system are "big piles of macros" and I think it's a very problematic approach:

  • Code footprint and compile time are strongly sensitive to macros, and while this one is "fairly small" they have a tendency to expand over time, become serious problems.
  • They tend to be used to paper-over bad interface designs, which are then left unfixed and then both macros and bad-interface become mandatory for everyone using the system.
  • IDEs, server-side cross reference tools, etc. are all unable to see through them.
  • Users are even further from "normal semantics" than they are with our already-weird custom library types / no_std situation; they are even more likely to have to guess-at code meaning / macroexpand-and-inspect code. It's a learning hazard, encouraging the user to "randomly try stuff until it works", hoping the macro has some special case to handle their input, rather than understanding what they're doing at least in terms of normal Rust semantics (which are perplexing enough -- eg. the trait system is quite complex and hard to reason about even in isolation).

In this case in particular, users are taken a step away from the dynamic Val type, which I actually want them to see and think about because:

  • Only some types are interconvertable with Val, and only some transparently so (others are try-conversions that might fail). Still other types -- almost all Rust types -- are not interconvertable at all, and the macro-expansion will succeed but produce an incomprehensible error about code the user didn't write.
  • The Val type encoding will be the basis for reasoning about contract-upgrade compatibility. Specifically: users using typed function signatures like this will certainly expect a change-of-type to a function argument to make a contract "statically incompatible" with a previous one, whereas at the macro-expanded code & wasm level it will not be. This will be extra confusing.

@graydon
Copy link
Contributor

graydon commented Apr 11, 2022

I should also point out that the example is a bit exaggerated. It would read:

fn contractfn_add(a : Val, b : Val) -> Val {
    return add(a.as_u32(), b.as_u32()).into();
}

@leighmcculloch
Copy link
Member Author

While experimenting in both rust and assemblyscript, writing wrapping encoders/decoders was annoying, and made it harder to read the contract code at hand. This was something @paulbellamy noticed as well. I don't think we should use macros everywhere, but if we use a macro to wrap the function, and then have the SDK otherwise expose only non-Val types, the developer will not need to interact with Vals.

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Apr 11, 2022

The alternative is we just encourage devs to write these functions manually, which is not horrible, as you pointed out because of the as_ functions. Maybe we should add .into functions?

@leighmcculloch
Copy link
Member Author

Closing in favor of writing these out by hand without a macro.

@paulbellamy
Copy link
Contributor

+1 on macros being bad. -1 on devs interacting with Val.

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.

3 participants