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

implemented the builder api for salsa inputs #418

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 106 additions & 1 deletion components/salsa-2022-macros/src/input.rs
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};

Expand Down Expand Up @@ -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();
Expand All @@ -60,6 +63,8 @@ impl InputStruct {

Ok(quote! {
#id_struct
#builder
#builder_impl
#inherent_impl
#ingredients_for_impl
#as_id_impl
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -189,6 +206,8 @@ impl InputStruct {
#(#field_getters)*

#(#field_setters)*

#builder_fn
}
}
} else {
Expand All @@ -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! {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

Suggested change
let buidler: syn::ItemStruct = parse_quote! {
let builder: syn::ItemStruct = parse_quote! {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Will fix

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")?;
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The 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 with_durabilities which works like with_fields? The current with_durability method could stay, but I'm not sure if the name isn't slightly ambiguous, because we have multiple fields all with their own durability.

Copy link
Member Author

@OLUWAMUYIWA OLUWAMUYIWA Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about having with_durabilities too, but I dropped the idea because it sorts of requires the user to keep the order of fields in mind. I don't know if that cognitive overhead is okay. @MihailMihov, @nikomatsakis , @xffxff
Though personally, I think it's better.

Another option I'm considering is having with_field, and not with_fields, so we get to set the fields one by one and take a tuple, i.e. its value and it's durability as arguments. That localizes concerns. But, is taking tuples neat? Also, is setting the fields one by one okay?

Thanks Mihail!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about having with_durabilities too, but I dropped the idea because it sorts of requires the user to keep the order of fields in mind. I don't know if that cognitive overhead is okay.

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with_values would be a better name in my opinion, but the current is fine too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmn. with_fields makes it obvious to the user what they're about to set. Wouldn't with_values be a little ambiguous? I really don't know.

Copy link
Member

Choose a reason for hiding this comment

The 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 with_values is better in my opinion as it communicates that you're setting just the value of the field. If I saw with_fields my first thought would be that it takes a struct or tuple which contains everything about a field. Maybe someone else could comment on what is the better option, but I think both are good?

#(self.#field_names = Some(#field_names);)*
self
}

#build_fn
}
}
};

(buidler, impls)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(buidler, impls)
(builder, impls)

}

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.
Expand Down Expand Up @@ -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 {}
65 changes: 65 additions & 0 deletions salsa-2022-tests/tests/builder_api.rs
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a Result from build isn't useless, but I also feel like almost always having to unwrap it will be something very repetitive and rarely useful. If you agree with my suggestion that the durability should have a default I have a different idea, which I think will make the API better, instead of having the build method be the terminal one, we could instead move that over to with_fields and we'd no longer need to return a Result, because we know that we have the values and the other thing we need is the db, but there a panic would be fine.

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 salsa::Setter API. The only thing that I kind of don't like is how to fields method is named similarly to the other ones, which doesn't clearly show that it's the terminal method that actually builds the type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing that I kind of don't like is how to fields method is named similarly to the other ones, which doesn't clearly show that it's the terminal method that actually builds the type.

That's a real concern. I actually think builders should have build, and should naturally return Results except when the value we're building can comfortably have default values all through. In this case, fields cannot have default values. And, panicking inside with_fields? hmmn maybe people don't expect that. But I wouldn't know. I don't have much experience here. If you're so sure...

Maybe others have something to say?
@xffxff @nikomatsakis @crlf0710

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, fields cannot have default values.

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 build method and panic only if the other is missing, but it's a tradeoff where we make the API calls slightly shorter, but lose a bit of clarity of which method is the one that builds the type.
But panicking instead of returning Result and making either with_db or with_fields the build method are two separate things. If you're sure that the build should return Result you can still consider putting that logic in with_db or with_fields.

But I wouldn't know. I don't have much experience here. If you're so sure...

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming back to this with fresh eyes, a few thoughts--

  1. I think that we should strive to avoid Result that are statically known to not apply.
  2. I don't love having with_values take a tuple, I think I'd rather have a method per field.
  3. This seems to imply we may need to do "type tricks" to make a builder that is statically known to have a value for all fields etc.

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();
}
Loading