Merge #414

414: better debugging for input structs by including id fields r=nikomatsakis a=OLUWAMUYIWA

Ref: #405 

Fixes #415 

Co-authored-by: OLUWAMUYIWA <onigbindemy@gmail.com>
Co-authored-by: Onigbinde Oluwamuyiwa Elijah <onigbindemy@gmail.com>
Co-authored-by: Niko Matsakis <niko@alum.mit.edu>

components/salsa-2022-macros/src/input.rs

@@ -51,8 +51,6 @@ impl crate::options::AllowedOptions for InputStruct {
 
 impl InputStruct {
     fn generate_input(&self) -> syn::Result<TokenStream> {
-        self.validate_input()?;
-
         let id_struct = self.id_struct();
         let inherent_impl = self.input_inherent_impl();
         let ingredients_for_impl = self.input_ingredients();
@@ -70,12 +68,6 @@ impl InputStruct {
         })
     }
 
-    fn validate_input(&self) -> syn::Result<()> {
-        // check for dissalowed fields
-        self.disallow_id_fields("input")?;
-        Ok(())
-    }
-
     /// Generate an inherent impl with methods on the entity type.
     fn input_inherent_impl(&self) -> syn::ItemImpl {
         let ident = self.id_ident();
@@ -112,8 +104,13 @@ impl InputStruct {
         )
         .collect();
 
+        // setters
         let set_field_names = self.all_set_field_names();
-        let field_setters: Vec<syn::ImplItemMethod> = field_indices.iter().zip(&set_field_names).zip(&field_vises).zip(&field_tys).map(|(((field_index, set_field_name), field_vis), field_ty)| {
+        let field_setters: Vec<syn::ImplItemMethod> = field_indices.iter()
+            .zip(&set_field_names)
+            .zip(&field_vises)
+            .zip(&field_tys)
+            .map(|(((field_index, set_field_name), field_vis), field_ty)| {
             parse_quote! {
                 #field_vis fn #set_field_name<'db>(self, __db: &'db mut #db_dyn_ty) -> salsa::setter::Setter<'db, #ident, #field_ty>
                 {
@@ -130,13 +127,18 @@ impl InputStruct {
 
         let constructor: syn::ImplItemMethod = if singleton {
             parse_quote! {
-                pub fn #constructor_name(__db: &mut #db_dyn_ty, #(#field_names: #field_tys,)*) -> Self
+                /// Creates a new singleton input
+                ///
+                /// # Panics
+                ///
+                /// If called when an instance already exists
+                pub fn #constructor_name(__db: &#db_dyn_ty, #(#field_names: #field_tys,)*) -> Self
                 {
-                    let (__jar, __runtime) = <_ as salsa::storage::HasJar<#jar_ty>>::jar_mut(__db);
+                    let (__jar, __runtime) = <_ as salsa::storage::HasJar<#jar_ty>>::jar(__db);
-                    let __ingredients = <#jar_ty as salsa::storage::HasIngredientsFor< #ident >>::ingredient_mut(__jar);
+                    let __ingredients = <#jar_ty as salsa::storage::HasIngredientsFor< #ident >>::ingredient(__jar);
                     let __id = __ingredients.#input_index.new_singleton_input(__runtime);
                     #(
-                        __ingredients.#field_indices.store_mut(__runtime, __id, #field_names, salsa::Durability::LOW);
+                        __ingredients.#field_indices.store_new(__runtime, __id, #field_names, salsa::Durability::LOW);
                     )*
                     __id
                 }
@@ -162,7 +164,7 @@ impl InputStruct {
                 pub fn get(__db: &#db_dyn_ty) -> Self {
                     let (__jar, __runtime) = <_ as salsa::storage::HasJar<#jar_ty>>::jar(__db);
                     let __ingredients = <#jar_ty as salsa::storage::HasIngredientsFor< #ident >>::ingredient(__jar);
-                    __ingredients.#input_index.get_singleton_input(__runtime).expect("singleton not yet initialized")
+                    __ingredients.#input_index.get_singleton_input(__runtime).expect("singleton input struct not yet initialized")
                 }
             };
 
@@ -289,6 +291,15 @@ impl InputStruct {
             .collect()
     }
 
+    /// Names of setters of all fields
+    /// setters are not created for fields with #[id] tag so they'll be safe to include in debug formatting
+    pub(crate) fn all_set_field_names(&self) -> Vec<&syn::Ident> {
+        self.all_fields()
+            .filter(|&field| !field.has_id_attr)
+            .map(|ef| ef.set_name())
+            .collect()
+    }
+
     /// Implementation of `SalsaStructInDb`.
     fn salsa_struct_in_db_impl(&self) -> syn::ItemImpl {
         let ident = self.id_ident();

components/salsa-2022-macros/src/salsa_struct.rs

@@ -99,8 +99,7 @@ impl<A: AllowedOptions> SalsaStruct<A> {
 
     pub(crate) fn is_identity_field(&self, field: &SalsaField) -> bool {
         match self.kind {
-            SalsaStructKind::Input => false,
+            SalsaStructKind::Input | SalsaStructKind::Tracked => field.has_id_attr,
-            SalsaStructKind::Tracked => field.has_id_attr,
             SalsaStructKind::Interned => true,
         }
     }
@@ -122,11 +121,6 @@ impl<A: AllowedOptions> SalsaStruct<A> {
         self.all_fields().map(|ef| ef.get_name()).collect()
     }
 
-    /// Names of setters of all fields
-    pub(crate) fn all_set_field_names(&self) -> Vec<&syn::Ident> {
-        self.all_fields().map(|ef| ef.set_name()).collect()
-    }
-
     /// Types of all fields (id and value).
     ///
     /// If this is an enum, empty vec.

components/salsa-2022/src/input.rs

@@ -49,9 +49,12 @@ where
         Id::from_id(crate::Id::from_u32(next_id))
     }
 
-    pub fn new_singleton_input(&mut self, _runtime: &Runtime) -> Id {
+    pub fn new_singleton_input(&self, _runtime: &Runtime) -> Id {
-        // There's only one singleton so record that we've created it
+        // when one exists already, panic
-        // and return the only id.
+        if self.counter.load(Ordering::Relaxed) >= 1 {
+            panic!("singleton struct may not be duplicated");
+        }
+        // fresh new ingredient
         self.counter.store(1, Ordering::Relaxed);
         Id::from_id(crate::Id::from_u32(0))
     }

salsa-2022-tests/tests/compile-fail/input_struct_id_fields_no_setters.rs

@@ -0,0 +1,31 @@
+
+#[salsa::jar(db = Db)]
+struct Jar(MyInput);
+
+trait Db: salsa::DbWithJar<Jar> {}
+
+#[salsa::input(jar = Jar)]
+struct MyInput {
+    field: u32,
+    #[id]
+    id_one: u32,
+}
+
+#[salsa::db(Jar)]
+#[derive(Default)]
+struct Database {
+    storage: salsa::Storage<Self>,
+}
+
+impl salsa::Database for Database {}
+
+impl Db for Database {}
+
+
+fn main() {
+    let mut db = Database::default();
+    let input = MyInput::new(&mut db, 3, 4);
+    // should not compile as `id_one` should not have a setter
+    // compile error: method `set_id_one` not found in scope
+    input.set_id_one(1);
+}

salsa-2022-tests/tests/compile-fail/input_struct_id_fields_no_setters.stderr

@@ -0,0 +1,8 @@
+error[E0599]: no method named `set_id_one` found for struct `MyInput` in the current scope
+  --> tests/compile-fail/input_struct_id_fields_no_setters.rs:30:11
+   |
+7  | #[salsa::input(jar = Jar)]
+   | -------------------------- method `set_id_one` not found for this
+...
+30 |     input.set_id_one(1);
+   |           ^^^^^^^^^^ help: there is an associated function with a similar name: `id_one`

salsa-2022-tests/tests/input_with_ids.rs

@@ -0,0 +1,39 @@
+#![allow(warnings)]
+
+use expect_test::expect;
+use salsa::DebugWithDb;
+
+#[salsa::jar(db = Db)]
+struct Jar(MyInput);
+
+trait Db: salsa::DbWithJar<Jar> {}
+
+#[salsa::input(jar = Jar)]
+struct MyInput {
+    field: u32,
+    #[id]
+    id_one: u32,
+    #[id]
+    id_two: u16,
+}
+
+#[salsa::db(Jar)]
+#[derive(Default)]
+struct Database {
+    storage: salsa::Storage<Self>,
+}
+
+impl salsa::Database for Database {}
+
+impl Db for Database {}
+
+#[test]
+fn test_debug() {
+    let mut db = Database::default();
+
+    let input = MyInput::new(&mut db, 22, 50, 10);
+
+    let actual = format!("{:?}", input.debug(&db));
+    let expected = expect![[r#"MyInput { [salsa id]: 0, id_one: 50, id_two: 10 }"#]];
+    expected.assert_eq(&actual);
+}

salsa-2022-tests/tests/singleton.rs

@@ -2,6 +2,8 @@
 //!
 //! Singleton structs are created only once. Subsequent `get`s and `new`s after creation return the same `Id`.
 
+use expect_test::expect;
+use salsa::DebugWithDb;
 use salsa_2022_tests::{HasLogger, Logger};
 
 use test_log::test;
@@ -14,6 +16,8 @@ trait Db: salsa::DbWithJar<Jar> + HasLogger {}
 #[salsa::input(singleton)]
 struct MyInput {
     field: u32,
+    #[id]
+    id_field: u16,
 }
 
 #[salsa::db(Jar)]
@@ -36,15 +40,33 @@ impl HasLogger for Database {
 #[test]
 fn basic() {
     let mut db = Database::default();
-    let input1 = MyInput::new(&mut db, 3);
+    let input1 = MyInput::new(&mut db, 3, 4);
     let input2 = MyInput::get(&db);
 
     assert_eq!(input1, input2);
 
     let input3 = MyInput::try_get(&db);
     assert_eq!(Some(input1), input3);
-
+}
-    let input4 = MyInput::new(&mut db, 3);
+
-
+#[test]
-    assert_eq!(input2, input4)
+#[should_panic]
+fn twice() {
+    let mut db = Database::default();
+    let input1 = MyInput::new(&mut db, 3, 4);
+    let input2 = MyInput::get(&db);
+
+    assert_eq!(input1, input2);
+
+    // should panic here
+    _ = MyInput::new(&mut db, 3, 5);
+}
+
+#[test]
+fn debug() {
+    let mut db = Database::default();
+    let input = MyInput::new(&mut db, 3, 4);
+    let actual = format!("{:?}", input.debug(&db));
+    let expected = expect![[r#"MyInput { [salsa id]: 0, id_field: 4 }"#]];
+    expected.assert_eq(&actual);
 }