-
Notifications
You must be signed in to change notification settings - Fork 161
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
implemented the builder api for salsa inputs #418
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,3 +1,5 @@ | ||||||
use std::fmt::Formatter; | ||||||
|
||||||
use crate::salsa_struct::{SalsaField, SalsaStruct, SalsaStructKind}; | ||||||
use proc_macro2::{Literal, TokenStream}; | ||||||
|
||||||
|
@@ -52,6 +54,7 @@ impl crate::options::AllowedOptions for InputStruct { | |||||
impl InputStruct { | ||||||
fn generate_input(&self) -> syn::Result<TokenStream> { | ||||||
let id_struct = self.id_struct(); | ||||||
let (builder, builder_impl) = self.input_builder(); | ||||||
let inherent_impl = self.input_inherent_impl(); | ||||||
let ingredients_for_impl = self.input_ingredients(); | ||||||
let as_id_impl = self.as_id_impl(); | ||||||
|
@@ -60,6 +63,8 @@ impl InputStruct { | |||||
|
||||||
Ok(quote! { | ||||||
#id_struct | ||||||
#builder | ||||||
#builder_impl | ||||||
#inherent_impl | ||||||
#ingredients_for_impl | ||||||
#as_id_impl | ||||||
|
@@ -159,6 +164,18 @@ impl InputStruct { | |||||
} | ||||||
}; | ||||||
|
||||||
let builder_fn: syn::ImplItemMethod = { | ||||||
let builder_name = self.builder_name(); | ||||||
parse_quote! { | ||||||
fn new_builder() -> #builder_name { | ||||||
#builder_name{ | ||||||
#(#field_names: None,)* | ||||||
durability: None, | ||||||
} | ||||||
} | ||||||
} | ||||||
}; | ||||||
|
||||||
if singleton { | ||||||
let get: syn::ImplItemMethod = parse_quote! { | ||||||
#[track_caller] | ||||||
|
@@ -189,6 +206,8 @@ impl InputStruct { | |||||
#(#field_getters)* | ||||||
|
||||||
#(#field_setters)* | ||||||
|
||||||
#builder_fn | ||||||
} | ||||||
} | ||||||
} else { | ||||||
|
@@ -199,11 +218,86 @@ impl InputStruct { | |||||
#(#field_getters)* | ||||||
|
||||||
#(#field_setters)* | ||||||
|
||||||
#builder_fn | ||||||
} | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
/// generate builder struct | ||||||
fn input_builder(&self) -> (syn::ItemStruct, syn::ItemImpl) { | ||||||
let ident = self.id_ident(); | ||||||
let struct_name = self.builder_name(); | ||||||
let field_names = self.all_field_names(); | ||||||
let field_tys: Vec<_> = self.all_field_tys(); | ||||||
let db_dyn_ty = self.db_dyn_ty(); | ||||||
let jar_ty = self.jar_ty(); | ||||||
let input_index = self.input_index(); | ||||||
let field_indices = self.all_field_indices(); | ||||||
|
||||||
let buidler: syn::ItemStruct = parse_quote! { | ||||||
struct #struct_name{ | ||||||
#(#field_names: Option<#field_tys>,)* | ||||||
durability: Option<salsa::durability::Durability>, | ||||||
} | ||||||
}; | ||||||
|
||||||
let build_fn: syn::ImplItemMethod = if self.0.is_isingleton() { | ||||||
parse_quote! { | ||||||
fn build(&mut self, __db: &#db_dyn_ty) -> std::result::Result<#ident, std::boxed::Box<dyn std::error::Error>> { | ||||||
let durability = self.durability.ok_or_else(|| "durability not provided")?; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why panic when a durability isn't specified, instead of using the default of low? I think one of the main advantages of a builder API is that you can have a lot of options, most of which aren't required, and you can only make the changes you need over sensible defaults. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmn true. Thanks. Will fix |
||||||
|
||||||
// } | ||||||
let (__jar, __runtime) = <_ as salsa::storage::HasJar<#jar_ty>>::jar(__db); | ||||||
let __ingredients = <#jar_ty as salsa::storage::HasIngredientsFor< #ident >>::ingredient(__jar); | ||||||
let __id = __ingredients.#input_index.new_singleton_input(__runtime); | ||||||
|
||||||
#( | ||||||
__ingredients.#field_indices.store_new(__runtime, __id, self.#field_names.clone().ok_or_else(||"field not provided")?, durability); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think using the same durability for all the fields gives enough flexibility to the user. Wouldn't it be better to have a method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thought about having Another option I'm considering is having Thanks Mihail! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The cognitive overhead point is something that I didn't consider, but I did go over the idea with the tuples and decided to not suggest it, because it wouldn't look very good if we needed to add more options for the fields in the future. And then we also lose the ability to change just 1 or 2 things about a field, we'd need to set all 5 (again this only applies if we do consider having more than just a value and a durability). Also if tuples were an option all the intermediate structs produced by the builder would have an incomplete state, and the user would need to keep track of all the fields being set. But that's just my opinion I think we should hear what others have to say. |
||||||
)* | ||||||
std::result::Result::Ok(__id) | ||||||
} | ||||||
} | ||||||
} else { | ||||||
parse_quote! { | ||||||
fn build(&mut self, __db: &#db_dyn_ty) -> std::result::Result<#ident, std::boxed::Box<dyn std::error::Error>> { | ||||||
let durability = self.durability.ok_or_else(|| "durability not provided")?; | ||||||
|
||||||
let (__jar, __runtime) = <_ as salsa::storage::HasJar<#jar_ty>>::jar(__db); | ||||||
let __ingredients = <#jar_ty as salsa::storage::HasIngredientsFor< #ident >>::ingredient(__jar); | ||||||
let __id =__ingredients.#input_index.new_input(__runtime); | ||||||
#( | ||||||
__ingredients.#field_indices.store_new(__runtime, __id, self.#field_names.clone().ok_or_else(||"field not provided")?, durability); | ||||||
)* | ||||||
std::result::Result::Ok(__id) | ||||||
} | ||||||
} | ||||||
}; | ||||||
|
||||||
let impls: syn::ItemImpl = { | ||||||
parse_quote! { | ||||||
impl #struct_name { | ||||||
fn with_durability(&mut self, durability: salsa::durability::Durability) -> &mut Self { | ||||||
self.durability = Some(durability); | ||||||
self | ||||||
} | ||||||
|
||||||
fn with_fields(&mut self, #(#field_names: #field_tys,)*) -> &mut Self { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmn. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think of a field as something which has a value, a durability and maybe other things associated with it, which is why |
||||||
#(self.#field_names = Some(#field_names);)* | ||||||
self | ||||||
} | ||||||
|
||||||
#build_fn | ||||||
} | ||||||
} | ||||||
}; | ||||||
|
||||||
(buidler, impls) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
fn builder_name(&self) -> syn::Ident { | ||||||
let ident = self.id_ident(); | ||||||
syn::Ident::new(&format!("{}Builder", ident), ident.span()) | ||||||
} | ||||||
|
||||||
/// Generate the `IngredientsFor` impl for this entity. | ||||||
|
@@ -319,3 +413,14 @@ impl InputStruct { | |||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
#[derive(Debug)] | ||||||
pub struct InputBuilderError(String); | ||||||
|
||||||
impl std::fmt::Display for InputBuilderError { | ||||||
fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), std::fmt::Error> { | ||||||
write!(f, "{:?}", self.0) | ||||||
} | ||||||
} | ||||||
|
||||||
impl std::error::Error for InputBuilderError {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
#![allow(warnings)] | ||
|
||
use expect_test::expect; | ||
use salsa::Durability; | ||
#[salsa::jar(db = Db)] | ||
struct Jar(MyInput, MySingletonInput); | ||
|
||
trait Db: salsa::DbWithJar<Jar> {} | ||
|
||
#[salsa::input(jar = Jar)] | ||
struct MyInput { | ||
field1: u32, | ||
field2: u32, | ||
} | ||
|
||
#[salsa::input(singleton)] | ||
struct MySingletonInput { | ||
field1: u32, | ||
} | ||
|
||
#[salsa::db(Jar)] | ||
#[derive(Default)] | ||
struct Database { | ||
storage: salsa::Storage<Self>, | ||
} | ||
|
||
impl salsa::Database for Database {} | ||
|
||
impl Db for Database {} | ||
|
||
#[test] | ||
fn test_builder() { | ||
let mut db = Database::default(); | ||
// durability set with new is always low | ||
let mut input = MyInput::new(&db, 12, 13); | ||
input | ||
.set_field1(&mut db) | ||
.with_durability(Durability::HIGH) | ||
.to(20); | ||
input | ||
.set_field2(&mut db) | ||
.with_durability(Durability::HIGH) | ||
.to(40); | ||
let input_from_builder = MyInput::new_builder() | ||
.with_durability(Durability::HIGH) | ||
.with_fields(20, 40) | ||
.build(&db) | ||
.unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning a let input_from_builder = MyInput::new_builder()
.with_db(&db)
.with_durability(Durability::HIGH)
.with_fields(20, 40); I think this makes the API calls shorter, with no downsides and it would be closer to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's a real concern. I actually think builders should have Maybe others have something to say? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That and the database are the two things that we need to set for the struct to be complete. I think either one of those could be used as the
I don't either, so take what I say with a grain of salt. I just wanted to bring it up as a talking point, so we could discuss it with the others. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Coming back to this with fresh eyes, a few thoughts--
It seems like we should back up from the PR and first we be more specific about the API that we want for an example. |
||
|
||
assert_eq!(input.field1(&db), input_from_builder.field1(&db)); | ||
assert_eq!(input.field2(&db), input_from_builder.field2(&db)); | ||
} | ||
|
||
#[test] | ||
#[should_panic] | ||
// should panic because were creating he same input twice | ||
fn test_sg_builder_panic() { | ||
let mut db = Database::default(); | ||
let input1 = MySingletonInput::new(&db, 5); | ||
let input_from_builder = MySingletonInput::new_builder() | ||
.with_durability(Durability::LOW) | ||
.with_fields(5) | ||
.build(&db) | ||
.unwrap(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Will fix