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

Support field dependencies #14

Open
chris-morgan opened this issue Mar 18, 2019 · 21 comments
Open

Support field dependencies #14

chris-morgan opened this issue Mar 18, 2019 · 21 comments

Comments

@chris-morgan
Copy link
Contributor

As a concrete example, when creating a customer in the Stripe API, there’s an argument tax_percent that is only permitted if plan has also been set. It is an error to set tax_percent if plan isn’t set, so it’s best to statically enforce that where possible.

I picture code looking like this:

#[derive(TypedBuilder)]
struct Customer {plan: T,
    #[builder(requires = "plan", default)]
    tax_percent: Option<T>,}

As schemed here, setting requires simply means “you are not permitted to override the default unless this field has been set”. Thus all requires things would need default or default_code.

An alternative scheme would allow you to specify different defaults depending on whether the required field was defined or not, e.g. replace default{,_code} with dependency_not_met_default{,_code} and dependency_met_default{,_code} (strawman names selected for lack of ambiguity).

This would work very well in conjunction with #6, extending “requires” to support arbitrary groups (named, or with any() &c. as in my comment there just now).

@idanarye
Copy link
Owner

There are two ways to do this:

  1. Forbid setting the dependent fields until its dependency is met. This means that:

    Customer::builder().tax_percent(...).plan(...).build()

    would be illegal, because even though you used both fields when you set the tax_percent there wasn't a plan yet. Instead, you'd have to use:

    Customer::builder().plan(...).tax_percent(...).build()
  2. Forbid building with unfulfilled dependencies. So

    Customer::builder().tax_percent(...).plan(...).build()

    would be OK, but

    Customer::builder().tax_percent(...).build()

    wouldn't.

While the second option seem more intuitive, I prefer the first option due to technical limitations:

  • I can't freely code when to allow build() and when to forbid it - I have to build a complex set of impl blocks to generate the required behavior. Doing this for the impl blocks of the individual dependant fields is much easier than doing it for the final impl block of the build().
  • I can't get a proper error message, because I rely on the nonexistence of the build() method. By making the tax_percent() fail to compile it will be easier for the user to determine what went wrong.

@chris-morgan
Copy link
Contributor Author

I had been assuming the first option. The second option would definitely be much, much messier technically, and would make life hard in IDEs and native error messages, and really scramble the docs on the builder type (which I value).

BTW, I’m quite happy to implement this myself.

@virtualritz
Copy link

I have need for this exact case too.

@mo8it
Copy link
Contributor

mo8it commented Jun 7, 2023

@idanarye Can I try this?

Sorry for spam with these questions, but it is frustrating to realize that the maintainer is not welcoming some PR after you have submitted it. Therefore I always ask first :)

@mo8it
Copy link
Contributor

mo8it commented Jun 13, 2023

I thought about it and any is complicated and has exponential scaling because you would have to implement the setter of the dependent field for the following configurations for the example any(a, b, c):

  1. a set, b unset, c unset
  2. a unset, b set, c unset
  3. a unset, b unset, c set
  4. a set, b set, c unset
  5. a set, b unset, c set
  6. a unset, b set, c set
  7. a set, b set, c set

This means that we have 2^3 - 1 = 7 combinations. For n fields in any, we get 2^n - 1 ⚠️

Let's say we remove any. Then we have to think about all and not.

all produces only one setter implementation. Therefore, I would implement it :D

not(a) on b is not helpful at all! It can be reflected in the setter of b, but we could just set a after setting b.

@idanarye
Copy link
Owner

Note that any is not really exponential. Your any(a, b, c) example only needs 3 cases:

  1. a set
  2. a unset, b set
  3. a unset, b unset, c set

@mo8it
Copy link
Contributor

mo8it commented Jun 13, 2023

WOW, I did not think about that!

@idanarye
Copy link
Owner

I did not come up with it either - it's just very similar to what @seb-bl's did in #30.

@mo8it
Copy link
Contributor

mo8it commented Jun 15, 2023

I did think about it for many hours and I do finally have results!

Let's look at any(a, b, c) to understand the notation that I did use:

a b c
S _ _
U S _
U U S

This means that _ stands for generic, U stands for unset, S stands for set.

The rows (except the first one) are the needed implementations.

all(a, b, c) is simple:

a b c
S S S

Now let's take a look at any(all(a, b), c):

a b c
S S _
? ? S

Problem: We can not replace both ? with U because then we can't set the field if c and (a xor b) are set.

This case has two solutions. But first, let's think about how these "implementation boards" are built.

  • A normal field sets only itself in an implementation row.
  • all sets all groups in an implementation row.
  • any generates an implementation row for every group.

After all S are placed in the board, fill the rest with _:

a b c
S S _
_ _ S

Now, cast a diagonal "U shadow" of the two S in the first row in the following way with S under the "shadow":

a b c
S S _
U _ S
S U S

This is one solution! The other solution which reduces the number of implementations is to sort the rows first based on their number of S entries:

a b c
_ _ S
S S _

Now we cast one "shadow" and we are done:

a b c
_ _ S
S S U

We know what problems we can get with combined any and all. How does the number of implementations scale?

Well, I have written an algorithm to test that: https://codeberg.org/mo8it/sus-impls

@mo8it
Copy link
Contributor

mo8it commented Jun 16, 2023

@idanarye I think that I did close all bugs in my algorithm groups. Do you want to try it out?

  1. Clone the repo
  2. Run cargo run 'GROUPS'. For example: cargo r 'all(any(a, b, c), any(d, e, f))'. You can use letters from a to z.

I have tests even with implementations on real Rust traits to make sure that there are no conflicts. It is possible that some weird case is not covered. Would it be possible to implement it and say in the release notes that it is kind of in beta?

The algorithm can be used for #6 too!

@idanarye
Copy link
Owner

A few words on how it works? Are you normalizing the condition?

@mo8it
Copy link
Contributor

mo8it commented Jun 18, 2023

I did start explaining the algorithm in the README. I will ping you when it is done.

Why would a normalization be needed? My plan was to check for conflicts in the input and error after finding one. An example for an input conflict is any(a, all(a, b)) which can be detected in the implementation matrix:

a b
S _
S S

I did implement that check today.

@idanarye
Copy link
Owner

It may be an overkill, but a normalization can potentially just remove these conflicts instead of rejecting them.

@idanarye
Copy link
Owner

I admit though that I hadn't study the algorithm yet, so I don't know how complicated it is. And I don't see an already existing crate for it.

@mo8it
Copy link
Contributor

mo8it commented Jun 19, 2023

I could remove the conflicts without normalization. But I don't think that the additional performance cost is worth it. I don't see why we should allow writing conflicting rules in the first place.

I would even go that far to think about adding a crate feature to optionally disable that input conflicts check because it is O(n^2) where n is the number of initial implementation rows.
Maybe we could use a feature like "less_debugging_sugar" that also removes the implementations for repeated and missing fields. But I am not sure how much it would improve the compilation time.

@mo8it
Copy link
Contributor

mo8it commented Jun 21, 2023

The documentation is finally done and I did fix some more bugs! I have been working on this algorithm for more than a week :D I did underestimate this issue xD

You can read the README and try out the algorithm by running the binary as described: https://codeberg.org/mo8it/sus-impls

@idanarye I am waiting for your feedback before starting a PR to use the algorithm in this crate.

How do we want to integrate the algorithm? Do we keep it as a separate crate or do we just add it to this one as a subcrate? The AGPL3 license is conflicting, right? I would downgrade it for this crate :/

@idanarye
Copy link
Owner

Yea, I'd rather keep this crate copyleft-free.

@AlexandreCassagne
Copy link

Screenshot 2023-11-18 at 12 21 32 The repo has added a second licence (MIT).

@mo8it
Copy link
Contributor

mo8it commented Nov 18, 2023

I wrote and documented the algorithm. The license is compatible. I also started a draft PR. I just have to sit down and finish that PR 😅
Sorry, I got distracted by a lot of stuff.

@AlexandreCassagne
Copy link

I wrote and documented the algorithm. The license is compatible. I also started a draft PR. I just have to sit down and finish that PR 😅

Sorry, I got distracted by a lot of stuff.

My bad for the pressure. Simply expressing it didn't have to be copyleft :-) didn't mean to rush anyone haha

@mo8it
Copy link
Contributor

mo8it commented Mar 20, 2024

I tried to get back to it again but it would require some time to get comfortable with the macros again. Therefore, I closed the PR #96 to not let you wait even longer and would ask @idanarye if he wants to implement it in typed-builder. I am ready to help on the sus_impls side if something is required. Just let me know.

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 a pull request may close this issue.

5 participants