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

Emit a deprecation warning on an encounter of a String field when deriving Properties #3382

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 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
47 changes: 46 additions & 1 deletion packages/yew-macro/src/derive_props/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ pub struct PropField {
}

impl PropField {
pub fn ty(&self) -> &Type {
&self.ty
}

/// All required property fields are wrapped in an `Option`
pub fn is_required(&self) -> bool {
matches!(self.attr, PropAttr::Required { .. })
Expand Down Expand Up @@ -323,6 +327,34 @@ fn is_path_an_option(path: &Path) -> bool {
is_path_segments_an_option(path.segments.iter().take(3).map(|ps| ps.ident.to_string()))
}

fn is_path_segments_a_string(path_segments: impl Iterator<Item = String>) -> bool {
fn is_string_path_seg(seg_index: usize, path: &str) -> u8 {
match (seg_index, path) {
(0, "alloc") => 0b001,
(0, "std") => 0b001,
(0, "String") => 0b111,
(1, "string") => 0b010,
(2, "String") => 0b100,
_ => 0,
}
}

path_segments
.enumerate()
.fold(0, |flags, (i, ps)| flags | is_string_path_seg(i, &ps))
== 0b111
}
its-the-shrimp marked this conversation as resolved.
Show resolved Hide resolved

/// returns true when the [`Path`] seems like a [`String`] type.
///
/// This function considers the following paths as Strings:
/// - std::string::String
/// - alloc::string::String
/// - String
pub(crate) fn is_path_a_string(path: &Path) -> bool {
is_path_segments_a_string(path.segments.iter().map(|ps| ps.ident.to_string()))
}

impl TryFrom<Field> for PropField {
type Error = Error;

Expand Down Expand Up @@ -379,7 +411,7 @@ impl PartialEq for PropField {

#[cfg(test)]
mod tests {
use crate::derive_props::field::is_path_segments_an_option;
use crate::derive_props::field::{is_path_segments_a_string, is_path_segments_an_option};

#[test]
fn all_std_and_core_option_path_seg_return_true() {
Expand All @@ -397,4 +429,17 @@ mod tests {
vec!["Option".to_owned(), "Vec".to_owned(), "Option".to_owned()].into_iter()
));
}

#[test]
fn all_std_and_alloc_string_seg_return_true() {
assert!(is_path_segments_a_string(
vec!["alloc".to_owned(), "string".to_owned(), "String".to_owned()].into_iter()
));
assert!(is_path_segments_a_string(
vec!["std".to_owned(), "string".to_owned(), "String".to_owned()].into_iter()
));
assert!(is_path_segments_a_string(
vec!["String".to_owned()].into_iter()
));
}
}
29 changes: 21 additions & 8 deletions packages/yew-macro/src/derive_props/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,14 @@ use std::convert::TryInto;
use builder::PropsBuilder;
use field::PropField;
use proc_macro2::{Ident, Span};
use proc_macro_error::emit_warning;
use quote::{format_ident, quote, ToTokens};
use syn::parse::{Parse, ParseStream, Result};
use syn::{Attribute, DeriveInput, Generics, Visibility};
use syn::spanned::Spanned;
use syn::{Attribute, DeriveInput, Generics, Type, TypePath, Visibility};
use wrapper::PropsWrapper;

use self::field::is_path_a_string;
use self::generics::to_arguments;

pub struct DerivePropsInput {
Expand Down Expand Up @@ -79,17 +82,27 @@ impl ToTokens for DerivePropsInput {
let Self {
generics,
props_name,
prop_fields,
preserved_attrs,
..
} = self;

for field in prop_fields {
match field.ty() {
Type::Path(TypePath { qself: None, path }) if is_path_a_string(path) => {
emit_warning!(
path.span(),
"storing string values with `String` is deprecated, use `AttrValue` \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not deprecated, just not recommended. We don't intend to take away the ability to do so. What is deprecated is the implicit conversion from String to AttrValue, but unfortunately we don't have a way to emit a warning for it.

Can you change the message accordingly? A link to documentation where downsides of using String is mentioned would be great to have as well

instead."
)
}
_ => (),
}
}

// The wrapper is a new struct which wraps required props in `Option`
let wrapper_name = format_ident!("{}Wrapper", props_name, span = Span::mixed_site());
let wrapper = PropsWrapper::new(
&wrapper_name,
generics,
&self.prop_fields,
&self.preserved_attrs,
);
let wrapper = PropsWrapper::new(&wrapper_name, generics, prop_fields, preserved_attrs);
tokens.extend(wrapper.into_token_stream());

// The builder will only build if all required props have been set
Expand All @@ -101,7 +114,7 @@ impl ToTokens for DerivePropsInput {
self,
&wrapper_name,
&check_all_props_name,
&self.preserved_attrs,
preserved_attrs,
);
let generic_args = to_arguments(generics);
tokens.extend(builder.into_token_stream());
Expand Down
1 change: 1 addition & 0 deletions packages/yew-macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ fn is_ide_completion() -> bool {
}
}

#[proc_macro_error::proc_macro_error]
#[proc_macro_derive(Properties, attributes(prop_or, prop_or_else, prop_or_default))]
pub fn derive_props(input: TokenStream) -> TokenStream {
let input = parse_macro_input!(input as DerivePropsInput);
Expand Down
Loading