SpacetimeDB icon indicating copy to clipboard operation
SpacetimeDB copied to clipboard

C#: split table codegen logic from type codegen logic

Open RReverser opened this issue 1 year ago • 0 comments

Description of Changes

Over time, we've been adding more table features that affect base BSATN type generation as well.

Initially this was limited to BSATN.Codegen looking up any types marked with [SpacetimeDB.Table] - which was already an abstraction leak but not too bad on its own.

However, most recent example that made entangling even tighter is timer tables, where codegen needs to add extra fields to the type early in the pipeline, so that they'd be visible both to BSATN (de)serialization and to table column methods. Those extra fields had to be added separately to the type and table codegens and must be kept in sync.

This pushed the entangling over the line and, back in timer tables implementation, I left a TODO to instead subclass the table codegen from the type one, which is what I'm doing here.

After this change, the BSATN.Codegen's Type generator doesn't need to know anything about table and column attributes - it doesn't even need to look for [SpacetimeDB.Table] types anymore - and, instead, is only responsible for taking a list of fields and generating BSATN type methods, and only runs on types with the [SpacetimeDB.Type] marker.

On the other hand, Codegen's TableDeclaration now inherits from TypeDeclaration, adds those extra fields in its own constructor, parses column-specific attributes in ColumnDeclaration, and appends extra code in its own codegen. The combined type+table output for a table now ends up in a single file.

This change will make it easier to add table-specific features in the future without touching BSATN.Codegen altogether.

Note for reviewers: I recommend reviewing commit by commit and with whitespace hidden; I left detailed commit descriptions.

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

2 - the changes move some code around, so there's small risk of codegen breakage, but as long as snapshot tests don't show it, I'm fairly confident it works.

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.

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!

  • [x] dotnet test
  • [ ] Write a test you want a reviewer to do here, so they can check it off when they're satisfied.

RReverser avatar Aug 07 '24 13:08 RReverser