Allow ref (&) parameters in contract fns

[leighmcculloch]

What

Allow contract functions parameters to be immutable references, rather than only owned values.

Why

This change is primarily about making it more convenient to write contract functions like typical Rust functions. In Rust function parameters can be of a few variations, they can be an owned value or a reference/borrow, and can be immutable or mutable. Today contract function parameters must be owned and immutable.

Reference parameters are useful when needing to pass around a value that the caller would like to retain ownership over. Mutable parameters are helpful when the function will mutate the parameter making the mutated value available to the caller.

It is common when writing small composable Rust functions to use ref and mut as needed.

When writing contract functions the composability of the functions being written is restricted. Owned values must be used, and this leads to situations where functions must received cloned values.

In contracts cloning SDK types when passing them around is not a performance concern since all SDK types are a small integer value. But it is inconvenient and it makes contracts look less like regular Rust code when there is an abundance of .clone() for no apparent reason other than the contract functions don't let you.

Is this a big issue? No, not particularly. But in general the soroban-sdk tries to let people write Rust how they'd write Rust elsewhere. And this change lifts a limitation that isn't really required.

Why not

This change introduces variance into the trait-compatible interface that implementations can take. For a contract interface that is intended to be standard, one implementation might use all owned values, another might use all refs, and another might use any combination. This doesn't affect a contracts ability to be called because this information does not make it into the exported interface. But if someone was to share a Rust trait to match one implementation, there's a good chance it would fail to match other implementations. Technically variance already exists today. The env: Env parameter is already optional, and can already take either an owned Env or a ref &Env. So the ship already sailed from day one.

You could argue that the top-level entry point value in a program should always be owned. Someone has to own every value. The fact that contract functions do not need to own the value is because there's actually an outer function that gets exported that is truly the entry point and that function can own the value. But in some ways that's a bit odd.

Merging

This change can be merged at anytime and can be released in a minor release. It does not change or limit any existing behaviour and only adds new behaviour that is disallowed today.

Thanks

Thanks to @willemneal contributing #1521 that this change is derived.

Todo

• Test the change on soroban-examples. Add workflow to test with soroban-examples #1591
• Test the change on several complex ecosystem contracts.
• Test with Test Improvements #1544
• Remove warnings from mutable function arguments #1606

[willemneal]

Love the added mutability! One future idea from this and it might make things too complex, but it would be amazing if &mut Env would be able to determine whether a method could write to storage/publish events. I assume at this point it would be too much of a breaking change, but it would be a very nice way to enforce in the types what you are allowed to do in a method.

[leighmcculloch]

but it would be amazing if &mut Env would be able to determine whether a method could write to storage/publish events

Agreed, but I think that's a bigger thing to tackle, and the timing isn't right. Lots of complexity, and with unclear benefits. There's a discussion over here discussing the idea, but I am not chasing it in the near future:

• Read-only Invocations stellar-protocol#1454

[leighmcculloch]

Something I noticed out of this change is that I missed adding support for returning & on functions, which should be possible. I'll have a look at potentially expanding the change to support that, but might defer it to another PR or to never depending on what I find.

[dmkozh]

Copy is a marker trait that indicates that the value can be copied by bits. It requires that all internal components are also Copy. The Env contains the Host, which contains an Rc and so on. Every type would have to be Copy. Rc and many other types used internally are not.

Hmm, I see, yeah, it would make sense in the 'guest' mode, but it indeed seems infeasible to have it in the 'host' mode for tests, so probably it's not worth pursuing in general.

Something I noticed out of this change is that I missed adding support for returning & on functions, which should be possible.

What would be the lifetime of that reference? I'm not sure I understand how that could work in a non-confusing way, but I guess I'll wait until the change is made.

[leighmcculloch]

Discovered today that mut parameters are actually supported today, without the ref, although it has some issues captured in:

• Warnings are displayed on mut params in contract functions #1605

I'm blocking this change on the following change that addresses those so that this change is operating on a better foundation with a better test bas:

• Remove warnings from mutable function arguments #1606

[leighmcculloch]

maybe & and mut are alright, but I still really dislike the &mut as it's misleading for the reader

The pr now disallows &mut while continuing to allow & and mut.

Disallowing &mut required adding functionality. The changes to allow & inherently also allows &mut, so to disallow &mut an additional check must be inserted to create an error in that case.