clockworklabs / SpacetimeDB

Multiplayer at the speed of light
https://spacetimedb.com
Other
4.12k stars 100 forks source link

Switch to a better API for tagged enums for C# #1177

Closed RReverser closed 6 days ago

RReverser commented 2 weeks ago

Description of Changes

With C# records - which are available since C# 9, so covers Unity requirements as well - we can use subclassing and pattern matching to get sum types that look a lot more like Rust tagged enums than my previous approach.

For example, the following Rust code:

let foo = match some_tagged_value {
  MyEnum::A(x) => ...,
  MyEnum::B(y) => ...
};

can now be expressed via almost equivalent C# code

var foo = someTaggedValue switch {
  MyEnum.A(var x) => ...,
  MyEnum.B(var y) => ...,
  // main difference is that, unlike in Rust, they're not exhaustive so you need to handle edge cases yourself
  _ => throw new ArgumentException(...)
};

This is a breaking change but IMO worth it for the better API going forward.

API and ABI breaking changes

If this is an API or ABI breaking change, please apply the corresponding GitHub label.

Expected complexity level and risk

How complicated do you think these changes are? Grade on a scale from 1 to 5, where 1 is a trivial change, and 5 is a deep-reaching and complex change.

2

This complexity rating applies not only to the complexity apparent in the diff, but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR, and what other components it interacts with in potentially concerning ways.

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do, so that you're confident that all the changes work as expected!

kazimuth commented 2 weeks ago

Oh this is extremely nice but it is going to be a huge patch for BitCraft right? How many enums are they using?

These records will also allocate, record struct isn't supported until C# 10 :/ I think the niceness of the API is worth the potential perf hit, we would have to profile to see if there was a serious difference.

Also note we can't use Unity serialization with records according to this: https://docs.unity3d.com/Manual/CSharpCompiler.html I'm not sure if this will be a problem.

RReverser commented 2 weeks ago

Oh this is extremely nice but it is going to be a huge patch for BitCraft right? How many enums are they using?

No, because this affects only server-side module SDK (for now), and their modules are written in Rust.

These records will also allocate, record struct isn't supported until C# 10 :/

Even then, you can't use record struct for this because they aren't allowed to be subclassed unfortunately, and subclassing is at the core of this approach. I don't think it will cause much of a hit though.

RReverser commented 2 weeks ago

Also note we can't use Unity serialization with records according to this: docs.unity3d.com/Manual/CSharpCompiler.html I'm not sure if this will be a problem.

As for this, I'm not sure either. Given that Unity / client C# code in general doesn't support tagged enums altogether for now, this is at least not a breaking change, but we can return to this question in the future after we integrate it into the client SDK.

kazimuth commented 2 weeks ago

Oh perfect, no complaints from me then.