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

Rewrite C# codegen to the new Lang infra + fixes #2184

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RReverser
Copy link
Member

@RReverser RReverser commented Jan 28, 2025

Description of Changes

Rewrites C# codegen to the V9 infra and fixes some issues & moves common cross-language utils in the process.

Fixed issues:

Detailed changelog:

  • Rewrites C# codegen to the V9 module definition and removes the legacy GenCtx infrastructure.
  • Splits C# output into individual Types/..., Tables/..., Reducers/... files and uses partial classes to split out almost all entity-specific code into those files instead of the global SpacetimeDBClient.cs. This should help reduce diffs significantly.
  • Moves iter_reducers, iter_tables, iter_types into the shared generate/utils.rs module. Their functionality was duplicated in each codegen otherwise, and (re)using those helpers ensures that all codegens iterate over entities in the same deterministic order.
  • Updates iter_reducers helper to exclude the "init" reducer as no codegen should ever generate client-side code for those.
  • Updates deletion of auto-generated files to walk the output folder recursively. Codegens could emit files in nested folders already, but after the change above this is utilised a lot more extensively, and top-level iteration would keep a lot of obsolete files.
  • Optimises said auto-generated file detection by only reading the prefix of each file, instead of the entire contents.
  • Adds is_reducer_invokable helper that tells whether an invocation should be generated for this reducer to ensure that we skip lifecycle reducers (currently used in C# and TypeScript, left Rust TODO for someone else as it's a more involved refactoring).
  • Renamed on_connect in rust-wasm-test to client_connected as the former name should be illegal and it's a bug that it's currently accepted by Rust. (Rust reducers must disallow reducers starting with reserved prefix #2175)
  • Adds format_files and possible_values() -> clap::builder::PossibleValues to the Lang trait, so that generate/mod.rs doesn't contain any language-specific functionality.
  • Adds dotnet format whitespace-based implementation for the C# spacetime generate output.
  • Removes InternalInvokeValueInserted and InternalInvokeValueDeleted methods that were "internal" in name only, but actually exposed to the users in generated code. Instead, now using truly private table events for index updates.
  • Moves common unique index and btree index code into base classes in C# SDK to reduce amount of generated code.
  • Removes <auto-generated /> tag from generated C# code, instead using .g.cs extension which is equivalent from C# compiler point of view, but should be a clearer signal to the users and makes it easier to e.g. ignore those files in Github diffs.
  • Updates regen-csharp-moduledef and regenerates server-side bindings.
  • Adds partial support for multi-column indices to C# (supports direct comparisons, but not BTree ranges).
  • Move namespace into the Csharp implementation. We already rejected it for other languages during parsing, so it only makes sense to remove it from the Lang trait and move into Csharp struct only. This should make generate/mod.rs even more language-agnostic.

Note that this PR will require C# SDK changes as well, companion PR here: clockworklabs/com.clockworklabs.spacetimedbsdk#220.

API and ABI breaking changes

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

In this PR, I'm no longer preserving the support for #[sats(name = "Foo.Bar")] - see #2174. This was a syntax that we preserved essentially only for BitCraft and only in C# codegen, but BitCraft team confirmed they can migrate away from it easily, and having consistent type names - regardless of whether it's a product or sum type, and regardless of whether it's C# or another language output - is more valuable for 1.0.

You can see the result in diffs, but to recap, before:
#[derive(SpacetimeType)]
#[sats(name = "Namespace.TestC")]
pub enum TestC {
    Foo,
    Bar,
}

would generate

in Rust:

#[derive(__lib::ser::Serialize, __lib::de::Deserialize, Clone, PartialEq, Debug)]
#[sats(crate = __lib)]
pub enum NamespaceTestC {
    Foo,

    Bar,

}

in TypeScript:

export namespace NamespaceTestC {
  // These are the generated variant types for each variant of the tagged union.
  // One type is generated per variant and will be used in the `value` field of
  // the tagged union.
  export type Foo = { tag: "Foo" };
  export type Bar = { tag: "Bar" };
  ...
}

but in C#:

namespace SpacetimeDB
{
	public partial class Namespace
	{
		public partial class Types
		{
			[SpacetimeDB.Type]
			public enum TestC
			{
				Foo,
				Bar,
			}
		}
	}
}

After this change, it generates the same name as any other type in any other language:

namespace SpacetimeDB
{
    [SpacetimeDB.Type]
    public enum NamespaceTestC
    {
        Foo,
        Bar,
    }
}

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.

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.

3 - some diffs are large, although a lot of changes are relatively trivial, e.g. moving functions to other modules or splitting flat generated code into Lang methods. Still, some extra care might be good, especially with testing the C# parts and V8 -> V9 upgrade has a lot of subtle differences.

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!

  • I've been running C# SDK tests with both .NET test and Unity tests, with files regenerated from this implementation.
  • Write a test you want a reviewer to do here, so they can check it off when they're satisfied. Would be good to do extensive BitCraft testing as well.

@RReverser RReverser added bugfix Fixes something that was expected to work differently api-break A PR that makes an API breaking change labels Jan 28, 2025
@RReverser RReverser requested a review from kazimuth January 28, 2025 17:19
Copy link
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

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

I'm signing off on my one codeowned file crates/cli/src/subcommands/generate/mod.rs. The changes are in the part(s) of the file that are codegen-related more than CLI-related, so they weren't really meant to be covered by my codeownership (but the CLI-related parts of this file were, which aren't changed in this PR).

I didn't review the rest of the PR, someone else should review + be the real approver.

Copy link
Contributor

@kazimuth kazimuth left a comment

Choose a reason for hiding this comment

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

Way cleaner overall. Very happy with this.

crates/cli/src/subcommands/generate/util.rs Outdated Show resolved Hide resolved
@@ -142,7 +142,7 @@ jobs:
uses: actions/checkout@v4
with:
repository: clockworklabs/spacetimedb-csharp-sdk
ref: staging
ref: ingvar/csharp-codegen-rewrite # TODO: revert to `staging` before merging
Copy link
Contributor

Choose a reason for hiding this comment

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

resolve this comment when you do this

crates/cli/src/subcommands/generate/csharp.rs Show resolved Hide resolved
crates/cli/src/subcommands/generate/csharp.rs Show resolved Hide resolved
crates/cli/src/subcommands/generate/csharp.rs Show resolved Hide resolved
@RReverser RReverser force-pushed the ingvar/csharp-new-codegen branch from fcecb13 to b31388a Compare January 30, 2025 02:23
@rekhoff
Copy link

rekhoff commented Jan 31, 2025

I ran into errors after testing BitCraft with the new SDK from this branch.

Testing steps:

  1. Setup:
    Using the latest from SpacetimeDB repo's Master branch, with freshly built:
    spacetimedb-standalone
    spacetimedb-cli
    spacetimedb-update

  2. Followed getting started guide until "Generate a World section":
    https://github.com/clockworklabs/BitCraftClient/wiki/Getting-Started
    Note, to publish the server, the following was used:
    spacetimedb-cli publish -c -y bitcraft

  3. Once in Unity, to setup the world:

  • From BitCraft menu, set World Defintion to Spacetime20x20
  • Under "Upload everything" click Worldgen World
  • If asked, "Cache Everything"
  • Once the Queued Messages reaches 0:
  • In the Scene, _Project > Scenes > Main you can hit Play
  • Play the game and perform your tests

Tip: To skip tutorial, Use Shift+O
Tip: Once in the world, you'll need the Ruined Town to start. Spawn it while in game by going to:
Bitcraft > I'm not cheating, you're cheating > Place Ruined Town

Tests to perform (initially using the up-to-date repo SDK):

  1. Publish the bitcraft module
  2. Use the client to:
  • login
  • walk around
  • do the tutorial
  • build buildings
  • mine some resources
  • mess around with inventory
  • send a chat message
    Result: Success, game loads and all actions can be completed
  1. Remove the old SpacetimeDB SDK from Package Manager, add the new "ingvar/csharp-new-codegen" repo SDK, and check console for errors:
    The location of the local SDK package from the cloned sdk repo is:
    com.clockworklabs.spacetimedbsdk\package.json

Result:

  • 976 errors of type "CS0246: The type or namespace name 'IDatabaseRow' could not be found"
  • 244 errors of type "CS0534: 'RemoteTables.PlayerDeathTimerHandle' does not implement inherited abstract member 'RemoteTableHandle<EventContext, PlayerDeathTimer>.Name.get'"
    Note: "PlayerDeathTimerHandle" and "PlayerDeathTimer" varried for each error.
  • 74 errors of type "CS0311: The type 'BitCraft.Spacetime.BuffDesc' cannot be used as type parameter 'T' in the generic type or method 'StaticDataScriptableObject'. There is no implicit reference conversion from 'BitCraft.Spacetime.BuffDesc' to 'IDatabaseRow'."
    Note: "BuffDesc" varried for each error.
  • 1 error of type "CS0314: The type 'T' cannot be used as type parameter 'T' in the generic type or method 'MultiStaticDataScriptableObject'."
  • 1 error of type "CS0308: The non-generic type 'RemoteBase' cannot be used with type arguments"
  • 1 error of type "CS0305: Using the generic type 'DbConnectionBase<DbConnection, Tables, Reducer>' requires 3 type arguments"
  1. Regenerate the server defintions with
    spacetimedb-cli generate --lang csharp --out-dir .\BitCraftClient\Assets_Project\autogen --project-path .\BitCraft\packages\game --yes
    Result:
    More errors. All errors from SpacetimeDBClient.cs went away, but other errors took their place.

@RReverser
Copy link
Member Author

RReverser commented Jan 31, 2025

Using the latest from SpacetimeDB repo's Master branch, with freshly built:

You need to use SpacetimeDB built from this branch, not from master, as the codegen has changed.

4. Regenerate the server defintions with

You also definitely need to do this first. Testing newer SDK with older codegen is not possible and won't result in meaningful diagnostics, so those errors above can be ignored.

@rekhoff
Copy link

rekhoff commented Jan 31, 2025

Re-tested above steps using branches:

https://github.com/clockworklabs/com.clockworklabs.spacetimedbsdk/tree/ingvar/csharp-codegen-rewrite @ 5b21810
https://github.com/clockworklabs/SpacetimeDB/tree/ingvar/csharp-new-codegen @ 9b3b5d1
https://github.com/clockworklabs/BitCraft @ 4afb21b
https://github.com/clockworklabs/BitCraftClient @ 435ce33

Unity Error logs:
unity_error_log_sdk-5b21810_sdb-9b3b5d1.txt

After regeneration, Unity still reports 999+ errors. Reviewing them:

  • 534 of error CS0426
    Using example:
    Assets\_Project\Scripts\GameSystems\Audio\Parameters\Providers\BiomeMusic.cs(4,45): error CS0426: The type name 'Types' does not exist in the type 'TerrainCell'
    Later in the same file, Biome is unable to be resolved.

In BiomeMusic.cs at line 4, reads using static BitCraft.Spacetime.TerrainCell.Types;
Previously this "Types" was defined in the autogen file TerrainCellBiomes.cs
The same file is now generated:

// THIS FILE IS AUTOMATICALLY GENERATED BY SPACETIMEDB. EDITS TO THIS FILE
// WILL NOT BE SAVED. MODIFY TABLES IN YOUR MODULE SOURCE CODE INSTEAD.

#nullable enable

using System;

namespace SpacetimeDB.Types
{
    [SpacetimeDB.Type]
    public enum TerrainCellBiome
    {
        Dev,
        CalmForest,
        PineWoods,
        SnowyPeaks,
        BreezyPlains,
        AutumnForest,
        Tundra,
        Desert,
        Swamp,
        Canyon,
        Ocean,
        SafeMeadows,
        Cave,
    }
}

This can be resolved by removing the using static BitCraft.Spacetime.TerrainCell.Types; line and replacing Biome with TerrainCellBiome
I am still evaluating the other errors.

@RReverser
Copy link
Member Author

Yeah all of those are generated from #[sats(...)] and will need search-replace in BitCraft code, see this section of the description:

In this PR, I'm no longer preserving the support for #[sats(name = "Foo.Bar")] - see #2174. This was a syntax that we preserved essentially only for BitCraft and only in C# codegen, but BitCraft team confirmed they can migrate away from it easily, and having consistent type names - regardless of whether it's a product or sum type, and regardless of whether it's C# or another language output - is more valuable for 1.0.

@rekhoff
Copy link

rekhoff commented Jan 31, 2025

Thanks @RReverser. It also appears that the 1685 reports of error CS0246 from namespaces changing in types from:
namespace BitCraft.Spacetime to namespace SpacetimeDB.Types
Because BitCraft relies on custom additions to the partial classes generated by SpacetimeDB, this is creating ambiguity for resolving terms. The real issue though, is that what used to be a single class, is now split into two namespaces.
Currently generated HealthState.cs in BitCraftClient uses the name BitCraft.Spacetime namespace
Non-generated HealthState.cs in BitCraftClient uses the name BitCraft.Spacetime namespace
Newly generated HealthState.g.cs uses SpacetimeDB.Types namespace reads:

// THIS FILE IS AUTOMATICALLY GENERATED BY SPACETIMEDB. EDITS TO THIS FILE
// WILL NOT BE SAVED. MODIFY TABLES IN YOUR MODULE SOURCE CODE INSTEAD.

#nullable enable

using System;
using System.Collections.Generic;
using System.Runtime.Serialization;

namespace SpacetimeDB.Types
{
    [SpacetimeDB.Type]
    [DataContract]
    public sealed partial class HealthState
    {
        [DataMember(Name = "entity_id")]
        public ulong EntityId;
        [DataMember(Name = "last_health_decrease_timestamp")]
        public ulong LastHealthDecreaseTimestamp;
        [DataMember(Name = "health")]
        public float Health;
        [DataMember(Name = "died_timestamp")]
        public int DiedTimestamp;

        public HealthState(
            ulong EntityId,
            ulong LastHealthDecreaseTimestamp,
            float Health,
            int DiedTimestamp
        )
        {
            this.EntityId = EntityId;
            this.LastHealthDecreaseTimestamp = LastHealthDecreaseTimestamp;
            this.Health = Health;
            this.DiedTimestamp = DiedTimestamp;
        }

        public HealthState()
        {
        }
    }
}

It may be that the BitCraftClient-specific partial classes in the BitCraftClient would also need to have there namespaces updated from BitCraft.Spacetime to SpacetimeDB.Types. This might make the most sense, as these BitCraftClient-specific partial class definitions typically contain no using statements, and where likely meant to just add additional functionality to classes generated by SpacetimeDB.

@rekhoff
Copy link

rekhoff commented Jan 31, 2025

Looks like there was an issue during generation. Looks like generation should have included the --namespace=BitCraft.Spacetime. There are still a lot of issues, but at least the namespace one should no longer be a factor.

@RReverser
Copy link
Member Author

Newly generated HealthState.g.cs uses SpacetimeDB.Types namespace reads:

You need to use --namespace in spacetime generate to customize it to the one BitCraft uses. You might want to talk to either @kazimuth or someone from the BitCraft team to help guide you further :)

@rekhoff
Copy link

rekhoff commented Feb 1, 2025

Regenerating using namespace=BitCraft.Spacetime resolve all but around 125 issues, attached:
unity_error_log2_sdk-5b21810_sdb-9b3b5d1.txt

Working through the issues as described:
Clearing error CS0426 (count 11):
BiomeMusic.cs,
GroundMaterial.cs,
Remove using static BitCraft.Spacetime.TerrainCell.Types;
Biome should be TerrainCellBiome

MovementSpeed.cs,
TerrainChunk.cs,
Helpers\MovementSpeed.cs,
Remove using static BitCraft.Spacetime.TerrainCell.Types;
SurfaceType should be TerrainCellSurfaceType

WorldUploader.cs:
Remove using static BitCraft.Spacetime.TerrainCell.Types;
SurfaceType should be TerrainCellSurfaceType
Biome should be TerrainCellBiome

DeployableValidator.cs:
Remove using static BitCraft.Spacetime.DeployableDesc.Types;
Add using BitCraft.Spacetime;
DeployableType should be DeployableDescDeployableType

BuildingSpawnToolManager.cs:
Remove using static BitCraft.Spacetime.BuildingSpawnDesc.Types;
BuildingSpawnType should be BuildingSpawnDescBuildingSpawnType

ClaimDescriptionState.cs
Types.ClaimPermission should be ClaimDescriptionStateClaimPermission

Clearing error CS0246 (count 34, unresolved 3):

AlertDesc.cs, Types.AlertType should be AlertDescAlertType
Enemy.cs, Types.EnemyType should be EnemyDescEnemyType
Npc.cs, Types.NpcType should be NpcDescNpcType
ProgressiveActionState.cs, Types.ProgressiveActionStatus should be ProgressiveActionStateProgressiveActionStatus
EquipmentDesc.cs, Types.EquipmentSlotType should be EquipmentDescEquipmentSlotType
FootprintDelta.cs, Types.FootprintType should be FootprintTileFootprintType
PlayerActionState.cs, Types.PlayerActionLayer should be PlayerActionStatePlayerActionLayer
ItemConversionRecipeDesc.cs, Types.ItemConversionLocationContext should be ItemConversionRecipeDescItemConversionLocationContext
CharacterStatDesc.cs, Types.CharacterStatType should be CharacterStatDescCharacterStatType
CollectibleDesc.cs, Types.CollectibleType should be CollectibleDescCollectibleType

Still reviewing the 74 cases of CS0311.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break A PR that makes an API breaking change bugfix Fixes something that was expected to work differently release-1.0
Projects
None yet
4 participants