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

its-the-shrimp · 2023-08-21T13:51:51Z

Description

Makes the derive(Properties) macro emit a warning when encountering a field of type String in the struct, which suggests to use AttrValue instead.

Adsing this warning raises 2 questions:

Should it also report using other string types in Rust's standard library as deprecated?
Should we fix the tests which now raise the new warning or should we leave them as they are for them to be the tests for the new warning?

Checklist

I have reviewed my own code
I have added tests

github-actions · 2023-08-21T13:54:24Z

Visit the preview URL for this PR (updated for commit c7bd63e):

https://yew-rs-api--pr3382-add-string-prop-fiel-n2djdgpk.web.app

_{(expires Tue, 29 Aug 2023 15:28:46 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

github-actions · 2023-08-21T14:00:09Z

Size Comparison

examples	master (KB)	pull request (KB)	diff (%)
async_clock	102.316	102.316	0.000%
boids	175.516	175.516	0.000%
communication_child_to_parent	95.226	95.226	0.000%
communication_grandchild_with_grandparent	108.895	108.895	0.000%
communication_grandparent_to_grandchild	105.543	105.543	0.000%
communication_parent_to_child	92.697	92.697	0.000%
contexts	110.885	110.885	0.000%
counter	89.208	89.208	0.000%
counter_functional	89.923	89.923	0.000%
dyn_create_destroy_apps	92.237	92.237	0.000%
file_upload	103.544	103.544	0.000%
function_memory_game	174.320	174.320	0.000%
function_router	352.376	352.376	0.000%
function_todomvc	163.354	163.354	0.000%
futures	227.708	227.708	0.000%
game_of_life	112.298	112.298	0.000%
immutable	188.933	188.933	0.000%
inner_html	86.012	86.012	0.000%
js_callback	112.998	112.998	0.000%
keyed_list	201.203	201.203	0.000%
mount_point	89.208	89.208	0.000%
nested_list	114.501	114.501	0.000%
node_refs	96.160	96.160	0.000%
password_strength	1582.822	1582.822	0.000%
portals	98.235	98.235	0.000%
router	318.365	318.365	0.000%
simple_ssr	143.640	143.640	0.000%
ssr_router	389.579	389.579	0.000%
suspense	118.644	118.644	0.000%
timer	91.755	91.755	0.000%
timer_functional	100.285	100.285	0.000%
todomvc	143.697	143.697	0.000%
two_apps	89.916	89.916	0.000%
web_worker_fib	154.517	154.517	0.000%
webgl	88.476	88.476	0.000%

✅ None of the examples has changed their size significantly.

github-actions · 2023-08-21T14:01:30Z

Benchmark - SSR

Yew Master

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	425.275	443.131	431.272	6.992
Hello World	10	753.379	797.022	764.166	14.831
Function Router	10	2499.345	2641.066	2526.388	42.980
Concurrent Task	10	1007.581	1012.871	1009.527	1.538
Many Providers	10	1695.328	1765.432	1727.520	26.692

Pull Request

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	424.969	438.085	427.026	3.941
Hello World	10	736.351	756.885	739.457	6.281
Function Router	10	2489.429	2501.457	2495.839	3.725
Concurrent Task	10	1008.343	1011.174	1009.733	0.967
Many Providers	10	1676.206	1735.954	1699.866	24.006

cecton

LGTM

packages/yew-macro/src/derive_props/field.rs

ranile

Can you also add a test case for it?

ranile · 2023-08-21T16:23:59Z

packages/yew-macro/src/derive_props/mod.rs

+                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` \


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

packages/yew-macro/src/derive_props/field.rs

…ctions

its-the-shrimp · 2023-08-21T23:24:36Z

I couldn't find a way to explicitly test if a warning is emitted by a derive macro, but there are already some tests that raise this warning, for example here

msga-mmm · 2023-08-22T04:15:13Z

packages/yew-macro/src/derive_props/field.rs

+    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")
+    })


I think adding an early return might help to make the conditions chain easier to read.

Suggested change

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")

})

iter.next().map_or(false, |first| {

if first == "String" {

return true;

}

(first == "std" || first == "alloc")

&& iter.next().map_or(false, |x| x == "string")

&& iter.next().map_or(false, |x| x == "String")

})

Also, do you think that could be a good idea to add some names like second and third?

Suggested change

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")

})

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")

})

What do you think?

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

futursolo · 2023-08-22T11:37:46Z

packages/yew-macro/src/derive_props/field.rs

+pub(crate) fn is_path_a_string(path: &Path) -> bool {
+    is_path_segments_a_string(path.segments.iter().map(|ps| &ps.ident))


Suggested change

pub(crate) fn is_path_a_string(path: &Path) -> bool {

is_path_segments_a_string(path.segments.iter().map(|ps| &ps.ident))

pub(crate) fn is_path_a_string(path: &Path) -> bool {

use syn::parse_quote as q;

path == q!(String)

|| path == q!(alloc::string::String)

|| path == q!(std::string::String)

|| path == q!(::alloc::string::String)

|| path == q!(::std::string::String)

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.

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 cargo bench:

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

Whilst I do not believe the readability is the same (imagine if we need to add Rc<str>), if the first approach demonstrates better performance at this moment and it is not difficult to understand, I am fine with the current approach as well.

futursolo · 2023-08-22T12:03:38Z

I couldn't find a way to explicitly test if a warning is emitted by a derive macro

You can write a failing test in the packages/yew-macro/tests directory, which will check the stderr for the failed tests.

You can add #![deny(deprecated)] at the beginning of the test to make it an error.

its-the-shrimp · 2023-08-22T12:08:45Z

That's a great idea! I'll do that right away

its-the-shrimp · 2023-08-22T12:13:35Z

Wait, it doesn't emit a #[deprecated], I tried that but the warning message wasn't too user-friendly, I just used proc_macro_error::emit_warning!

futursolo · 2023-08-22T12:17:58Z

Wait, it doesn't emit a #[deprecated], I tried that but the warning message wasn't too user-friendly, I just used proc_macro_error::emit_warning!

Does this mean users will have no way to suppress this warning even if they want to?

its-the-shrimp · 2023-08-22T12:35:20Z

On the other hand, I'm not sure about emitting a #[deprecated] for something that's not supposed to be deprecated

ranile · 2023-08-22T12:45:10Z

The lints are supposed to be opt-in: #2882

futursolo · 2023-08-22T12:46:15Z

On the other hand, I'm not sure about emitting a #[deprecated] for something that's not supposed to be deprecated

If this is not a deprecation, then IMO, it should be classified as a lint (undesirable usage). Which means it is not in the scope of a procedural macro to do it but a tool like clippy.

its-the-shrimp · 2023-08-22T15:29:21Z

There's something wrong with the way lints are configured, yew-macro's Makefile seemed to have seen no changes after #2882 , and the lints themselves wouldn't appear no matter what I tried

added a warning for props fields of type String

bbfeb29

fixed clippy errors

97cecf3

its-the-shrimp mentioned this pull request Aug 21, 2023

added IntoPropValue implementation to convert from Cows #3346

Merged

2 tasks

cecton previously approved these changes Aug 21, 2023

View reviewed changes

packages/yew-macro/src/derive_props/field.rs Outdated Show resolved Hide resolved

ranile requested changes Aug 21, 2023

View reviewed changes

changed warning message, optimised is_path_(a_string/an_option) fun…

d7c868a

…ctions

its-the-shrimp dismissed cecton’s stale review via d7c868a August 21, 2023 23:21

its-the-shrimp added 2 commits August 22, 2023 00:46

replaced is_some_and with map_or

1a19956

fixed formatting

719edac

msga-mmm reviewed Aug 22, 2023

View reviewed changes

changed closure arguments' names

0504b16

futursolo reviewed Aug 22, 2023

View reviewed changes

made the warning an opt-in lint

c7bd63e

its-the-shrimp marked this pull request as draft December 5, 2023 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

its-the-shrimp commented Aug 21, 2023

github-actions bot commented Aug 21, 2023 •

edited

Loading

github-actions bot commented Aug 21, 2023 •

edited

Loading

github-actions bot commented Aug 21, 2023 •

edited

Loading

cecton left a comment

ranile left a comment

ranile Aug 21, 2023

its-the-shrimp commented Aug 21, 2023

msga-mmm Aug 22, 2023 •

edited

Loading

its-the-shrimp Aug 22, 2023 •

edited

Loading

futursolo Aug 22, 2023 •

edited

Loading

its-the-shrimp Aug 22, 2023

futursolo Aug 22, 2023

futursolo commented Aug 22, 2023

its-the-shrimp commented Aug 22, 2023

its-the-shrimp commented Aug 22, 2023

futursolo commented Aug 22, 2023

its-the-shrimp commented Aug 22, 2023

ranile commented Aug 22, 2023

futursolo commented Aug 22, 2023

its-the-shrimp commented Aug 22, 2023

		pub(crate) fn is_path_a_string(path: &Path) -> bool {
		is_path_segments_a_string(path.segments.iter().map(\|ps\| &ps.ident))

-pub(crate) fn is_path_a_string(path: &Path) -> bool {
-    is_path_segments_a_string(path.segments.iter().map(|ps| &ps.ident))
+pub(crate) fn is_path_a_string(path: &Path) -> bool {
+    use syn::parse_quote as q;
+    path == q!(String)
+        || path == q!(alloc::string::String)
+        || path == q!(std::string::String)
+        || path == q!(::alloc::string::String)
+        || path == q!(::std::string::String)

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

Are you sure you want to change the base?

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

Conversation

its-the-shrimp commented Aug 21, 2023

Description

Checklist

github-actions bot commented Aug 21, 2023 • edited Loading

github-actions bot commented Aug 21, 2023 • edited Loading

Size Comparison

github-actions bot commented Aug 21, 2023 • edited Loading

Benchmark - SSR

Yew Master

Pull Request

cecton left a comment

Choose a reason for hiding this comment

ranile left a comment

Choose a reason for hiding this comment

ranile Aug 21, 2023

Choose a reason for hiding this comment

its-the-shrimp commented Aug 21, 2023

msga-mmm Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

its-the-shrimp Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

futursolo Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

its-the-shrimp Aug 22, 2023

Choose a reason for hiding this comment

futursolo Aug 22, 2023

Choose a reason for hiding this comment

futursolo commented Aug 22, 2023

its-the-shrimp commented Aug 22, 2023

its-the-shrimp commented Aug 22, 2023

futursolo commented Aug 22, 2023

its-the-shrimp commented Aug 22, 2023

ranile commented Aug 22, 2023

futursolo commented Aug 22, 2023

its-the-shrimp commented Aug 22, 2023

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

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

github-actions bot commented Aug 21, 2023 •

edited

Loading

github-actions bot commented Aug 21, 2023 •

edited

Loading

github-actions bot commented Aug 21, 2023 •

edited

Loading

msga-mmm Aug 22, 2023 •

edited

Loading

its-the-shrimp Aug 22, 2023 •

edited

Loading

futursolo Aug 22, 2023 •

edited

Loading