Remove special handing of struct fields of type Option in `derive(P…

…roperties)` (#3398) * removed PropAttr::Option * fixed formatting * removed an irrelevant test * added a test for converting value into Some(value) in the html! macro * added more tests for derive(Properties) * added more tests (again)
yewstack · Sep 23, 2023 · ce7702e · ce7702e
1 parent b71dbfe
commit ce7702e
Show file tree

Hide file tree

Showing 5 changed files with 93 additions and 120 deletions.
diff --git a/packages/yew-macro/src/derive_props/field.rs b/packages/yew-macro/src/derive_props/field.rs
@@ -5,10 +5,7 @@ use proc_macro2::{Ident, Span};
 use quote::{format_ident, quote, quote_spanned};
 use syn::parse::Result;
 use syn::spanned::Spanned;
-use syn::{
-    parse_quote, Attribute, Error, Expr, Field, GenericParam, Generics, Path, Type, TypePath,
-    Visibility,
-};
+use syn::{parse_quote, Attribute, Error, Expr, Field, GenericParam, Generics, Type, Visibility};
 
 use super::should_preserve_attr;
 use crate::derive_props::generics::push_type_param;
@@ -17,7 +14,6 @@ use crate::derive_props::generics::push_type_param;
 #[derive(PartialEq, Eq)]
 enum PropAttr {
     Required { wrapped_name: Ident },
-    Option,
     PropOr(Expr),
     PropOrElse(Expr),
     PropOrDefault,
@@ -82,11 +78,6 @@ impl PropField {
                     #name: ::std::option::Option::unwrap(this.wrapped.#wrapped_name),
                 }
             }
-            PropAttr::Option => {
-                quote! {
-                    #name: this.wrapped.#name,
-                }
-            }
             PropAttr::PropOr(value) => {
                 quote_spanned! {value.span()=>
                     #name: ::std::option::Option::unwrap_or(this.wrapped.#name, #value),
@@ -115,19 +106,9 @@ impl PropField {
         let ty = &self.ty;
         let extra_attrs = &self.extra_attrs;
         let wrapped_name = self.wrapped_name();
-        match &self.attr {
-            PropAttr::Option => {
-                quote! {
-                    #( #extra_attrs )*
-                    #wrapped_name: #ty,
-                }
-            }
-            _ => {
-                quote! {
-                    #( #extra_attrs )*
-                    #wrapped_name: ::std::option::Option<#ty>,
-                }
-            }
+        quote! {
+            #( #extra_attrs )*
+            #wrapped_name: ::std::option::Option<#ty>,
         }
     }
 
@@ -164,19 +145,6 @@ impl PropField {
                     }
                 }
             }
-            PropAttr::Option => {
-                quote! {
-                    #[doc(hidden)]
-                    #vis fn #name<#token_ty>(
-                        &mut self,
-                        token: #token_ty,
-                        value: impl ::yew::html::IntoPropValue<#ty>,
-                    ) -> #token_ty {
-                        self.wrapped.#name = value.into_prop_value();
-                        token
-                    }
-                }
-            }
             _ => {
                 quote! {
                     #[doc(hidden)]
@@ -216,12 +184,6 @@ impl PropField {
             } else {
                 unreachable!()
             }
-        } else if matches!(
-            &named_field.ty,
-            Type::Path(TypePath { path, .. })
-            if is_path_an_option(path)
-        ) {
-            Ok(PropAttr::Option)
         } else {
             let ident = named_field.ident.as_ref().unwrap();
             let wrapped_name = format_ident!("{}_wrapper", ident, span = Span::mixed_site());
@@ -294,36 +256,6 @@ impl<'a> PropFieldCheck<'a> {
     }
 }
 
-fn is_path_segments_an_option(path_segments: impl Iterator<Item = String>) -> bool {
-    fn is_option_path_seg(seg_index: usize, path: &str) -> u8 {
-        match (seg_index, path) {
-            (0, "core") => 0b001,
-            (0, "std") => 0b001,
-            (0, "Option") => 0b111,
-            (1, "option") => 0b010,
-            (2, "Option") => 0b100,
-            _ => 0,
-        }
-    }
-
-    path_segments
-        .enumerate()
-        .fold(0, |flags, (i, ps)| flags | is_option_path_seg(i, &ps))
-        == 0b111
-}
-
-/// Returns true when the [`Path`] seems like an [`Option`] type.
-///
-/// This function considers the following paths as Options:
-/// - core::option::Option
-/// - std::option::Option
-/// - Option::*
-///
-/// Users can define their own [`Option`] type and this will return true - this is unavoidable.
-fn is_path_an_option(path: &Path) -> bool {
-    is_path_segments_an_option(path.segments.iter().take(3).map(|ps| ps.ident.to_string()))
-}
-
 impl TryFrom<Field> for PropField {
     type Error = Error;
 
@@ -369,25 +301,3 @@ impl PartialEq for PropField {
         self.name == other.name
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use crate::derive_props::field::is_path_segments_an_option;
-
-    #[test]
-    fn all_std_and_core_option_path_seg_return_true() {
-        assert!(is_path_segments_an_option(
-            vec!["core".to_owned(), "option".to_owned(), "Option".to_owned()].into_iter()
-        ));
-        assert!(is_path_segments_an_option(
-            vec!["std".to_owned(), "option".to_owned(), "Option".to_owned()].into_iter()
-        ));
-        assert!(is_path_segments_an_option(
-            vec!["Option".to_owned()].into_iter()
-        ));
-        // why OR instead of XOR
-        assert!(is_path_segments_an_option(
-            vec!["Option".to_owned(), "Vec".to_owned(), "Option".to_owned()].into_iter()
-        ));
-    }
-}
diff --git a/packages/yew-macro/tests/derive_props/fail.rs b/packages/yew-macro/tests/derive_props/fail.rs
@@ -36,6 +36,18 @@ mod t3 {
     }
 }
 
+mod t4 {
+    use super::*;
+    #[derive(Clone, Properties, PartialEq)]
+    pub struct Props {
+        value: Option<String>
+    }
+
+    fn required_option_should_be_provided() {
+        ::yew::props!{ Props { } };
+    }
+}
+
 mod t5 {
     use super::*;
     #[derive(Clone, Properties, PartialEq)]

diff --git a/packages/yew-macro/tests/derive_props/fail.stderr b/packages/yew-macro/tests/derive_props/fail.stderr
@@ -1,7 +1,7 @@
 error: unexpected end of input, expected expression
-  --> tests/derive_props/fail.rs:44:19
+  --> tests/derive_props/fail.rs:56:19
    |
-44 |         #[prop_or()]
+56 |         #[prop_or()]
    |                   ^
 
 error: cannot find attribute `props` in this scope
@@ -11,18 +11,18 @@ error: cannot find attribute `props` in this scope
    |           ^^^^^
 
 error[E0425]: cannot find value `foo` in this scope
-   --> tests/derive_props/fail.rs:74:24
+   --> tests/derive_props/fail.rs:86:24
     |
-74  |         #[prop_or_else(foo)]
+86  |         #[prop_or_else(foo)]
     |                        ^^^ not found in this scope
     |
 note: these functions exist but are inaccessible
-   --> tests/derive_props/fail.rs:88:5
+   --> tests/derive_props/fail.rs:100:5
     |
-88  |     fn foo(bar: i32) -> String {
+100 |     fn foo(bar: i32) -> String {
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ `crate::t9::foo`: not accessible
 ...
-102 |     fn foo() -> i32 {
+114 |     fn foo() -> i32 {
     |     ^^^^^^^^^^^^^^^ `crate::t10::foo`: not accessible
 
 error[E0277]: the trait bound `Value: Default` is not satisfied
@@ -107,10 +107,39 @@ note: required by a bound in `html::component::properties::__macro::PreBuild::<T
    |                    ^^^^^^^^^^^^^^^^^^^ required by this bound in `html::component::properties::__macro::PreBuild::<Token, B>::build`
    = note: this error originates in the derive macro `Properties` (in Nightly builds, run with -Z macro-backtrace for more info)
 
+error[E0277]: the trait bound `AssertAllProps: HasProp<t4::_Props::value, _>` is not satisfied
+  --> tests/derive_props/fail.rs:47:24
+   |
+47 |         ::yew::props!{ Props { } };
+   |                        ^^^^^ the trait `HasProp<t4::_Props::value, _>` is not implemented for `AssertAllProps`
+   |
+   = help: the following other types implement trait `HasProp<P, How>`:
+             <CheckChildrenPropsAll<B> as HasProp<P, &dyn HasProp<P, How>>>
+             <CheckContextProviderPropsAll<B> as HasProp<P, &dyn HasProp<P, How>>>
+             <HasContextProviderPropschildren<B> as HasProp<P, &dyn HasProp<P, How>>>
+             <HasContextProviderPropschildren<B> as HasProp<children, HasContextProviderPropschildren<B>>>
+             <HasContextProviderPropscontext<B> as HasProp<P, &dyn HasProp<P, How>>>
+             <HasContextProviderPropscontext<B> as HasProp<yew::context::_ContextProviderProps::context, HasContextProviderPropscontext<B>>>
+             <suspense::component::CheckSuspensePropsAll<B> as HasProp<P, &dyn HasProp<P, How>>>
+             <t10::CheckPropsAll<B> as HasProp<P, &dyn HasProp<P, How>>>
+           and $N others
+note: required because of the requirements on the impl of `HasAllProps<t4::Props, (_,)>` for `t4::CheckPropsAll<AssertAllProps>`
+  --> tests/derive_props/fail.rs:41:21
+   |
+41 |     #[derive(Clone, Properties, PartialEq)]
+   |                     ^^^^^^^^^^
+   = note: required because of the requirements on the impl of `AllPropsFor<t4::PropsBuilder, (_,)>` for `AssertAllProps`
+note: required by a bound in `html::component::properties::__macro::PreBuild::<Token, B>::build`
+  --> $WORKSPACE/packages/yew/src/html/component/properties.rs
+   |
+   |             Token: AllPropsFor<B, How>,
+   |                    ^^^^^^^^^^^^^^^^^^^ required by this bound in `html::component::properties::__macro::PreBuild::<Token, B>::build`
+   = note: this error originates in the derive macro `Properties` (in Nightly builds, run with -Z macro-backtrace for more info)
+
 error[E0308]: mismatched types
-  --> tests/derive_props/fail.rs:54:19
+  --> tests/derive_props/fail.rs:66:19
    |
-54 |         #[prop_or(123)]
+66 |         #[prop_or(123)]
    |                   ^^^
    |                   |
    |                   expected struct `String`, found integer
@@ -119,36 +148,36 @@ error[E0308]: mismatched types
 note: associated function defined here
 help: try using a conversion method
    |
-54 |         #[prop_or(123.to_string())]
+66 |         #[prop_or(123.to_string())]
    |                      ++++++++++++
-54 |         #[prop_or(123.to_string())]
+66 |         #[prop_or(123.to_string())]
    |                      ++++++++++++
 
 error[E0277]: expected a `FnOnce<()>` closure, found `{integer}`
-  --> tests/derive_props/fail.rs:64:24
+  --> tests/derive_props/fail.rs:76:24
    |
-64 |         #[prop_or_else(123)]
+76 |         #[prop_or_else(123)]
    |                        ^^^ expected an `FnOnce<()>` closure, found `{integer}`
    |
    = help: the trait `FnOnce<()>` is not implemented for `{integer}`
    = note: wrap the `{integer}` in a closure with no arguments: `|| { /* code */ }`
 note: required by a bound in `Option::<T>::unwrap_or_else`
 
 error[E0593]: function is expected to take 0 arguments, but it takes 1 argument
-  --> tests/derive_props/fail.rs:84:24
-   |
-84 |         #[prop_or_else(foo)]
-   |                        ^^^ expected function that takes 0 arguments
+   --> tests/derive_props/fail.rs:96:24
+    |
+96  |         #[prop_or_else(foo)]
+    |                        ^^^ expected function that takes 0 arguments
 ...
-88 |     fn foo(bar: i32) -> String {
-   |     -------------------------- takes 1 argument
-   |
+100 |     fn foo(bar: i32) -> String {
+    |     -------------------------- takes 1 argument
+    |
 note: required by a bound in `Option::<T>::unwrap_or_else`
 
 error[E0271]: type mismatch resolving `<fn() -> i32 {t10::foo} as FnOnce<()>>::Output == String`
-  --> tests/derive_props/fail.rs:98:24
-   |
-98 |         #[prop_or_else(foo)]
-   |                        ^^^ expected struct `String`, found `i32`
-   |
+   --> tests/derive_props/fail.rs:110:24
+    |
+110 |         #[prop_or_else(foo)]
+    |                        ^^^ expected struct `String`, found `i32`
+    |
 note: required by a bound in `Option::<T>::unwrap_or_else`
diff --git a/packages/yew-macro/tests/derive_props/pass.rs b/packages/yew-macro/tests/derive_props/pass.rs
@@ -227,7 +227,7 @@ mod t12 {
     }
 
     fn optional_prop_generics_should_work() {
-        ::yew::props! { Props::<::std::primitive::bool> { } };
+        ::yew::props! { Props::<::std::primitive::bool> { value: ::std::option::Option::Some(true) } };
         ::yew::props! { Props::<::std::primitive::bool> { value: true } };
     }
 }
@@ -249,7 +249,28 @@ mod raw_field_names {
         r#true: u32,
         r#pointless_raw_name: u32,
     }
+}
+
+mod value_into_some_value_in_props {
+    #[derive(::std::cmp::PartialEq, ::yew::Properties)]
+    struct Props {
+        required: ::std::option::Option<usize>,
+        #[prop_or_default]
+        optional: ::std::option::Option<usize>
+    }
 
+    #[::yew::function_component]
+    fn Inner(_props: &Props) -> ::yew::html::Html {
+        ::yew::html!{}
+    }
+
+    #[::yew::function_component]
+    fn Main() -> ::yew::html::Html {
+        ::yew::html! {<>
+            <Inner required=3 optional=5/>
+            <Inner required={::std::option::Option::Some(6)}/>
+        </>}
+    }
 }
 
 fn main() {}
diff --git a/packages/yew/src/suspense/component.rs b/packages/yew/src/suspense/component.rs
@@ -25,6 +25,7 @@ mod feat_csr_ssr {
     #[derive(Properties, PartialEq, Debug, Clone)]
     pub(crate) struct BaseSuspenseProps {
         pub children: Html,
+        #[prop_or(None)]
         pub fallback: Option<Html>,
     }