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

Remove special handing of struct fields of type Option in derive(Properties) #3398

Merged
merged 6 commits into from
Sep 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 4 additions & 94 deletions packages/yew-macro/src/derive_props/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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>,
}
}

Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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()
));
}
}
12 changes: 12 additions & 0 deletions packages/yew-macro/tests/derive_props/fail.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
79 changes: 54 additions & 25 deletions packages/yew-macro/tests/derive_props/fail.stderr
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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`
23 changes: 22 additions & 1 deletion packages/yew-macro/tests/derive_props/pass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 } };
}
}
Expand All @@ -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() {}
1 change: 1 addition & 0 deletions packages/yew/src/suspense/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
}

Expand Down
Loading