You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is an answer to @mo8it's comment in rust-lang/rfcs#3458 (comment). I don't want to spam the RFC's comments (especially since GitHub does not support threads in PR comments) so I've opened this discussion instead.
Ignoring my opinion on the RFC itself - I think if you are using TypedBuilder for a struct with an unsafe field (for whatever definition of "unsafe field" that makes any kind of sense) then you are doing something very wrong.
If you have an unsafe field in your struct, it does not mean the field is unsafe by itself - it means that it holds some invariant about the struct, and the question of whether or not this invariant holds is a property of the entire struct, not just that one field's value.
Why is this a problem for TypedBuilder? Because a Typed Builder provides a setter for each field:
But if the invariant is a combined property of multiple fields, is it really a good idea to provide separate setters for them? Unsafety is not something that should eagerly propagate - you want to wrap it in a safe interface as soon as you can - so an unsafe builder is probably not a good idea.
let foo = Foo::builder().bar(1).baz(2).build().expect("safety invariant violated");
But this just converts the safety from compile time to runtime. Which is better than exposing an unsafe interface, but still not good enough. To explain why, let's take the example from the RFC - Vec - and pretend it had such a builder:
let my_vec = Vec::builder().buf(RawVec::with_capacity(4)).len(2).build().expect("`len` does not match what was allocated in the `buf`");
We want this code to panic. buf contains 4 cells of junk (because it's a raw vector and we did not put anything there), but len implies that the first two cells contain actual data. So build() should return an Err. But... how is it going to know exactly? RawVec does not contain any information about which cells of it contain actual data. That's Vec's job!
There may be cases where such a check is possible, but I doubt they are that common. You need the unsafe field because you need to keep track of an invariant - if you could determine it from the other fields (and it wouldn't be too costly), you wouldn't need the invariant field. The invariant field only makes sense if you are limiting the mutation to a closed set of methods that make sure to maintain the invariant. Having a public setter method for each field does the exact opposite.
You may come with the idea of having a single setter method that sets both buf and len from something that already contains that information - like a fixed size array:
let my_vec = Vec::builder().buf_and_len_from_fixed_size_array([100,200]).build();
Beside the fact that TypedBuilder does not support custom multi-field setters (unless you open the hood, which is both complex and unstable), at this point the Typed Builder becomes a clumsy overengineered version of:
let my_vec = Vec::from([100,200]);
Compared to this, Typed Builder was not even automating the boilerplate, because you'd have to implement buf_and_len_from_fixed_size_array yourself (and the macro will just help you connect it to the builder type)
It'd only make sense to make a builder for it if the struct would contain more fields, unrelated to the invariant. But this is a big anti-pattern. Unsafety is something that should be localized and contained as much as possible. Putting fields unrelated to the unsafety violates this important rule, because they become part of the encapsulation and need to be considered in every mutating method - even though they are not related to the invariant. I don't want Typed Builder to encourage this - I want to encourage putting the fields related to the invariant in their own struct, one that does not have a Typed Builder, and then you can put that struct, together with the completely safe fields, inside a struct with a Typed Builder.
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
-
This is an answer to @mo8it's comment in rust-lang/rfcs#3458 (comment). I don't want to spam the RFC's comments (especially since GitHub does not support threads in PR comments) so I've opened this discussion instead.
Ignoring my opinion on the RFC itself - I think if you are using
TypedBuilder
for astruct
with an unsafe field (for whatever definition of "unsafe field" that makes any kind of sense) then you are doing something very wrong.If you have an unsafe field in your struct, it does not mean the field is unsafe by itself - it means that it holds some invariant about the struct, and the question of whether or not this invariant holds is a property of the entire struct, not just that one field's value.
Why is this a problem for
TypedBuilder
? Because a Typed Builder provides a setter for each field:But if the invariant is a combined property of multiple fields, is it really a good idea to provide separate setters for them? Unsafety is not something that should eagerly propagate - you want to wrap it in a safe interface as soon as you can - so an unsafe builder is probably not a good idea.
If we had the invariants (#67) we could do this:
But this just converts the safety from compile time to runtime. Which is better than exposing an unsafe interface, but still not good enough. To explain why, let's take the example from the RFC -
Vec
- and pretend it had such a builder:We want this code to panic.
buf
contains 4 cells of junk (because it's a raw vector and we did not put anything there), butlen
implies that the first two cells contain actual data. Sobuild()
should return anErr
. But... how is it going to know exactly?RawVec
does not contain any information about which cells of it contain actual data. That'sVec
's job!There may be cases where such a check is possible, but I doubt they are that common. You need the unsafe field because you need to keep track of an invariant - if you could determine it from the other fields (and it wouldn't be too costly), you wouldn't need the invariant field. The invariant field only makes sense if you are limiting the mutation to a closed set of methods that make sure to maintain the invariant. Having a public setter method for each field does the exact opposite.
You may come with the idea of having a single setter method that sets both
buf
andlen
from something that already contains that information - like a fixed size array:Beside the fact that
TypedBuilder
does not support custom multi-field setters (unless you open the hood, which is both complex and unstable), at this point the Typed Builder becomes a clumsy overengineered version of:Compared to this, Typed Builder was not even automating the boilerplate, because you'd have to implement
buf_and_len_from_fixed_size_array
yourself (and the macro will just help you connect it to the builder type)It'd only make sense to make a builder for it if the struct would contain more fields, unrelated to the invariant. But this is a big anti-pattern. Unsafety is something that should be localized and contained as much as possible. Putting fields unrelated to the unsafety violates this important rule, because they become part of the encapsulation and need to be considered in every mutating method - even though they are not related to the invariant. I don't want Typed Builder to encourage this - I want to encourage putting the fields related to the invariant in their own struct, one that does not have a Typed Builder, and then you can put that struct, together with the completely safe fields, inside a struct with a Typed Builder.
Beta Was this translation helpful? Give feedback.
All reactions