[POC] Redo properties #2369
[POC] Redo properties #2369
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
My observations with You have a A wrapper struct around the builder keeps track of the "step" at which to builder is to store the state in a generic argument. This is kinda of hacky and I think there are now better ways to do it. After bumping to 1.56 I think we can do better and use Tl;dr: don't use generics to keep track of set props, use a const fn. Or I might be wrong and const fn is not sufficient as of now. EDIT: ah, I see there is const_deref which would allow this? but is not stabilized, I was under the impression it already was :( EDIT2: Look mah only two feature gates :( |
|
Can you maybe expand upon
I don't see why that is necessary. |
|
Another experiment, this one actually works on stable 1.56. |
We need to construct the builder struct. In order to do so, we need to able to name all the fields, which we can't because we have no idea (other than the ident) of the struct passed as extends. This is solved by requiring it to be yew/packages/yew-macro/src/properties_attr.rs Lines 29 to 101 in 4ffb023 I couldn't think of any other way to handle this case.
It seems that we have to know all the props upfront. We can't know that because in the proc-macro, all we have access to is the ident of ancestor. const FOO_PROPS: RequiredProps = RequiredProps {
own: &["a"],
ancestor: None,
};
const BAR_PROPS: RequiredProps = RequiredProps {
own: &["b"],
ancestor: Some(&FOO_PROPS),
};
const MORE_PROPS: RequiredProps = RequiredProps {
own: &["c"],
ancestor: Some(&BAR_PROPS),
};At the time of constructing more_props, we have no idea which props it can take and who they belong to. |
I didn't expect #[derive(PartialEq, Properties)]
struct BaseProps {
#[prop] required: String,
#[prop_or_default] optional: String,
}
#[derive(PartialEq, Properties)]
struct ExtendedProps {
#[prop] foo: String,
#[prop_extends] base: BaseProps, // only allow one item to be marked, corresponds to the extends =
}
// Expanded builders (not considering the wrapper for state)
struct BasePropsBuilder {
required: Option<String>,
optional: Option<String>,
}
struct ExtendedPropsBuilder {
foo: Option<String>,
base: BasePropsBuilder, // expanded from BaseProps::Builder
}
impl Deref for ExtendedPropsBuilder {
type Target = BasePropsBuilder;
fn deref(&self) -> &Self::Target {
&self.base
}
}
// + DerefMut. Stil can't implement the old style of validating all required props, since the type of BasePropsBuilder changesThat way props shouldn't need to be
Well, at the point of the const _: () = check_all_present(&Props::Builder::REQUIRED_PROPS, &[<list of all provided props>]);which errors if not all of them are given. No idea if this scales, if good enough error messages can be constructed or if this is really practical 😆 |
This approach looks really interesting to me but I can't wrap my head around it. Can you give some rough input and output of the macro? |
Input in comments, output in code. //#[derive(PartialEq, Properties)]
//struct FooProps {
// a: String,
//}
const FOO_PROPS: RequiredProps = RequiredProps {
own: &["a"],
ancestor: None,
};
//#[derive(PartialEq, Properties)]
//struct BarProps {
// b: String,
// #[prop_extends]
// base: FooProps,
//}
const BAR_PROPS: RequiredProps = RequiredProps {
own: &["b"],
ancestor: Some(&FOO_PROPS),
};
pub fn main() {
// html! { <Foo a="a string" /> }
const _: () = check_all_present(&FOO_PROPS, &["a"]); // No error
// html! { <Bar a="a string" /> }
const _: () = check_all_present(&BAR_PROPS, &["a"]); // error!
// although the error message is not too pretty
// html! { <Bar a="a string" b="another string" /> }
const _: () = check_all_present(&BAR_PROPS, &["a", "b"]); // No error
// Remember: this should only check that all props were provided, building them is separate.
} |
@WorldSEnder, The big problem comes from building because of function signatures. If you have a way to implement a builder with this pattern, I would love to hear it. |
|
@hamza1311 So as I understand this PR can become a draft as we will not be pursuing this change? |
|
#2396 exists. Closing this. |
|
For completeness, I have tried yet another approach to inheritance, and apart from requiring a feature ( |
|
That looks clean. Is there a way to backport this to stable? Instead of |
|
It might be possible to use the hashes of prop names to identify them with a low chance of collisions. Downside is that at the moment I have no idea for making the error messages readable then :/ |
|
I think we may be able to take a page from typed-builder's book and use deprecated warnings for errors. The compile error can be unreadable but that wouldn't matter |
|
It's that time of the month again, and I think this time I've done it! A stable version that seems like a very clean implementation: It would take some time to write the macro code for all this, but i think it's doable! |
This PR is a proof-of-concept which redoes properties. Right now, the new properties attribute macro is not feature complete and thus the code does not compile. However, this implementation demonstrates inheritance for props (see #1533)
It is possible to build upon on this implementation. While implementing this, I ran into some roadblocks that may or may not be possible to overcome while keeping certain API in mind.
My Observations
DefaultDeref/DerefMutto "emulate" inheritance for properties. This results in function signature of setters similar to:fn class(&mut self, value: AttrValue). These are called on the builder struct one-by-one and cannot be chained.Default) are checked at compile time. This is possible by following "typestate pattern". The function signature of the setter is used to ensure all required props are set and handle the default cases.html!macropanic!at runtime for missing props. This would ensure the function signature is the same.TL;DR
Having both, compile time verification of validity of passed props and a fix for #1533 (props inheritance) is impossible. We need to pick a poison and go with it.
We should explore more ways to solving the issue at hand: being able to pass props which are not explicitly defined in the Properties struct (may be defined else where)