Add schema::type_for_generate, update validation to use it #1661

kazimuth · 2024-09-03T21:34:35Z

Description of Changes

This should significantly ease writing client codegen. @RReverser, the added types aren't serializable yet, I'll leave that to you if you'd like to do it in a further PR.

Note: I've added a requirement that all product and sum types in a ModuleDef have names for all elements, because I believe this is currently a requirement for client codegen. This can be removed if we ever figure out how to do client codegen without it.

API and ABI breaking changes

None

Expected complexity level and risk

1, the logic added here is somewhat intricate. It's tested against a good number of test cases, all of the modules in the repo, and a proptest, which gives me some confidence in it.

Testing

See the added tests in the source code.

cloutiertyler

These changes make sense to me in light of our discussion yesterday.

RReverser · 2024-09-05T09:49:21Z

crates/schema/src/type_for_generate.rs

+                            .and_then(|def| {
+                                if def == &AlgebraicType::Ref(ref_) {
+                                    // Self-reference.
+                                    Err(TypeRefError::RecursiveTypeRef(ref_))


So this now prohibits recursive types altogether? Can we please keep them?

I currently can autogenerate C# AlgebraicType structure with all its children types directly from Rust with no issues, despite it being recursive. I'm already making use of this in ModuleDef autogen in #1670, and I'd like to use it more in the future.

If we error out on recursive types, then AlgebraicType as well as anything that depends on it - which includes ModuleDef - will error out and won't codegen anymore.

I think instead of an error we should have AlgebraicTypeUse::Ref { ref_, is_recursive } variant so that langs that need to explicitly box the reference (only Rust among langs we currently support), have boolean tag to know that they need to do so, whereas others that already keep all the objects on the heap, can deal with it as a normal reference.

Ah, I thought we didn't support recursive types in modules though. Does the code generation for that pass through this, like are you using the module codegen to generate those? I'm happy to add that tag if so.

Does the code generation for that pass through this, like are you using the module codegen to generate those?

Yeah, since we don't have any other codegen, we use regular spacetime generate for ClientApi and for this new ModuleDef and then delete files we're not interested in heh.

Someday we might clean it up, but I imagine the basic codegen pipeline is still going to be shared between modules and "just types".

Cool, I'll fix that then.
Ugh now I need to actually handle cycles though. ;-;

Luckily, you use Arc so it should be as easy as downgrading it all to sync::Weak... maybe? hopefully? 😅

The cycles all pass through AlgebraicTypeRef so no need for that even.

crates/schema/src/type_for_generate.rs

RReverser · 2024-09-05T10:27:19Z

crates/schema/src/type_for_generate.rs

+/// Chains of `AlgebraicTypeRef`s in the original `Typespace` are contracted to point to their ending `AlgebraicTypeDef`.
+#[derive(Debug, Clone)]
+pub struct TypespaceForGenerate {
+    defs: HashMap<AlgebraicTypeRef, AlgebraicTypeDef>,


Could we also provide fully resolved type names here?

It would be nice not to have to worry about #[sats(name)], --namespace and so on in each generator, and instead just have something akin to your ScopedTypeName prepared and ready to be used for codegen.

The relevant ScopedTypeNames are stored in the containing ModuleDef, I can add a helper accessor, or just clone them in here.

In particular, you should be looking things up in here based on the TypeDefs in ModuleDef, so you'll have it available.

crates/schema/src/type_for_generate.rs

coolreader18 · 2024-09-05T17:00:45Z

In #1675 I added some utilities to ModuleDef that make it easier/possible to do codegen - would it be ok to add those here? Maybe I'll make a PR into this branch.

crates/sats/src/proptest.rs

crates/schema/src/type_for_generate.rs

kazimuth · 2024-09-09T17:00:34Z

crates/schema/src/type_for_generate.rs

+
+    /// Cycles passing through definitions are allowed.
+    /// This function is called after all definitions have been processed.
+    fn mark_allowed_cycles(&mut self) {


This could use review

kazimuth · 2024-09-10T17:38:50Z

crates/lib/src/db/raw_def/v9.rs

@@ -296,6 +296,37 @@ pub struct RawScheduleDefV9 {
    pub reducer_name: RawIdentifier,
 }

+/// A constraint definition attached to a table.


This is also important

minor bug in the tests in the process Add utilities to ModuleDef for easier ues for codegen (#1678) Update crates/sats/src/proptest.rs Co-authored-by: Mazdak Farrokhzad <[email protected]> Signed-off-by: james gilles <[email protected]> Move constraints around for easier future ABI evolution WIP: Allow cyclic AlgebraicTypes. Final fixes, address review comments Final comments addressed & copy editing Remove outdated comment Regenerate C# bindings autogen

crates/sats/src/algebraic_type.rs

crates/lib/src/db/raw_def/v9.rs

Co-authored-by: Ingvar Stepanyan <[email protected]> Signed-off-by: james gilles <[email protected]>

kazimuth requested review from RReverser, Centril, cloutiertyler and coolreader18 September 3, 2024 21:34

kazimuth mentioned this pull request Sep 3, 2024

WIP: updating Schema and system tables #1662

Closed

2 tasks

coolreader18 approved these changes Sep 4, 2024

View reviewed changes

cloutiertyler approved these changes Sep 4, 2024

View reviewed changes

coolreader18 mentioned this pull request Sep 5, 2024

Switch Rust codegen to use new ModuleDef #1675

Merged

RReverser reviewed Sep 5, 2024

View reviewed changes

crates/schema/src/type_for_generate.rs Show resolved Hide resolved

RReverser reviewed Sep 5, 2024

View reviewed changes

crates/schema/src/type_for_generate.rs Outdated Show resolved Hide resolved

RReverser reviewed Sep 5, 2024

View reviewed changes

crates/schema/src/type_for_generate.rs Show resolved Hide resolved

Centril reviewed Sep 5, 2024

View reviewed changes

crates/sats/src/proptest.rs Outdated Show resolved Hide resolved

crates/schema/src/type_for_generate.rs Outdated Show resolved Hide resolved

coolreader18 mentioned this pull request Sep 5, 2024

Add utilities to ModuleDef for easier ues for codegen #1678

Merged

kazimuth force-pushed the jgilles/algebraic_type_for_generate branch from f2fe780 to 0981bb1 Compare September 9, 2024 16:54

kazimuth commented Sep 9, 2024

View reviewed changes

bfops added the release-any To be landed in any release window label Sep 9, 2024

kazimuth force-pushed the jgilles/algebraic_type_for_generate branch from 16d16b4 to f88641d Compare September 10, 2024 17:28

kazimuth commented Sep 10, 2024

View reviewed changes

kazimuth enabled auto-merge September 10, 2024 20:17

kazimuth disabled auto-merge September 11, 2024 16:09

kazimuth force-pushed the jgilles/algebraic_type_for_generate branch from 35c7cef to 52ec634 Compare September 11, 2024 16:09

RReverser reviewed Sep 11, 2024

View reviewed changes

crates/sats/src/algebraic_type.rs Outdated Show resolved Hide resolved

RReverser reviewed Sep 11, 2024

View reviewed changes

crates/lib/src/db/raw_def/v9.rs Outdated Show resolved Hide resolved

kazimuth and others added 3 commits September 11, 2024 12:46

Update crates/sats/src/algebraic_type.rs

2ea1158

Co-authored-by: Ingvar Stepanyan <[email protected]> Signed-off-by: james gilles <[email protected]>

Update outdated comments

308058f

nit

bcbfb1e

kazimuth added this pull request to the merge queue Sep 11, 2024

kazimuth removed this pull request from the merge queue due to a manual request Sep 11, 2024

kazimuth added this pull request to the merge queue Sep 11, 2024

cloutiertyler removed this pull request from the merge queue due to a manual request Sep 11, 2024

cloutiertyler merged commit f3e3d92 into master Sep 11, 2024
8 checks passed

kazimuth deleted the jgilles/algebraic_type_for_generate branch September 11, 2024 20:44

This was referenced Sep 11, 2024

Regenerate C# types #1698

Closed

Migrate C# ModuleDef to V9 #1670

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add schema::type_for_generate, update validation to use it #1661

Add schema::type_for_generate, update validation to use it #1661

kazimuth commented Sep 3, 2024

cloutiertyler left a comment

RReverser Sep 5, 2024 •

edited

Loading

kazimuth Sep 5, 2024

RReverser Sep 5, 2024

kazimuth Sep 5, 2024

RReverser Sep 6, 2024

kazimuth Sep 9, 2024

RReverser Sep 5, 2024

kazimuth Sep 5, 2024

kazimuth Sep 9, 2024

coolreader18 commented Sep 5, 2024 •

edited

Loading

kazimuth Sep 9, 2024

kazimuth Sep 10, 2024

Add schema::type_for_generate, update validation to use it #1661

Add schema::type_for_generate, update validation to use it #1661

Conversation

kazimuth commented Sep 3, 2024

Description of Changes

API and ABI breaking changes

Expected complexity level and risk

Testing

cloutiertyler left a comment

Choose a reason for hiding this comment

RReverser Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coolreader18 commented Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RReverser Sep 5, 2024 •

edited

Loading

coolreader18 commented Sep 5, 2024 •

edited

Loading