-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
bbfeb29
97cecf3
d7c868a
1a19956
719edac
0504b16
c7bd63e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 { .. }) | ||||||||||||||||||||||
|
@@ -293,22 +297,16 @@ 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 | ||||||||||||||||||||||
fn is_path_segments_an_option<'a, T>(mut iter: impl Iterator<Item = &'a T>) -> bool | ||||||||||||||||||||||
where | ||||||||||||||||||||||
T: 'a + ?Sized + PartialEq<str>, | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
iter.next().map_or(false, |first| { | ||||||||||||||||||||||
first == "Option" | ||||||||||||||||||||||
|| (first == "std" || first == "core") | ||||||||||||||||||||||
&& iter.next().map_or(false, |x| x == "option") | ||||||||||||||||||||||
&& iter.next().map_or(false, |x| x == "Option") | ||||||||||||||||||||||
}) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Returns true when the [`Path`] seems like an [`Option`] type. | ||||||||||||||||||||||
|
@@ -320,7 +318,29 @@ fn is_path_segments_an_option(path_segments: impl Iterator<Item = String>) -> bo | |||||||||||||||||||||
/// | ||||||||||||||||||||||
/// 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())) | ||||||||||||||||||||||
is_path_segments_an_option(path.segments.iter().map(|ps| &ps.ident)) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
fn is_path_segments_a_string<'a, T>(mut iter: impl Iterator<Item = &'a T>) -> bool | ||||||||||||||||||||||
where | ||||||||||||||||||||||
T: 'a + ?Sized + PartialEq<str>, | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
iter.next().map_or(false, |first| { | ||||||||||||||||||||||
first == "String" | ||||||||||||||||||||||
|| (first == "std" || first == "alloc") | ||||||||||||||||||||||
&& iter.next().map_or(false, |x| x == "string") | ||||||||||||||||||||||
&& iter.next().map_or(false, |x| x == "String") | ||||||||||||||||||||||
}) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// 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)) | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think is_path_segments_a_string is only used for testing purpose, so we can simply check the path here instead. This should also help with adding more variants in the future should we need it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to benchmark both variants, here's the code of the benchmarks: #![feature(test)]
extern crate test;
use syn::parse_quote as q;
use syn::Path;
use test::Bencher;
fn is_path_segments_a_string<'a, T>(mut iter: impl Iterator<Item = &'a T>) -> bool
where
T: 'a + ?Sized + PartialEq<str>,
{
iter.next().map_or(false, |first| {
first == "String"
|| (first == "std" || first == "alloc")
&& iter.next().map_or(false, |second| second == "string")
&& iter.next().map_or(false, |third| third == "String")
})
}
fn is_path_a_string_1(path: &Path) -> bool {
is_path_segments_a_string(path.segments.iter().map(|ps| &ps.ident))
}
fn is_path_a_string_2(path: &Path) -> bool {
path == &q!(String)
|| path == &q!(alloc::string::String)
|| path == &q!(std::string::String)
|| path == &q!(::alloc::string::String)
|| path == &q!(::std::string::String)
}
#[bench]
fn bench_1(b: &mut Bencher) {
b.iter(|| {
is_path_a_string_1(&q!(String));
is_path_a_string_1(&q!(::std::String));
is_path_a_string_1(&q!(std::string::String));
is_path_a_string_1(&q!(Vec));
})
}
#[bench]
fn bench_2(b: &mut Bencher) {
b.iter(|| {
is_path_a_string_2(&q!(String));
is_path_a_string_2(&q!(::std::String));
is_path_a_string_2(&q!(std::string::String));
is_path_a_string_2(&q!(Vec));
})
} here's the output of running 2 tests
test bench_1 ... bench: 4,079 ns/iter (+/- 137)
test bench_2 ... bench: 23,215 ns/iter (+/- 541)
The first approach is as readable as the second one, but is much faster There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whilst I do not believe the readability is the same (imagine if we need to add |
||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
impl TryFrom<Field> for PropField { | ||||||||||||||||||||||
|
@@ -379,22 +399,33 @@ impl PartialEq for PropField { | |||||||||||||||||||||
|
||||||||||||||||||||||
#[cfg(test)] | ||||||||||||||||||||||
mod tests { | ||||||||||||||||||||||
use crate::derive_props::field::is_path_segments_an_option; | ||||||||||||||||||||||
use std::iter::once; | ||||||||||||||||||||||
|
||||||||||||||||||||||
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() { | ||||||||||||||||||||||
assert!(is_path_segments_an_option( | ||||||||||||||||||||||
vec!["core".to_owned(), "option".to_owned(), "Option".to_owned()].into_iter() | ||||||||||||||||||||||
["core", "option", "Option"].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() | ||||||||||||||||||||||
["std", "option", "Option"].into_iter() | ||||||||||||||||||||||
)); | ||||||||||||||||||||||
assert!(is_path_segments_an_option(once("Option"))); | ||||||||||||||||||||||
// why OR instead of XOR | ||||||||||||||||||||||
assert!(is_path_segments_an_option( | ||||||||||||||||||||||
vec!["Option".to_owned(), "Vec".to_owned(), "Option".to_owned()].into_iter() | ||||||||||||||||||||||
["Option", "Vec", "Option"].into_iter() | ||||||||||||||||||||||
)); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
#[test] | ||||||||||||||||||||||
fn all_std_and_alloc_string_seg_return_true() { | ||||||||||||||||||||||
assert!(is_path_segments_a_string( | ||||||||||||||||||||||
["alloc", "string", "String"].into_iter() | ||||||||||||||||||||||
)); | ||||||||||||||||||||||
assert!(is_path_segments_a_string( | ||||||||||||||||||||||
["std", "string", "String"].into_iter() | ||||||||||||||||||||||
)); | ||||||||||||||||||||||
assert!(is_path_segments_a_string(once("String"))); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding an early return might help to make the conditions chain easier to read.
Also, do you think that could be a good idea to add some names like
second
andthird
?What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the
||
operator is perfectly readable and makes the function concise, but I do agree that giving descriptive names to closures' arguments is a worthwhile measure, I'll do that right away