Skip to content

Conversation

cormacrelf
Copy link

@cormacrelf cormacrelf commented May 7, 2023

This adds quite a lot of functionality. The intended use case was a struct like this. It obviously has a problem because if you do not specify update or specialized_initial, then the compiler doesn't know what type they should be. This is because the macro was not utilising the default type params (which provide fn-pointers, which are enough to compile with since they're never executed because they're None).

#[derive(Builder)]
pub struct ClosureFold<K, V, R, FAdd, FRemove,
    FUpdate = for<'a> fn(R, &'a K, &'a V, &'a V) -> R,
    FInitial = for<'a> fn(R, &::im_rc::OrdMap<K, V>) -> R
> where
    FAdd: for<'a> FnMut(R, &'a K, &'a V) -> R + 'static,
    FRemove: for<'a> FnMut(R, &'a K, &'a V) -> R + 'static,
    FUpdate: for<'a> FnMut(R, &'a K, &'a V, &'a V) -> R + 'static,
    FInitial: for<'a> FnMut(R, &::im_rc::OrdMap<K, V>) -> R + 'static,
{
    pub add: FAdd,
    pub remove: FRemove,
    #[default(None)]
    pub update: Option<FUpdate>,
    #[default(None)]
    pub specialized_initial: Option<FInitial>,
    #[default(false)]
    pub revert_to_init_when_empty: bool,
    #[default(PhantomData)]
    #[hidden]
    pub phantom: PhantomData<(K, V, R)>,
}

The solution required a bunch of changes:

  1. Make the fn new() return a builder with the default types substituted in their spots.
  2. When you provide update, the FUpdate closure type can be different from the default. Hence you need to get the setter method to infer the FUpdate, and replace it in the return type. This is done using fn update<FUpdate_>(update: FUpdate_) and doing lots of substitutions on the bounds / generic params.
  3. However, because you don't know what shape the field type will be, you need to tell the macro which params to do this with. Hence there's a new #[infer(T1, T2)] param. For this example you apply it to the update and specialized_initial fields, with #[infer(FUpdate)] and #[infer(FInitial)] respectively.
  4. Lots of minor improvements, much of it relating to commas and macro edge cases.
  5. Finally, an extra feature for fields like phantom: PhantomData<T> where T was specified with a default type T = f64 + a separate #[infer(T)] field. It needed special handling because if the macro writes a PhantomData in the fn new(), then it has to write another one with the inferred T_ (which means a different PhantomData<T_> type needs to be in the phantom field. So I introduced "late binding" of defaults, where they are only populated in the fn build() function, but the types are carried along the way. This required simple compile-time reflection in the form of the refl crate, which I pulled a few select functions from, in order to prove that if you still had a Some(Setter::LateBoundDefault) in the field, that basically T = T_. It worked pretty well!
  6. Late binding is applied by default to #[hidden] fields. It also needed some way of doing it while also generating a setter method, so I made #[late_bound_default], but it might need a better name.

Altogether it needs some more docs, and I want to split out some of the example code, but the rest of it should be reviewable.

@SeokminHong
Copy link
Owner

Thanks for your kind contribution! I'll look at it.

@cormacrelf
Copy link
Author

One todo I forgot to mention is reproducing what I did for normal setters in the lazy and async cases. I think it should be easy. At the very least they could produce a compile_error!("infer not supported with async setter"), if implementing that proves complicated.


tokens.extend(quote! {
impl <#impl_tokens> #ident <#(#lifetimes,)* #ty_tokens> #where_clause {
impl <#impl_tokens> #ident <#(#lifetimes,)* #ty_tokens> #where_tokens {
Copy link
Owner

@SeokminHong SeokminHong May 9, 2023

Choose a reason for hiding this comment

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

struct X<A, B = f64> {
    a: A,
    b: B
}

It will generate the following code for the above struct:

impl<A> X<A, f64> {
    fn new<'fn_lifetime>() -> XBuilder<'fn_lifetime, A, f64, (), (), (), (), ()> {}
}

X::<i32>::new(); // ok
X::new().a(3); // ok
X::new().a(3).b(5); // expected: f64, given: i32
X::<i32, i32>::new(); // No associated function `new`

For the as-is way, it will generate the following code:

impl<A, B> X<A, B> {
    fn new<'fn_lifetime>() -> XBuilder<'fn_lifetime, A, B, (), (), (), (), ()> {}
}

X::<i32>::new(); // ok
X::new().a(3); // Cannot infer B
X::new().a(3).b(5); // ok
X::<i32, i32>::new(); // ok

I think the as-is case can handle more cases. Is this change required for implementing the infer attribute?

Copy link
Author

@cormacrelf cormacrelf May 9, 2023

Choose a reason for hiding this comment

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

This is a bit like the difference between HashMap::new and HashMap::default, which becomes apparent when you use an alternative hash algorithm (e.g. fnv::FnvHashMap).

impl<K, V> HashMap<K, V, RandomState> { fn new() -> Self { ... } }
// whereas
impl<K, V, S> Default for HashMap<K, V, S> where S: Default { ... }

To use any other hasher than the default DOS-protected RandomState one, you can't call new, you must use e.g. fnv::FnvHashMap::default(). The standard library chose to write new that way because otherwise it's impossible to infer S in 99% of people's code: the S parameter is stored in a PhantomData, not passed in, and hence not inferable except when your hash map is about to be returned from a function (with -> HashMap<K, V> and hence a default S param) or passed to a function / placed in a structure.

If we translate that into builder-pattern's terms, the use case is when the b field has a #[default(...)] attribute and is therefore optional. In those cases you need the type checker to choose something when you do not provide b.

I think in general you need two ways of creating a builder, one with the default types substituted (like HashMap::new) and one with them as generic params (like HashMap::default). In my view the most user-friendly way to do it would be to do the same thing as HashMap, and have the Default impl be the generic one. This is also how all the A = Global allocator params on all of the standard library's collection types are done. Vec::new() gives you a Vec<T, Global>, and so on.

It seems that this PR's behaviour would be a breaking change to builder-pattern, if it's the only option. So maybe we could add a struct-level attribute to accept & substitute the default type params in the new function. I still think it makes more sense to follow the standard library's behaviour and put out a version bump, but at least there are options. In either case implementing Default is a good idea.

Copy link
Author

@cormacrelf cormacrelf May 9, 2023

Choose a reason for hiding this comment

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

The ability to choose which ones are substituted in the new function would be quite nice. Here's a comparison of two methods of choosing.

Method Breaking change? Behaviour without customization what the attribute does
#[infer_new(B)] struct X... yes like std's HashMap, Vec, etc allows inference of B in new, prevents #[default(...)] b: B from working
#[new_default(B)] struct X... no like existing builder-pattern, not like std substitutes B=f64 in new, allows #[default(...)] b: B to work

#[infer_new] also matches up with #[infer], learn it once and understand both. Whereas there is no way to name the #[new_default] attribute in a way that doesn't require reading docs eight times, because there are already #[default] attributes that do completely different stuff.

Copy link
Owner

@SeokminHong SeokminHong May 9, 2023

Choose a reason for hiding this comment

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

I understand what you meant, and I think substituting way is more suit for builder-pattern. In my thought, it doesn't seem idiomatic to me that cannot call the new function for overriding default types.

@SeokminHong
Copy link
Owner

@cormacrelf Using refl seems amazing! I wonder why you distinguish late_bound_default and default? It seems that using just late_bound_default only can handle every case.

@cormacrelf
Copy link
Author

Great question, I haven't tried it with everything late bound AND the refl tricks. It broke examples/default-fn.re without the refl cast IIRC.

@SeokminHong
Copy link
Owner

SeokminHong commented May 10, 2023

@cormacrelf
I finally understand the meaning of this PR 😂. It was quite difficult to understand for me at first because I didn't expect this detailed usage.

Now I agree your suggestion works like Default::default methods for implicit type inference. However, it adds some complexities to declaring attributes. So, what about using infer and late_bound_default by default for each field?

If you agree, I'll merge this PR to the new dev branch and work on it.

@cormacrelf
Copy link
Author

cormacrelf commented May 10, 2023

If #[infer] were the default, you would need to disable it (e.g. #[no_infer(T)]) on every other field. This is what happens when you simulate your proposed behaviour:

image

It fails because the B_ param inserted into fn field_a<B_>(field_a: A) -> XBuilder<A, B_, ...> does not appear in the field type. It's not inferable. Most fields will not be able to infer anything about type params that don't appear in their setter function args, so most fields will end up having a completely free type parameter that the compiler has no way of choosing something for.

The best you could do i think would be trying to find the type param ident inside the field type, and automatically infer that param if it appears. For this example, field_b: B obviously has a B in it, so it would automatically get #[infer(B)]. You'd be looking for B at the beginning of a syn::Path, or something. The token-stream type substitution is a bit dumber though.

With struct X<A, B=f64, C=f64>:

field type auto-infer?
B #[infer(B)]
(A, B) #[infer(B)], no need to infer A as that's done throughout the builder calls
(B, C) #[infer(B, C)]
Vec<(A, C)> #[infer(C)]

@SeokminHong
Copy link
Owner

I think it would be possible to implement it as your table. When users set default types for generic types for a struct, I think it also implies that the types are inferred automatically. I'll try it this weekend!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants