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

Type: Symbol, etc: Cannot be used in match statements #198

Closed
leighmcculloch opened this issue Jul 12, 2022 · 11 comments
Closed

Type: Symbol, etc: Cannot be used in match statements #198

leighmcculloch opened this issue Jul 12, 2022 · 11 comments
Assignees

Comments

@leighmcculloch
Copy link
Member

leighmcculloch commented Jul 12, 2022

Symbol, and any other type we define as a TaggedVal or with a RawVal cannot be used in a match statement.

They can't be used in match patterns because the RawVal type is not #[derive(Eq, PartialEq)], and types must derive those traits to be used in pattern matching. It's not sufficient that we implement the traits manually, they must be derived.

This has an unfortunate affect that mapping we need to do on Symbol's and other primitive non-object RawVal/TaggedVal types must occur within a set of if blocks, which Rust does not optimize as well as it does match blocks.

For a small match block of only 3 conditions this causes a Symbol mapping to be an extra 37 bytes in compiled WASM. Given that we use Symbols as constants in enum encoding, this problem seems to be worth solving.

Related discussion: https://discord.com/channels/897514728459468821/996186142007369778

@leighmcculloch leighmcculloch linked a pull request Jul 12, 2022 that will close this issue
@leighmcculloch
Copy link
Member Author

Options:

  1. We derive Eq, PartialEq on RawVal.

  2. We stop aliasing TaggedVal for primitive types like Symbol, aka Make Symbol usage in match statements #197.

@graydon
Copy link
Contributor

graydon commented Jul 12, 2022

The underlying weirdness here is that rust marks derived-eq types as #[structural_match] since they are, by definition, treating equality as structural. There's a feature to let us mark our own types this way -- https://doc.rust-lang.org/unstable-book/language-features/structural-match.html -- but I'm not 100% sure we actually ought to be approaching this via match.

I don't think "doesn't optimize if as well as match" is exactly what's happening here; I think it's that structural equality is cheaper than the equality we have defined in our trait, and we might want to make it possible to access that from user code somehow. And possibly the right angle to do that is enabling compare-against-a-constant via constants-in-patterns. But I'm not sure. Need to think it over and play around a bit and I'm not sure it's the highest priority to get right just now.

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Jul 12, 2022

@graydon pointed out that as an interim solution we can use the RawVal::get_payload function and then do matches on it. This works sufficiently as an optimization in stellar/rs-soroban-sdk#230.

I'm going to keep this issue open because I think we should address this problem holistically. Specifically for contract developers who will not know that they need to call sym.to_raw().get_payload() to efficiently match on them.

@leighmcculloch
Copy link
Member Author

possibly the right angle to do that is enabling compare-against-a-constant

I was doing a compare against a constant with the if-else blocks and still seeing much more code being generated.

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Sep 13, 2022

@graydon fixed this recently 🎉. RawVal still cannot be used in matches but other types like Symbol can be. 🎉

@leighmcculloch
Copy link
Member Author

Doh. I was looking at the wrong code 🤦🏻 . This hasn't happened yet.

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Sep 21, 2022

Here's an example of where this is inconvenient:

      const DEPOSIT_RAW: u64 = symbol!("deposit").to_raw().get_payload();
      match function.to_raw().get_payload() {
          DEPOSIT_RAW => {

https://github.com/stellar/soroban-examples/pull/120/files#diff-ad9bb39cd40eef28cb328f2278447eb2eaf0ee3f7f6a24322a9d1e7ea5de1541R186-R188

@graydon
Copy link
Contributor

graydon commented Feb 13, 2023

So I think what I can do here is make the new small-value wrapper types (SymbolSmall and such) wrap a plain u64 rather than a RawVal (and I guess remove the AsRef/AsMut<RawVal> impls, assuming nobody's using those) and then we can do #[derive(Eq,PartialEq)] on each type done that way. Not beautiful but it ought to work. I'll move this under the value reform issue #679 to track.

@graydon
Copy link
Contributor

graydon commented Feb 27, 2023

I should note that this will only work with SmallSymbol rather than general Symbol -- the latter wraps possibly-host-values that can't be shallow-compared for equality at all. Is it still worth doing, given that?

@graydon
Copy link
Contributor

graydon commented Mar 13, 2023

(postponing this for now -- will discuss whether it's worth it on the small-value cases with @leighmcculloch when he's back)

@leighmcculloch
Copy link
Member Author

It's not possible afaik to do match on Symbol now that Symbol's are no longer constant values. Symbols can be handles to host data, and therefore cannot be constant.

@leighmcculloch leighmcculloch closed this as not planned Won't fix, can't repro, duplicate, stale Jun 21, 2023
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 a pull request may close this issue.

2 participants