Don't autogen schedule fields (#1894)

Co-authored-by: Phoebe Goldman <[email protected]>

crates/bindings-macro/src/lib.rs

@@ -45,6 +45,7 @@ mod sym {
     symbol!(public);
     symbol!(sats);
     symbol!(scheduled);
+    symbol!(scheduled_at);
     symbol!(unique);
     symbol!(update);
 
@@ -154,7 +155,7 @@ mod sym {
 pub fn reducer(args: StdTokenStream, item: StdTokenStream) -> StdTokenStream {
     cvt_attr::<ItemFn>(args, item, quote!(), |args, original_function| {
         let args = reducer::ReducerArgs::parse(args)?;
-        reducer::reducer_impl(args, &original_function)
+        reducer::reducer_impl(args, original_function)
     })
 }
 
@@ -241,7 +242,7 @@ pub fn table(args: StdTokenStream, item: StdTokenStream) -> StdTokenStream {
 
 /// Provides helper attributes for `#[spacetimedb::table]`, so that we don't get unknown attribute errors.
 #[doc(hidden)]
-#[proc_macro_derive(__TableHelper, attributes(sats, unique, auto_inc, primary_key, index))]
+#[proc_macro_derive(__TableHelper, attributes(sats, unique, auto_inc, primary_key, index, scheduled_at))]
 pub fn table_helper(_input: StdTokenStream) -> StdTokenStream {
     Default::default()
 }

crates/bindings-macro/src/table.rs

@@ -1,6 +1,6 @@
 use crate::sats::{self, derive_deserialize, derive_satstype, derive_serialize};
 use crate::sym;
-use crate::util::{check_duplicate, check_duplicate_msg, ident_to_litstr, match_meta, MutItem};
+use crate::util::{check_duplicate, check_duplicate_msg, ident_to_litstr, match_meta};
 use heck::ToSnakeCase;
 use proc_macro2::{Span, TokenStream};
 use quote::{format_ident, quote, quote_spanned, ToTokens};
@@ -36,21 +36,6 @@ impl TableAccess {
     }
 }
 
-// add scheduled_id and scheduled_at fields to the struct
-fn add_scheduled_fields(item: &mut syn::DeriveInput) {
-    if let syn::Data::Struct(struct_data) = &mut item.data {
-        if let syn::Fields::Named(fields) = &mut struct_data.fields {
-            let extra_fields: syn::FieldsNamed = parse_quote!({
-                #[primary_key]
-                #[auto_inc]
-                pub scheduled_id: u64,
-                pub scheduled_at: spacetimedb::spacetimedb_lib::ScheduleAt,
-            });
-            fields.named.extend(extra_fields.named);
-        }
-    }
-}
-
 struct IndexArg {
     name: Ident,
     kind: IndexType,
@@ -373,6 +358,7 @@ enum ColumnAttr {
     AutoInc(Span),
     PrimaryKey(Span),
     Index(IndexArg),
+    ScheduledAt(Span),
 }
 
 impl ColumnAttr {
@@ -392,23 +378,18 @@ impl ColumnAttr {
         } else if ident == sym::primary_key {
             attr.meta.require_path_only()?;
             Some(ColumnAttr::PrimaryKey(ident.span()))
+        } else if ident == sym::scheduled_at {
+            attr.meta.require_path_only()?;
+            Some(ColumnAttr::ScheduledAt(ident.span()))
         } else {
             None
         })
     }
 }
 
-pub(crate) fn table_impl(mut args: TableArgs, mut item: MutItem<syn::DeriveInput>) -> syn::Result<TokenStream> {
-    let scheduled_reducer_type_check = args.scheduled.as_ref().map(|reducer| {
-        add_scheduled_fields(&mut item);
-        let struct_name = &item.ident;
-        quote! {
-            const _: () = spacetimedb::rt::scheduled_reducer_typecheck::<#struct_name>(#reducer);
-        }
-    });
-
+pub(crate) fn table_impl(mut args: TableArgs, item: &syn::DeriveInput) -> syn::Result<TokenStream> {
     let vis = &item.vis;
-    let sats_ty = sats::sats_type_from_derive(&item, quote!(spacetimedb::spacetimedb_lib))?;
+    let sats_ty = sats::sats_type_from_derive(item, quote!(spacetimedb::spacetimedb_lib))?;
 
     let original_struct_ident = sats_ty.ident;
     let table_ident = &args.name;
@@ -437,7 +418,7 @@ pub(crate) fn table_impl(mut args: TableArgs, mut item: MutItem<syn::DeriveInput
 
     if fields.len() > u16::MAX.into() {
         return Err(syn::Error::new_spanned(
-            &*item,
+            item,
             "too many columns; the most a table can have is 2^16",
         ));
     }
@@ -446,6 +427,7 @@ pub(crate) fn table_impl(mut args: TableArgs, mut item: MutItem<syn::DeriveInput
     let mut unique_columns = vec![];
     let mut sequenced_columns = vec![];
     let mut primary_key_column = None;
+    let mut scheduled_at_column = None;
 
     for (i, field) in fields.iter().enumerate() {
         let col_num = i as u16;
@@ -454,6 +436,7 @@ pub(crate) fn table_impl(mut args: TableArgs, mut item: MutItem<syn::DeriveInput
         let mut unique = None;
         let mut auto_inc = None;
         let mut primary_key = None;
+        let mut scheduled_at = None;
         for attr in field.original_attrs {
             let Some(attr) = ColumnAttr::parse(attr, field_ident)? else {
                 continue;
@@ -472,6 +455,10 @@ pub(crate) fn table_impl(mut args: TableArgs, mut item: MutItem<syn::DeriveInput
                     primary_key = Some(span);
                 }
                 ColumnAttr::Index(index_arg) => args.indices.push(index_arg),
+                ColumnAttr::ScheduledAt(span) => {
+                    check_duplicate(&scheduled_at, span)?;
+                    scheduled_at = Some(span);
+                }
             }
         }
 
@@ -497,10 +484,27 @@ pub(crate) fn table_impl(mut args: TableArgs, mut item: MutItem<syn::DeriveInput
             check_duplicate_msg(&primary_key_column, span, "can only have one primary key per table")?;
             primary_key_column = Some(column);
         }
+        if let Some(span) = scheduled_at {
+            check_duplicate_msg(
+                &scheduled_at_column,
+                span,
+                "can only have one scheduled_at column per table",
+            )?;
+            scheduled_at_column = Some(column);
+        }
 
         columns.push(column);
     }
 
+    let scheduled_at_typecheck = scheduled_at_column.map(|col| {
+        let ty = col.ty;
+        quote!(let _ = |x: #ty| { let _: spacetimedb::ScheduleAt = x; };)
+    });
+    let scheduled_id_typecheck = primary_key_column.filter(|_| args.scheduled.is_some()).map(|col| {
+        let ty = col.ty;
+        quote!(spacetimedb::rt::assert_scheduled_table_primary_key::<#ty>();)
+    });
+
     let row_type = quote!(#original_struct_ident);
 
     let mut indices = args
@@ -548,17 +552,46 @@ pub(crate) fn table_impl(mut args: TableArgs, mut item: MutItem<syn::DeriveInput
     let primary_col_id = primary_key_column.iter().map(|col| col.index);
     let sequence_col_ids = sequenced_columns.iter().map(|col| col.index);
 
+    let scheduled_reducer_type_check = args.scheduled.as_ref().map(|reducer| {
+        quote! {
+            spacetimedb::rt::scheduled_reducer_typecheck::<#original_struct_ident>(#reducer);
+        }
+    });
     let schedule = args
         .scheduled
         .as_ref()
         .map(|reducer| {
-            // scheduled_at was inserted as the last field
-            let scheduled_at_id = (fields.len() - 1) as u16;
-            quote!(spacetimedb::table::ScheduleDesc {
+            // better error message when both are missing
+            if scheduled_at_column.is_none() && primary_key_column.is_none() {
+                return Err(syn::Error::new(
+                    original_struct_ident.span(),
+                    "scheduled table missing required columns; add these to your struct:\n\
+                             #[primary_key]\n\
+                             #[auto_inc]\n\
+                             scheduled_id: u64,\n\
+                             #[scheduled_at]\n\
+                             scheduled_at: spacetimedb::ScheduleAt,",
+                ));
+            }
+            let scheduled_at_column = scheduled_at_column.ok_or_else(|| {
+                syn::Error::new(
+                    original_struct_ident.span(),
+                    "scheduled tables must have a `#[scheduled_at] scheduled_at: spacetimedb::ScheduleAt` column.",
+                )
+            })?;
+            let scheduled_at_id = scheduled_at_column.index;
+            if primary_key_column.is_none() {
+                return Err(syn::Error::new(
+                    original_struct_ident.span(),
+                    "scheduled tables should have a `#[primary_key] #[auto_inc] scheduled_id: u64` column",
+                ));
+            }
+            Ok(quote!(spacetimedb::table::ScheduleDesc {
                 reducer_name: <#reducer as spacetimedb::rt::ReducerInfo>::NAME,
                 scheduled_at_column: #scheduled_at_id,
-            })
+            }))
         })
+        .transpose()?
         .into_iter();
 
     let unique_err = if !unique_columns.is_empty() {
@@ -572,7 +605,6 @@ pub(crate) fn table_impl(mut args: TableArgs, mut item: MutItem<syn::DeriveInput
         quote!(::core::convert::Infallible)
     };
 
-    let field_names = fields.iter().map(|f| f.ident.unwrap()).collect::<Vec<_>>();
     let field_types = fields.iter().map(|f| f.ty).collect::<Vec<_>>();
 
     let tabletype_impl = quote! {
@@ -607,16 +639,6 @@ pub(crate) fn table_impl(mut args: TableArgs, mut item: MutItem<syn::DeriveInput
         }
     };
 
-    let col_num = 0u16..;
-    let field_access_impls = quote! {
-        #(impl spacetimedb::table::FieldAccess<#col_num> for #original_struct_ident {
-            type Field = #field_types;
-            fn get_field(&self) -> &Self::Field {
-                &self.#field_names
-            }
-        })*
-    };
-
     let row_type_to_table = quote!(<#row_type as spacetimedb::table::__MapRowTypeToTable>::Table);
 
     // Output all macro data
@@ -643,6 +665,9 @@ pub(crate) fn table_impl(mut args: TableArgs, mut item: MutItem<syn::DeriveInput
     let emission = quote! {
         const _: () = {
             #(let _ = <#field_types as spacetimedb::rt::TableColumn>::_ITEM;)*
+            #scheduled_reducer_type_check
+            #scheduled_at_typecheck
+            #scheduled_id_typecheck
         };
 
         #trait_def
@@ -677,10 +702,6 @@ pub(crate) fn table_impl(mut args: TableArgs, mut item: MutItem<syn::DeriveInput
         #schema_impl
         #deserialize_impl
         #serialize_impl
-
-        #field_access_impls
-
-        #scheduled_reducer_type_check
     };
 
     if std::env::var("PROC_MACRO_DEBUG").is_ok() {

crates/bindings-macro/src/util.rs

@@ -10,48 +10,21 @@ pub(crate) fn cvt_attr<Item: Parse + quote::ToTokens>(
     args: StdTokenStream,
     item: StdTokenStream,
     extra_attr: TokenStream,
-    f: impl FnOnce(TokenStream, MutItem<'_, Item>) -> syn::Result<TokenStream>,
+    f: impl FnOnce(TokenStream, &Item) -> syn::Result<TokenStream>,
 ) -> StdTokenStream {
     let item: TokenStream = item.into();
-    let mut parsed_item = match syn::parse2::<Item>(item.clone()) {
+    let parsed_item = match syn::parse2::<Item>(item.clone()) {
         Ok(i) => i,
         Err(e) => return TokenStream::from_iter([item, e.into_compile_error()]).into(),
     };
-    let mut modified = false;
-    let mut_item = MutItem {
-        val: &mut parsed_item,
-        modified: &mut modified,
-    };
-    let generated = f(args.into(), mut_item).unwrap_or_else(syn::Error::into_compile_error);
-    let item = if modified {
-        parsed_item.into_token_stream()
-    } else {
-        item
-    };
+    let generated = f(args.into(), &parsed_item).unwrap_or_else(syn::Error::into_compile_error);
     TokenStream::from_iter([extra_attr, item, generated]).into()
 }
 
 pub(crate) fn ident_to_litstr(ident: &Ident) -> syn::LitStr {
     syn::LitStr::new(&ident.to_string(), ident.span())
 }
 
-pub(crate) struct MutItem<'a, T> {
-    val: &'a mut T,
-    modified: &'a mut bool,
-}
-impl<T> std::ops::Deref for MutItem<'_, T> {
-    type Target = T;
-    fn deref(&self) -> &Self::Target {
-        self.val
-    }
-}
-impl<T> std::ops::DerefMut for MutItem<'_, T> {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        *self.modified = true;
-        self.val
-    }
-}
-
 pub(crate) trait ErrorSource {
     fn error(self, msg: impl std::fmt::Display) -> syn::Error;
 }

crates/bindings/src/table.rs

@@ -261,19 +261,6 @@ impl MaybeError for AutoIncOverflow {
     }
 }
 
-/// A trait for types exposing an operation to access their `N`th field.
-///
-/// In other words, a type implementing `FieldAccess<N>` allows
-/// shared projection from `self` to its `N`th field.
-#[doc(hidden)]
-pub trait FieldAccess<const N: u16> {
-    /// The type of the field at the `N`th position.
-    type Field;
-
-    /// Project to the value of the field at position `N`.
-    fn get_field(&self) -> &Self::Field;
-}
-
 pub trait Column {
     type Row;
     type ColType;

crates/bindings/tests/ui/reducers.rs

@@ -25,8 +25,22 @@ fn missing_ctx(_a: u8) {}
 #[spacetimedb::reducer]
 fn ctx_by_val(_ctx: ReducerContext, _a: u8) {}
 
+#[spacetimedb::table(name = scheduled_table_missing_rows, scheduled(scheduled_table_missing_rows_reducer))]
+struct ScheduledTableMissingRows {
+    x: u8,
+    y: u8,
+}
+
+// #[spacetimedb::reducer]
+// fn scheduled_table_missing_rows_reducer(_ctx: &ReducerContext, _: &ScheduledTableMissingRows) {}
+
 #[spacetimedb::table(name = scheduled_table, scheduled(scheduled_table_reducer))]
 struct ScheduledTable {
+    #[primary_key]
+    #[auto_inc]
+    scheduled_id: u64,
+    #[scheduled_at]
+    scheduled_at: spacetimedb::ScheduleAt,
     x: u8,
     y: u8,
 }

crates/bindings/tests/ui/reducers.stderr

@@ -10,16 +10,27 @@ error: const parameters are not allowed on reducers
 20 | fn const_param<const X: u8>() {}
    |                ^^^^^^^^^^^
 
+error: scheduled table missing required columns; add these to your struct:
+       #[primary_key]
+       #[auto_inc]
+       scheduled_id: u64,
+       #[scheduled_at]
+       scheduled_at: spacetimedb::ScheduleAt,
+  --> tests/ui/reducers.rs:29:8
+   |
+29 | struct ScheduledTableMissingRows {
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^
+
 error[E0593]: function is expected to take 2 arguments, but it takes 3 arguments
-  --> tests/ui/reducers.rs:28:56
+  --> tests/ui/reducers.rs:37:56
    |
-28 | #[spacetimedb::table(name = scheduled_table, scheduled(scheduled_table_reducer))]
+37 | #[spacetimedb::table(name = scheduled_table, scheduled(scheduled_table_reducer))]
    | -------------------------------------------------------^^^^^^^^^^^^^^^^^^^^^^^---
    | |                                                      |
    | |                                                      expected function that takes 2 arguments
    | required by a bound introduced by this call
 ...
-35 | fn scheduled_table_reducer(_ctx: &ReducerContext, _x: u8, _y: u8) {}
+49 | fn scheduled_table_reducer(_ctx: &ReducerContext, _x: u8, _y: u8) {}
    | ----------------------------------------------------------------- takes 3 arguments
    |
    = note: required for `for<'a> fn(&'a ReducerContext, u8, u8) {scheduled_table_reducer}` to implement `Reducer<'_, (ScheduledTable,)>`