From ce7702e9e73332b6ee0686f4b9fa364858ba8a0b Mon Sep 17 00:00:00 2001 From: Tim Kurdov Date: Sat, 23 Sep 2023 13:34:30 +0100 Subject: [PATCH] Remove special handing of struct fields of type `Option` in `derive(Properties)` (#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) --- packages/yew-macro/src/derive_props/field.rs | 98 +------------------ packages/yew-macro/tests/derive_props/fail.rs | 12 +++ .../yew-macro/tests/derive_props/fail.stderr | 79 ++++++++++----- packages/yew-macro/tests/derive_props/pass.rs | 23 ++++- packages/yew/src/suspense/component.rs | 1 + 5 files changed, 93 insertions(+), 120 deletions(-) diff --git a/packages/yew-macro/src/derive_props/field.rs b/packages/yew-macro/src/derive_props/field.rs index 4f8fd9186c9..37dc28c8bc3 100644 --- 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) -> 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 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 index bba6ab1cab5..93acb335801 100644 --- 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 + } + + 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 index 441b184c78d..881609aeaf9 100644 --- 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::::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` is not satisfied + --> tests/derive_props/fail.rs:47:24 + | +47 | ::yew::props!{ Props { } }; + | ^^^^^ the trait `HasProp` is not implemented for `AssertAllProps` + | + = help: the following other types implement trait `HasProp`: + as HasProp>> + as HasProp>> + as HasProp>> + as HasProp>> + as HasProp>> + as HasProp>> + as HasProp>> + as HasProp>> + and $N others +note: required because of the requirements on the impl of `HasAllProps` for `t4::CheckPropsAll` + --> tests/derive_props/fail.rs:41:21 + | +41 | #[derive(Clone, Properties, PartialEq)] + | ^^^^^^^^^^ + = note: required because of the requirements on the impl of `AllPropsFor` for `AssertAllProps` +note: required by a bound in `html::component::properties::__macro::PreBuild::::build` + --> $WORKSPACE/packages/yew/src/html/component/properties.rs + | + | Token: AllPropsFor, + | ^^^^^^^^^^^^^^^^^^^ required by this bound in `html::component::properties::__macro::PreBuild::::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,15 +148,15 @@ 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}` @@ -135,20 +164,20 @@ error[E0277]: expected a `FnOnce<()>` closure, found `{integer}` note: required by a bound in `Option::::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::::unwrap_or_else` error[E0271]: type mismatch resolving ` 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::::unwrap_or_else` diff --git a/packages/yew-macro/tests/derive_props/pass.rs b/packages/yew-macro/tests/derive_props/pass.rs index 9bee699e2c0..39086f80581 100644 --- 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, + #[prop_or_default] + optional: ::std::option::Option + } + #[::yew::function_component] + fn Inner(_props: &Props) -> ::yew::html::Html { + ::yew::html!{} + } + + #[::yew::function_component] + fn Main() -> ::yew::html::Html { + ::yew::html! {<> + + + } + } } fn main() {} diff --git a/packages/yew/src/suspense/component.rs b/packages/yew/src/suspense/component.rs index 17c68053fea..80c580f87a3 100644 --- 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, }