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

Migrate C# ModuleDef to V9 #1670

Merged
merged 15 commits into from
Dec 16, 2024
Merged

Migrate C# ModuleDef to V9 #1670

merged 15 commits into from
Dec 16, 2024

Conversation

RReverser
Copy link
Member

@RReverser RReverser commented Sep 4, 2024

Description of Changes

This migrates module definition to V9 for C# modules as we're planning to drop V8, and it can't represent some of the new features.

In the process, also improved Debug pretty-printing for the Rust typespace and nested types (to use in future diffs in tests).

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.

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!

  • Write a test you've completed here.
  • Write a test you want a reviewer to do here, so they can check it off when they're satisfied.

@RReverser RReverser requested a review from kazimuth September 4, 2024 19:32
@RReverser RReverser changed the title Derive entire ModuleDef tree Migrate ModuleDef to V9 Sep 4, 2024
@RReverser RReverser changed the title Migrate ModuleDef to V9 Migrate C# ModuleDef to V9 Sep 4, 2024
Base automatically changed from ingvar/csharp-table-inherit-type to master September 5, 2024 18:19
@RReverser RReverser changed the base branch from master to ingvar/derive-meta-types September 5, 2024 19:37
@RReverser RReverser force-pushed the ingvar/derive-meta-types branch 4 times, most recently from 0b6c0f0 to 0fddd55 Compare September 6, 2024 16:50
Base automatically changed from ingvar/derive-meta-types to master September 9, 2024 20:14
@RReverser RReverser mentioned this pull request Sep 18, 2024
@RReverser RReverser force-pushed the ingvar/v9 branch 2 times, most recently from f63859d to 71c7722 Compare September 18, 2024 18:41
@RReverser
Copy link
Member Author

Updated this PR to latest definitions. Thanks to #1661 + @coolreader18's #1675, I can verify that I'm now producing correct ModuleDef as it can generate Rust sources from a C# module. Further testing is still blocked by lack of V9 support in the actual runtime - I'm hitting

_ => {
return Err(InitializationError::Describe(
DescribeError::UnimplementedRawModuleDefVersion,
))
}
so leaving this PR as a draft.

Migrated C# ModuleDef to V9 as well as adds tests to ensure that identical modules indeed produce identical schemas.

The two remaining discrepancies between Rust and C# are:
 - #1891
 - Row-level security not having a designed API & implementation for C#.

 - Fixes #1948.
 - Fixes #1589.
 - Supersedes #1619.
@RReverser RReverser marked this pull request as ready for review December 9, 2024 17:09
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.

All of this looks good. The codegen changes look sensible and the better debug-formatting Rust-side is a good idea. My big question is whether it would make sense to get rid of the ColumnAttrs bitfield entirely.

crates/bindings-csharp/BSATN.Codegen/Type.cs Show resolved Hide resolved
crates/bindings-csharp/Codegen/Module.cs Show resolved Hide resolved
crates/bindings-csharp/BSATN.Codegen/Type.cs Show resolved Hide resolved
modules/spacetimedb-quickstart/src/lib.rs Show resolved Hide resolved
crates/schema/tests/ensure_same_schema.rs Show resolved Hide resolved
crates/sats/src/lib.rs Outdated Show resolved Hide resolved
@RReverser
Copy link
Member Author

My big question is whether it would make sense to get rid of the ColumnAttrs bitfield entirely.

What would you replace it with?

@RReverser RReverser added the breaks library compatibility This change breaks the SpacetimeDB library interface label Dec 16, 2024
@kazimuth
Copy link
Contributor

My big question is whether it would make sense to get rid of the ColumnAttrs bitfield entirely.

What would you replace it with?

On the Rust side, we have a data structure listing indexes, a data structure listing constraints, a data structure listing sequences, and that's it. It seems to work fine, but the logic in C# may be structured differently so that that isn't as easy to do. No need to make the change if it's just makework.

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.

I have no complaints. I think we should merge this ASAP.

@RReverser RReverser enabled auto-merge December 16, 2024 21:12
@RReverser RReverser added this pull request to the merge queue Dec 16, 2024
Merged via the queue into master with commit 08a3f32 Dec 16, 2024
8 checks passed
@RReverser RReverser deleted the ingvar/v9 branch December 16, 2024 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks library compatibility This change breaks the SpacetimeDB library interface release-1.0
Projects
None yet
3 participants